Vinzenz Feenstra 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"? the force might be the wrong wording - Explicit might be more appropriate 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? Because closing a fd, according to the epoll manage causes it automatically to be unregistered. So I only care about this if we don't listen to events 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. No, no race - Not if it is not in self._channels self._channels only contains tracked fds - And that list is maintained by the same thread that checks this. So no, it's not caused by that. In theory there might be some other issue, ENOENT should not be happening if we're getting the event, however if it does happen we really shouldn't care about that error 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 conta I don't see a point in having ENOENT logged with a higher info level. ENOENT just says that the unregister failed because epoll doesn't know about the fd we passed 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. Well but it shouldn't - We don't know about it in _channels, that means epoll also shouldn't care about it 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
