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

nsoffer at redhat.com nsoffer at redhat.com
Mon Jun 30 09:36:34 UTC 2014


Nir Soffer has posted comments on this change.

Change subject: signals: Better handle signals to non-main threads
......................................................................


Patch Set 1:

(9 comments)

Nice! suggested some cleanup.

http://gerrit.ovirt.org/#/c/29392/1/lib/vdsm/utils.py
File lib/vdsm/utils.py:

Line 1141: 
Line 1142:     def prependDefer(self, func, *args, **kwargs):
Line 1143:         self._finally.insert(0, (func, args, kwargs))
Line 1144: 
Line 1145: 
utils.py is becoming little too big - maybe create a new signals module?
Line 1146: _signalPoller = None
Line 1147: 
Line 1148: 
Line 1149: def _getSignalPoller():


Line 1154: 
Line 1155:     This function has to be called from the main thread.
Line 1156:     '''
Line 1157:     if _signalPoller is None:
Line 1158:         (fd_r, fd_w) = os.pipe()
Lets use nicer names - read_fd, write_fd ?

And not use unneeded () - tuple is not defined by "()" but by the ",". For example:

    ("foo") - not a tuple
    "foo", - a tuple
Line 1159: 
Line 1160:         # The write end is going to be written to from a c-level signal
Line 1161:         # handler, so it is a good idea to make sure it does not block.
Line 1162:         flags = fcntl.fcntl(fd_w, fcntl.F_GETFL, 0)


Line 1157:     if _signalPoller is None:
Line 1158:         (fd_r, fd_w) = os.pipe()
Line 1159: 
Line 1160:         # The write end is going to be written to from a c-level signal
Line 1161:         # handler, so it is a good idea to make sure it does not block.
This is not "a good idea" - this is a requirement of signal.set_wakeup_fd api - make it clear in the comment.
Line 1162:         flags = fcntl.fcntl(fd_w, fcntl.F_GETFL, 0)
Line 1163:         flags |= os.O_NONBLOCK
Line 1164:         fcntl.fcntl(fd_w, fcntl.F_SETFL, flags)
Line 1165: 


Line 1162:         flags = fcntl.fcntl(fd_w, fcntl.F_GETFL, 0)
Line 1163:         flags |= os.O_NONBLOCK
Line 1164:         fcntl.fcntl(fd_w, fcntl.F_SETFL, flags)
Line 1165: 
Line 1166:         # This call must be done from the main thread.
Please document in the docstring of this function and waitForSignals that they must be called only from the main thread.

We can also raise if called from another thread.
Line 1167:         signal.set_wakeup_fd(fd_w)
Line 1168: 
Line 1169:         poller = select.poll()
Line 1170:         poller.register(fd_r, select.POLLIN)


Line 1168: 
Line 1169:         poller = select.poll()
Line 1170:         poller.register(fd_r, select.POLLIN)
Line 1171: 
Line 1172:         global _signalPoller
It is common to put global at the start of the function, which makes it clear that this function modify globals.
Line 1173:         _signalPoller = poller
Line 1174: 
Line 1175:     return _signalPoller
Line 1176: 


Line 1183:     '''
Line 1184:     poller = _getSignalPoller()
Line 1185:     try:
Line 1186:         events = poller.poll()
Line 1187:         for (fd, flags) in events:
The docs describe poll() as "returning tuple (fd, event) - lets use the terms used in the docs.

Lets simplify this to:

    for fd, event i poller.poll():
        ...
Line 1188:             if flags & select.POLLIN:
Line 1189:                 os.read(fd, 1)
Line 1190:     except select.error as e:
Line 1191:         # Signal was received in this thread while poll() was active


Line 1185:     try:
Line 1186:         events = poller.poll()
Line 1187:         for (fd, flags) in events:
Line 1188:             if flags & select.POLLIN:
Line 1189:                 os.read(fd, 1)
We better read couple of bytes, there is no point in waking up multiple times if several signals received at once.
Line 1190:     except select.error as e:
Line 1191:         # Signal was received in this thread while poll() was active
Line 1192:         if (e.args[0] == errno.EINTR):
Line 1193:             return


Line 1188:             if flags & select.POLLIN:
Line 1189:                 os.read(fd, 1)
Line 1190:     except select.error as e:
Line 1191:         # Signal was received in this thread while poll() was active
Line 1192:         if (e.args[0] == errno.EINTR):
e.args is assuming the structure of the error, while the docs promise that e is a pair: "The accompanying value is a pair containing the numeric error code from errno and the corresponding string"

Also we don't need the parenthesis. 

So we should use:

    if e[0] == errno.EINTR:

But the code would be more clear if we handle only the interesting case:

    if e[0] != errno.EINTR:
        logging.exception(...)
Line 1193:             return
Line 1194:         logging.error('Received error while waiting for signal',


Line 1191:         # Signal was received in this thread while poll() was active
Line 1192:         if (e.args[0] == errno.EINTR):
Line 1193:             return
Line 1194:         logging.error('Received error while waiting for signal',
Line 1195:                       exc_info=True)
Use standard logging.execption instead of non-standard vdsm exc_info.


-- 
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: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Dima Kuznetsov <dkuznets at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken 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: automation at ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list