Dima Kuznetsov has posted comments on this change. Change subject: signals: Handle signals to non-main threads ......................................................................
Patch Set 5: (7 comments) 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. Done 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. Done 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 Done 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. I'm open to ideas to make it better. My other alternative was to use time.sleep() to 'sync' but it is not a great way to do it. Line 44: Line 45: def testSignalReceived(self): Line 46: read_pipe, write_pipe = mp.Pipe() Line 47: 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) > its not really needed for a test I think using multiprocessing simplifies a lot of things. We don't have to worry about import paths, can share variables (including pipes) without putting them in env/args and monkeypatching is much easier this way. 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" Well, what I want to wait for, is for process to trigger wait_for_signal() because if it did not, we're not testing much as the signal will get handled before the call and child will hang forever. 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 ot Will add this test case -- 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
