Francesco Romani has posted comments on this change.
Change subject: utils: add CommandStream class ......................................................................
Patch Set 2:
(9 comments)
preliminary review - easy parts first
http://gerrit.ovirt.org/#/c/33909/2/lib/vdsm/utils.py File lib/vdsm/utils.py:
Line 327: if endtime is not None: Line 328: timeout = max(0, endtime - time.time()) Line 329: Line 330: Line 331: class CommandStream(object): A docstring would be helpful, mostly to understand why and when use this class, comparing for example with AsyncProc Line 332: def __init__(self, command, stdoutcb, stderrcb, cwd=None, Line 333: deathSignal=0): Line 334: self.command = command Line 335: self.returncode = None
Line 330: Line 331: class CommandStream(object): Line 332: def __init__(self, command, stdoutcb, stderrcb, cwd=None, Line 333: deathSignal=0): Line 334: self.command = command all the members could be made _private Line 335: self.returncode = None Line 336: Line 337: self.child = CPopen(self.command, cwd=cwd, close_fds=True, Line 338: deathSignal=deathSignal)
Line 343: self.child.stdout.fileno(): stdoutcb, Line 344: self.child.stderr.fileno(): stderrcb, Line 345: } Line 346: Line 347: for fd in self.iocb.keys(): why not for fd in self.iocb ? Line 348: self.epoll.register(fd, select.EPOLLIN) Line 349: Line 350: def terminate(self): Line 351: self.child.terminate()
Line 381: # file descriptor was closed because the process Line 382: # terminated. Line 383: self.returncode = self.child.poll() Line 384: Line 385: def wait(self, timeout=None): despite your explanation (thanks for that) I still find the 'timeout' usage a bit confusing Line 386: if timeout is None: Line 387: epoll_remaining = -1 Line 388: else: Line 389: endtime = os.times()[4] + timeout
Line 385: def wait(self, timeout=None): Line 386: if timeout is None: Line 387: epoll_remaining = -1 Line 388: else: Line 389: endtime = os.times()[4] + timeout usage of os.times() could be surprising (I must admit it was like this for me), better to extract a tiny helper function for the sake of readability like mine http://gerrit.ovirt.org/#/c/35350/ - just consider it as example of what I mean Line 390: Line 391: while self.returncode is None: Line 392: if timeout is not None: Line 393: epoll_remaining = endtime - os.times()[4]
Line 393: epoll_remaining = endtime - os.times()[4] Line 394: if epoll_remaining <= 0: Line 395: break Line 396: Line 397: if len(self.iocb): why not if self.iocb ? Line 398: self._epoll_timeout(epoll_remaining) Line 399: else: Line 400: # This is a busy-loop taken from issue5673, and Line 401: # python 3.4 still uses this implementation.
http://gerrit.ovirt.org/#/c/33909/2/tests/utilsTests.py File tests/utilsTests.py:
Line 639: def test_empty(self): Line 640: self.assertEquals(utils._list2cmdline([]), "") Line 641: Line 642: Line 643: class CommandStreamTests(TestCaseBase): tests are nice, but for the sake of readability I'd suggest to put short summaries for each of them as docstrings. Line 644: Line 645: @contextmanager Line 646: def assertElapsed(self, expected, tolerance=0.5): Line 647: start = os.times()[4]
Line 663: Line 664: def recv_stdout(buffer): Line 665: # cannot use received += buffer with a variable Line 666: # defined in the parent function. Line 667: operator.iadd(received, buffer) what about the bytearray.extend() method? Line 668: Line 669: p = utils.CommandStream(["echo", "-n", text], Line 670: recv_stdout, Line 671: self.assertNoOutput)
Line 701: self.assertNoOutput, Line 702: self.assertNoOutput) Line 703: Line 704: with self.assertElapsed(2): Line 705: retcode = p.wait(2) this indeed suggests the wait() timeout parameter means "wait at least X time units" which may be misleading Line 706: Line 707: self.assertEqual(retcode, None) Line 708: Line 709: p.terminate()