Nir Soffer has posted comments on this change. Change subject: signals: Handle signals to non-main threads ......................................................................
Patch Set 5: (8 comments) Nice, there are some tiny thing that could be cleaned up, and I think the tests should be simplified and test some more cases. http://gerrit.ovirt.org/#/c/29392/5/lib/vdsm/sigutils.py File lib/vdsm/sigutils.py: Line 82: if event & select.POLLIN: Line 83: try: Line 84: os.read(fd, 128) Line 85: except OSError as e: Line 86: if (e.errno not in [errno.EAGAIN, errno.EINTR]): - The parenthesis are not needed, this just noise. - The list of expected errors should be a tuple, as you do not plan to mutate it. Line 87: logging.exception( Line 88: 'Received error while reading from pipe') Line 89: except select.error as e: Line 90: if (e[0] != errno.EINTR): Line 86: if (e.errno not in [errno.EAGAIN, errno.EINTR]): Line 87: logging.exception( Line 88: 'Received error while reading from pipe') Line 89: except select.error as e: Line 90: if (e[0] != errno.EINTR): The parenthesis are not needed, this just noise. http://gerrit.ovirt.org/#/c/29392/5/tests/signalTests.py File tests/signalTests.py: Line 1: # Please rename this to sigutilsTests - we should be consistent with our test module names. Line 2: # Copyright 2014 Red Hat, Inc. Line 3: # Line 4: # This program is free software; you can redistribute it and/or modify Line 5: # it under the terms of the GNU General Public License as published by Line 39: if syscall_nr in [7, 168]: # These are __NR_poll for Line 40: # x86_64 and i686 Line 41: break Line 42: except IOError: Line 43: pass This is too fragile. Line 44: Line 45: def testSignalReceived(self): Line 46: read_pipe, write_pipe = mp.Pipe() Line 47: Line 46: read_pipe, write_pipe = mp.Pipe() Line 47: Line 48: def child_process(): Line 49: signal.signal(signal.SIGUSR1, Line 50: lambda *_: write_pipe.send('one')) You can do this here, and wait for this on the parent: write_pipe.send('ready') Line 51: sigutils.wait_for_signal() Line 52: write_pipe.send('two') Line 53: Line 54: proc = mp.Process(target=child_process) Line 50: lambda *_: write_pipe.send('one')) Line 51: sigutils.wait_for_signal() Line 52: write_pipe.send('two') Line 53: Line 54: proc = mp.Process(target=child_process) I would not use multiprocessing, this just add uneeded complication and dependencies. Why not run a child process using builtin subprocess, and communicate over pipes open as the child process stdin and stdout? Line 55: proc.start() Line 56: self._waitForPoll(proc.pid) Line 57: os.kill(proc.pid, signal.SIGUSR1) Line 58: self.assertEquals(read_pipe.recv(), 'one') Line 52: write_pipe.send('two') Line 53: Line 54: proc = mp.Process(target=child_process) Line 55: proc.start() Line 56: self._waitForPoll(proc.pid) If you want to wait until the child process is ready, why not send "ready" on the pipe, and read it on the other side? Line 57: os.kill(proc.pid, signal.SIGUSR1) Line 58: self.assertEquals(read_pipe.recv(), 'one') Line 59: proc.join() Line 60: self.assertEquals(read_pipe.recv(), 'two') Line 95: self._waitForPoll(proc.pid) Line 96: os.kill(proc.pid, signal.SIGUSR1) Line 97: self.assertEqual(read_pipe.recv(), 'one') Line 98: proc.join() Line 99: self.assertEquals(read_pipe.recv(), 'two') You are not testing the most interesting thing - that getting signals on other threads wakes up the main thread. A nice test would be to start a child process on the child, and have it exit and check that the parent report the sigchld on the pipe. -- 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: 5 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: [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
