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 <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Francesco Romani <[email protected]> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Polednik <[email protected]> Gerrit-Reviewer: Nir Soffer <[email protected]> Gerrit-Reviewer: Vinzenz Feenstra <[email protected]> Gerrit-Reviewer: gerrit-hooks <[email protected]> Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
