Nir Soffer has posted comments on this change. Change subject: virt: Forcibly unregister fds with errors from epoll ......................................................................
Patch Set 9: (5 comments) https://gerrit.ovirt.org/#/c/42570/9/vdsm/virt/vmchannels.py File vdsm/virt/vmchannels.py: Line 58: else: Line 59: self.log.debug("Received %.08X. On fd removed by epoll.", Line 60: event) Line 61: # Try to forcibly remove this FD from EPOLL Line 62: # To really stop receiving events Why forcibly? this looks like normal usage of epoll, where is the "force"? Line 63: try: Line 64: self._epoll.unregister(fileno) Line 65: except IOError as e: Line 66: # Ignore ENOENT Line 60: event) Line 61: # Try to forcibly remove this FD from EPOLL Line 62: # To really stop receiving events Line 63: try: Line 64: self._epoll.unregister(fileno) Why do this only if fileno is not in self._channels? Line 65: except IOError as e: Line 66: # Ignore ENOENT Line 67: if e.errno != errno.ENOENT: Line 68: raise Line 62: # To really stop receiving events Line 63: try: Line 64: self._epoll.unregister(fileno) Line 65: except IOError as e: Line 66: # Ignore ENOENT The comment is not needed, the code explains it self. What can be useful is why do we need to ignore ENOENT - I guess because the race with another thread calling stop(), right? Line 67: if e.errno != errno.ENOENT: Line 68: raise Line 69: else: Line 70: self.log.debug('Forcibly unregistering stray fd %d ' Line 67: if e.errno != errno.ENOENT: Line 68: raise Line 69: else: Line 70: self.log.debug('Forcibly unregistering stray fd %d ' Line 71: 'from epoll FAILED', fileno) This should be a log.error, and more important, we want to log e - it contains an error code and error message which may help to understand the issue. Line 72: else: Line 73: self.log.debug('Forcibly unregistering stray fd %d from ' Line 74: 'epoll SUCCEEDED', fileno) Line 75: elif (event & select.EPOLLIN): Line 70: self.log.debug('Forcibly unregistering stray fd %d ' Line 71: 'from epoll FAILED', fileno) Line 72: else: Line 73: self.log.debug('Forcibly unregistering stray fd %d from ' Line 74: 'epoll SUCCEEDED', fileno) Why stray? this is just another fd monitored by epoll. Line 75: elif (event & select.EPOLLIN): Line 76: obj = self._channels.get(fileno, None) Line 77: if obj: Line 78: obj['timeout_seen'] = False -- To view, visit https://gerrit.ovirt.org/42570 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ic4a452f7837267434bc836fced4c2efd92d745cf Gerrit-PatchSet: 9 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Vinzenz Feenstra <vfeen...@redhat.com> Gerrit-Reviewer: Dan Kenigsberg <dan...@redhat.com> Gerrit-Reviewer: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Gal Hammer <ghammer%redhat....@gtempaccount.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Michal Skrivanek <michal.skriva...@redhat.com> Gerrit-Reviewer: Michal Skrivanek <mskri...@redhat.com> Gerrit-Reviewer: Nir Soffer <nsof...@redhat.com> Gerrit-Reviewer: Vinzenz Feenstra <vfeen...@redhat.com> Gerrit-Reviewer: gerrit-hooks <automat...@ovirt.org> Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
