Nir Soffer has posted comments on this change. Change subject: signals: Handle signals to non-main threads ......................................................................
Patch Set 6: (5 comments) http://gerrit.ovirt.org/#/c/29392/6/tests/sigutilsTests.py File tests/sigutilsTests.py: Line 46: stdout=subprocess.PIPE) Line 47: self.assertRead(popen.stdout, 'ready\n') Line 48: self.assertRead(popen.stdout, 'done\n') Line 49: Line 50: def testSignal10Times(self): > yes, want to make sure it works more than once (because 1st call is a bit d Right - so lets make two calls, and add a comment explaining the difference between the first and second call. Line 51: TIMES = 10 Line 52: popen = subprocess.Popen([CHILD_SCRIPT, Line 53: 'testSignalTimes', Line 54: str(TIMES)], Line 54: str(TIMES)], Line 55: stdout=subprocess.PIPE) Line 56: self.assertRead(popen.stdout, 'ready\n') Line 57: for _ in range(TIMES): Line 58: time.sleep(0.1) > yes, because upon event, we read all the bytes written to pipe, so i can fi I think that instead of the sleep, you simply should send a signal, wait for the message "signal\n", then wait for the message "wokeup" (which I suggested to add in the helper). Then you don't need to sleep - unless you want to make sure that the other process is waiting inside wait_for_signal, but I don't know if we care about it. Anway, if we send only 2 signals, little sleep here does not matter. Line 59: popen.send_signal(signal.SIGUSR1) Line 60: self.assertRead(popen.stdout, 'signal\n') Line 61: self.assertRead(popen.stdout, 'done\n') Line 62: http://gerrit.ovirt.org/#/c/29392/6/tests/sigutilsTests_child.py File tests/sigutilsTests_child.py: Line 13: Line 14: Line 15: def sighandler(signum, frame): Line 16: counter[0] += 1 Line 17: sys.stdout.write('signal\n') > I am running python with unbuffered stdin/out (-u flag). A bit hacky but I Nice, did not know about that flag. Using python instead of extra code is nice, although less clear. Choose what you like. Line 18: Line 19: Line 20: def testSignalReceived(): Line 21: signal.signal(signal.SIGUSR1, sighandler) Line 36: signal.signal(signal.SIGUSR1, sighandler) Line 37: sys.stdout.write('ready\n') Line 38: while counter[0] < times: Line 39: sigutils.wait_for_signal() Line 40: sys.stdout.write('done\n') > I think it over complicates and is unnecessary for tests simple as those (f If you worried about timeout firing twice, make it larger? Or - call wait_for_signal() twice: wait_for_signal() send_to_parent('wokeup 1') wait_for_signal() send_to_parent('wokeup 2') Line 41: Line 42: Line 43: def testSignalToThread(): Line 44: ''' Line 65: 'testSignalTimes': testSignalTimes, Line 66: 'testSignalToThread': testSignalToThread} Line 67: Line 68: if __name__ == '__main__': Line 69: tests[sys.argv[1]](*sys.argv[2:]) > I use some arguments Right - but we don't need those - there is no reason to pass the number of times to loop, just send sigterm to exit when you are bored sending signals in the parent. -- 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: 6 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
