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()
--
To view, visit
http://gerrit.ovirt.org/33909
To unsubscribe, visit
http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie015368bb9c5992e5c73a149277c59fc4ffbd570
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com>
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes