Nir Soffer has posted comments on this change. Change subject: signals: Handle signals to non-main threads ......................................................................
Patch Set 15: (4 comments) Need to remove the return value of register. Other than that, it can be simpler. This code try to do things that we don't need and try to handle corner cases which are not worth the time. http://gerrit.ovirt.org/#/c/29392/15/lib/vdsm/sigutils.py File lib/vdsm/sigutils.py: Line 66: Line 67: _signal_poller = poller Line 68: _signal_read_fd = read_fd Line 69: Line 70: return _signal_poller You don't want to return internal stuff like this. Line 71: Line 72: Line 73: def wait_for_signal(timeout=None): Line 74: ''' Line 96: except select.error as e: Line 97: if e[0] == errno.EINTR: Line 98: interrupted = True Line 99: else: Line 100: logging.exception('Received error while waiting for signal') Do we expect any error here? What if we get unexpected error in a loop? I think that we should never get any error here that we can handle, so the caller must handle it. We can also simplify the code, replacing interrupted and events with one boolean like this: try: cleanup = _signal_poller.poll(timeout) except select.error as e: if e[0] != errno.EINTR: raise cleanup = True if cleanup: _empty_pipe() This would be even simpler if timeout is not used - we don't use timeout, and nobody asked to provide a timeout - why do you do more work then needed? Here without a timeout: try: _signal_poller.poll() except select.error as e: if e[0] != errno.EINTR: raise _empty_pipe() Why do we need to maintain more code then this? Line 101: Line 102: if events or interrupted: Line 103: _empty_pipe(_signal_read_fd) Line 104: Line 102: if events or interrupted: Line 103: _empty_pipe(_signal_read_fd) Line 104: Line 105: Line 106: def _read(fd, len): Why put this before _empty_pipe? this is implementation detail of _empty_pipe. Line 107: """ Line 108: Read that handles recoverable exceptions. Line 109: """ Line 110: while True: Line 123: try: Line 124: while len(_read(fd, 128)) == 128: Line 125: pass Line 126: except OSError: Line 127: logging.exception('Received error while reading from pipe') Do we really want to log exception here? What errors are expected here? If we don't expect any error, the impossible error should be handled by the caller, to avoid busy loops. And why do we need to get the fd, this is the scope of the module and we can use _signal_read_fd just like in a class you use self.xxx. This is not reusable function, it should be specific to this module. Removing exception handling, we can inline _read into _empty_pipe: def _empty_pipe(): while True: try: data = os.read(_signal_read_fd, 128) except OSError as e: if e.errno == errno.EINTR: continue # Signal received - likely to have data elif e.errno == errno.EAGAIN: return # No more data at this time else: raise else: if len(data) != 128: return # No more data available This is of course much too complicated, I would just use: def _empty_pipe(): try: os.read(_signal_read_fd, 128) except OSError as e: if e.errno not in (errno.EAGAIN, errno.EINTR): raise Because we don't care about the highly unlikely cases where os.read() is interrupted or invoked after more then 128 signals are received. -- To view, visit http://gerrit.ovirt.org/29392 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I5dbcd00cec22ef12f2b6253b016dcbd0aa889583 Gerrit-PatchSet: 15 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Dima Kuznetsov <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Dima Kuznetsov <[email protected]> Gerrit-Reviewer: Nir Soffer <[email protected]> Gerrit-Reviewer: Saggi Mizrahi <[email protected]> Gerrit-Reviewer: Yaniv Bronhaim <[email protected]> Gerrit-Reviewer: Yeela Kaplan <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Reviewer: mooli tayer <[email protected]> Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
