Change in vdsm[master]: signals: Handle signals to non-main threads

nsoffer at redhat.com nsoffer at redhat.com
Mon Oct 20 08:43:36 UTC 2014


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 <dkuznets at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Dima Kuznetsov <dkuznets at redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi at redhat.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei at redhat.com>
Gerrit-Reviewer: Yeela Kaplan <ykaplan at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-Reviewer: mooli tayer <mtayer at redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list