Nir Soffer has posted comments on this change.
Change subject: utils: add CommandStream class ......................................................................
Patch Set 3:
(18 comments)
http://gerrit.ovirt.org/#/c/33909/3//COMMIT_MSG Commit Message:
Line 4: Commit: Federico Simoncelli fsimonce@redhat.com Line 5: CommitDate: 2014-12-17 21:53:58 +0000 Line 6: Line 7: utils: add CommandStream class Line 8: Can we have some information on why this class is needed? Line 9: Change-Id: Ie015368bb9c5992e5c73a149277c59fc4ffbd570
http://gerrit.ovirt.org/#/c/33909/3/lib/vdsm/utils.py File lib/vdsm/utils.py:
Line 328: if endtime is not None: Line 329: timeout = max(0, endtime - time.time()) Line 330: Line 331: Line 332: class CommandStream(object): Looking at the tests, the only interesting public method is receive() - this does not look like a stream. How about naming it CommandReceiver or StreamReceiver? Line 333: def __init__(self, command, stdoutcb, stderrcb): Line 334: self._command = command Line 335: self.epoll = select.epoll() Line 336:
Line 335: self.epoll = select.epoll() Line 336: Line 337: self.iocb = { Line 338: self._command.stdout.fileno(): stdoutcb, Line 339: self._command.stderr.fileno(): stderrcb, Will this work when we redirect stderr to stdout (do we have same fileno for both files?) Line 340: } Line 341: Line 342: for fd in self.iocb: Line 343: self.epoll.register(fd, select.EPOLLIN)
Line 341: Line 342: for fd in self.iocb: Line 343: self.epoll.register(fd, select.EPOLLIN) Line 344: Line 345: def _epoll_input(self, fileno): I commented about this name in a previous version. Line 346: self.iocb[fileno](os.read(fileno, io.DEFAULT_BUFFER_SIZE)) Line 347: Line 348: def _epoll_event(self, fileno): Line 349: self.epoll.unregister(fileno)
Line 344: Line 345: def _epoll_input(self, fileno): Line 346: self.iocb[fileno](os.read(fileno, io.DEFAULT_BUFFER_SIZE)) Line 347: Line 348: def _epoll_event(self, fileno): I commented about this name in a previous version. Line 349: self.epoll.unregister(fileno) Line 350: del self.iocb[fileno] Line 351: Line 352: def _epoll_timeout(self, timeout):
http://gerrit.ovirt.org/#/c/33909/3/tests/utilsTests.py File tests/utilsTests.py:
Line 679: class CommandStreamTests(TestCaseBase): Line 680: Line 681: @contextmanager Line 682: def assertElapsed(self, expected, tolerance=0.5): Line 683: start = os.times()[4] Should use new utils.monotonic_time() Line 684: Line 685: yield Line 686: Line 687: elapsed = os.times()[4] - start
Line 686: Line 687: elapsed = os.times()[4] - start Line 688: Line 689: if (elapsed < expected - tolerance Line 690: or elapsed > expected + tolerance): How about:
if abs(elapsed - expected) > tolerance: raise ... Line 691: raise AssertionError("Operation time: %s" % elapsed) Line 692: Line 693: def assertNoOutput(self, data): Line 694: raise AssertionError("Unexpected data: " + repr(data))
Line 689: if (elapsed < expected - tolerance Line 690: or elapsed > expected + tolerance): Line 691: raise AssertionError("Operation time: %s" % elapsed) Line 692: Line 693: def assertNoOutput(self, data): This does not assert anything about output or data. Maybe call this assertUnexpectedCall? Line 694: raise AssertionError("Unexpected data: " + repr(data)) Line 695: Line 696: def _command(self, command): Line 697: return CPopen(command)
Line 690: or elapsed > expected + tolerance): Line 691: raise AssertionError("Operation time: %s" % elapsed) Line 692: Line 693: def assertNoOutput(self, data): Line 694: raise AssertionError("Unexpected data: " + repr(data)) And change this to "Unexpected call with data: %r" % data Line 695: Line 696: def _command(self, command): Line 697: return CPopen(command) Line 698:
Line 692: Line 693: def assertNoOutput(self, data): Line 694: raise AssertionError("Unexpected data: " + repr(data)) Line 695: Line 696: def _command(self, command): Rename to _startCommand? Line 697: return CPopen(command) Line 698: Line 699: def test_output(self): Line 700: text = "Hello World"
Line 695: Line 696: def _command(self, command): Line 697: return CPopen(command) Line 698: Line 699: def test_output(self): Rename to test_receive_stdout? Line 700: text = "Hello World" Line 701: received = bytearray() Line 702: Line 703: def recv_stdout(buffer):
Line 696: def _command(self, command): Line 697: return CPopen(command) Line 698: Line 699: def test_output(self): Line 700: text = "Hello World" We need another tests for real world output, maybe many lines (/usr/bin/yes?) Line 701: received = bytearray() Line 702: Line 703: def recv_stdout(buffer): Line 704: # cannot use received += buffer with a variable
Line 702: Line 703: def recv_stdout(buffer): Line 704: # cannot use received += buffer with a variable Line 705: # defined in the parent function. Line 706: operator.iadd(received, buffer) Too complicated.
Why not:
self.received = bytearray()
def recv_stdout(buffer): self.received += buffer Line 707: Line 708: c = self._command(["echo", "-n", text]) Line 709: p = utils.CommandStream(c, recv_stdout, self.assertNoOutput) Line 710:
Line 711: p.receive() Line 712: retcode = c.wait() Line 713: Line 714: self.assertEqual(retcode, 0) Line 715: self.assertEqual(text, str(received)) Works without converting the str. If you want to be more explicit, we can use text = bytes("Hello World") or b"Hello World". Line 716: Line 717: def test_write(self): Line 718: text = "Hello World" Line 719: received = bytearray()
Line 720: Line 721: def recv_stdout(buffer): Line 722: # cannot use received += buffer with a variable Line 723: # defined in the parent function. Line 724: operator.iadd(received, buffer) Same as above, but this time it is also duplicated :-) Line 725: Line 726: c = self._command(["cat"]) Line 727: p = utils.CommandStream(c, recv_stdout, self.assertNoOutput) Line 728:
Line 730: c.stdin.flush() Line 731: c.stdin.close() Line 732: Line 733: while not p.closed: Line 734: p.receive() Why do we need here a loop and we don't need one in test_output? Line 735: Line 736: retcode = c.wait() Line 737: Line 738: self.assertEqual(retcode, 0)
Line 741: def test_timeout(self): Line 742: c = self._command(["sleep", "5"]) Line 743: p = utils.CommandStream(c, self.assertNoOutput, self.assertNoOutput) Line 744: Line 745: with self.assertElapsed(2): If this assert fails, we never wait for the process, keeping a zombie until the test process exit. Line 746: p.receive(2) Line 747: Line 748: self.assertEqual(p.closed, False) Line 749:
Line 770: Line 771: with self.assertElapsed(0): Line 772: p.receive(2) Line 773: Line 774: self.assertEqual(c.wait(), -signal.SIGKILL) We need more tests:
- Receiving from both stdout and stderr - Redirecting stderr to stdout - Interacting with a command (shell?), writing commands to stdin, receiving output and errors from process - stdout closed while receiving, continue to receive from stderr - both stdout and stderr closed while receiving - read from stdout fails during receive - how the caller can handle this error? maybe we need an errorcb?