Nir Soffer has posted comments on this change. Change subject: signals: Handle signals to non-main threads ......................................................................
Patch Set 16: (3 comments) http://gerrit.ovirt.org/#/c/29392/16/lib/vdsm/sigutils.py File lib/vdsm/sigutils.py: Line 18: # Refer to the README and COPYING files for full details of the license Line 19: # Line 20: Line 21: import errno Line 22: import logging Since both logs here are not needed, you can remove this import. Line 23: import os Line 24: import select Line 25: import signal Line 26: Line 89: try: Line 90: cleanup = [] != _signal_poller.poll(timeout) Line 91: except select.error as e: Line 92: if e[0] != errno.EINTR: Line 93: logging.exception('Received error while waiting for signal') No need to log if you raise. You did everything you can so the caller have the complete stack trace. If it ignore the exception it is the caller problem. Line 94: raise Line 95: cleanup = True Line 96: Line 97: if cleanup: Line 117: try: Line 118: while len(_read(_signal_read_fd, 128)) == 128: Line 119: pass Line 120: except OSError: Line 121: logging.exception('Received error while reading from pipe') Again no need to log when you raise - in particular no an exception. Why do you want to have 2 backtraces in the log for this error? -- 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: 16 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
