Change in vdsm[master]: virt: Correct epoll unregistration usage in vmchannels

vfeenstr at redhat.com vfeenstr at redhat.com
Fri Jan 8 15:22:34 UTC 2016


Vinzenz Feenstra has posted comments on this change.

Change subject: virt: Correct epoll unregistration usage in vmchannels
......................................................................


Patch Set 2:

(5 comments)

https://gerrit.ovirt.org/#/c/51521/2/vdsm/virt/vmchannels.py
File vdsm/virt/vmchannels.py:

Line 53
Line 54
Line 55
Line 56
Line 57
> But in this branch we are doing nothing - are you assuming that this case i
Ok, I will


Line 75
Line 76
Line 77
Line 78
Line 79
> In another pathc, please fix the indentation here, the entire body is inden
ok


Line 113
Line 114
Line 115
Line 116
Line 117
> Why not unregister here, just before removing the channel?
Because the fd will be closed already at this time and cause an error - and then the above ENOENT will for sure happen


Line 53:             self._epoll.unregister(fileno)
Line 54:         except IOError as e:
Line 55:             # We do not care if it has been previously removed from epoll
Line 56:             # or is not tracked by epoll. The removal is exactly what we
Line 57:             # want to achieve.
> Can you explain the flow where this can happen?
Well there shouldn't be any flow anymore where this can happen, it's just that if it would be happening we don't want the thread to exit and cease all communications. It's a prevention for MAYBE issues. However I really hope it won't happen, I can't think of a reason right now.
Line 58:             if e.errno != errno.ENOENT:
Line 59:                 raise
Line 60: 
Line 61:     def _handle_event(self, fileno, event):


Line 223:         """ Unregister an exist file descriptor from the listener. """
Line 224:         self.log.debug("Delete fileno %d from listener.", fileno)
Line 225:         with self._update_lock:
Line 226:             # Threadsafe, fileno will be closed by caller
Line 227:             self._unregister_fd(fileno)
> Why unregister separately from del_channel?
Because this is called by GuestAgent.stop and that will close the socket right after this.
And would be wrong not to unregister it here already otherwise it will cause the ENOENT exception happening above.


-- 
To view, visit https://gerrit.ovirt.org/51521
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I9f7eab8318f41f653e0a24552c81bcd5b09d8690
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Vinzenz Feenstra <vfeenstr at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik <mpolednik at redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Vinzenz Feenstra <vfeenstr at redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation at ovirt.org>
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list