Nir Soffer has posted comments on this change. Change subject: virt: Correct epoll unregistration usage in vmchannels ......................................................................
Patch Set 2: (6 comments) https://gerrit.ovirt.org/#/c/51521/2/vdsm/virt/vmchannels.py File vdsm/virt/vmchannels.py: Line 52 Line 53 Line 54 Line 55 Line 56 In this branch, we will unregister the fd during the reconnect process. Line 53 Line 54 Line 55 Line 56 Line 57 But in this branch we are doing nothing - are you assuming that this case is impossible? If you do, please log a warning in this case. Line 75 Line 76 Line 77 Line 78 Line 79 In another pathc, please fix the indentation here, the entire body is indented too much. Line 113 Line 114 Line 115 Line 116 Line 117 Why not unregister here, just before removing the channel? 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? 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? -- 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
