Change in vdsm[master]: utils: add CommandStream class
automat...@ovirt.org has posted comments on this change. Change subject: utils: add CommandStream class .. Patch Set 11: * Update tracker::IGNORE, no Bug-Url found * Set MODIFIED::IGNORE, no Bug-Url found. -- To view, visit https://gerrit.ovirt.org/33909 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ie015368bb9c5992e5c73a149277c59fc4ffbd570 Gerrit-PatchSet: 11 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Federico Simoncelli Gerrit-Reviewer: Adam Litke Gerrit-Reviewer: Ala Hino Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Freddy Rolland Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Piotr Kliczewski Gerrit-Reviewer: Saggi Mizrahi Gerrit-Reviewer: Shahar Havivi Gerrit-Reviewer: automat...@ovirt.org Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: utils: add CommandStream class
Dan Kenigsberg has submitted this change and it was merged. Change subject: utils: add CommandStream class .. utils: add CommandStream class This patch adds CommandStream a generic class to process the output of external commands (both stdout and stderr) as soon as available. For this purpose CommandStream provides the ability to define callbacks that will be executed when data is available from the external command. This class is the foundation for long-running commands that are able to provide their progress during execution: their output must be processed as soon as possible to maintain and report status updates. Change-Id: Ie015368bb9c5992e5c73a149277c59fc4ffbd570 Signed-off-by: Federico Simoncelli Reviewed-on: https://gerrit.ovirt.org/33909 Reviewed-by: Adam Litke Reviewed-by: Nir Soffer --- M lib/vdsm/utils.py M tests/testlib.py M tests/utilsTests.py 3 files changed, 171 insertions(+), 0 deletions(-) Approvals: Nir Soffer: Looks good to me, but someone else must approve Adam Litke: Looks good to me, approved Federico Simoncelli: Verified -- To view, visit https://gerrit.ovirt.org/33909 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: merged Gerrit-Change-Id: Ie015368bb9c5992e5c73a149277c59fc4ffbd570 Gerrit-PatchSet: 11 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Federico Simoncelli Gerrit-Reviewer: Adam Litke Gerrit-Reviewer: Ala Hino Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Freddy Rolland Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Piotr Kliczewski Gerrit-Reviewer: Saggi Mizrahi Gerrit-Reviewer: Shahar Havivi Gerrit-Reviewer: automat...@ovirt.org ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: utils: add CommandStream class
Federico Simoncelli has posted comments on this change. Change subject: utils: add CommandStream class .. Patch Set 10: Verified+1 -- To view, visit https://gerrit.ovirt.org/33909 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ie015368bb9c5992e5c73a149277c59fc4ffbd570 Gerrit-PatchSet: 10 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Federico Simoncelli Gerrit-Reviewer: Adam Litke Gerrit-Reviewer: Ala Hino Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Freddy Rolland Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Piotr Kliczewski Gerrit-Reviewer: Saggi Mizrahi Gerrit-Reviewer: Shahar Havivi Gerrit-Reviewer: automat...@ovirt.org Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: utils: add CommandStream class
Nir Soffer has posted comments on this change. Change subject: utils: add CommandStream class .. Patch Set 10: Code-Review+1 -- To view, visit https://gerrit.ovirt.org/33909 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ie015368bb9c5992e5c73a149277c59fc4ffbd570 Gerrit-PatchSet: 10 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Federico Simoncelli Gerrit-Reviewer: Adam Litke Gerrit-Reviewer: Ala Hino Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Freddy Rolland Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Piotr Kliczewski Gerrit-Reviewer: Saggi Mizrahi Gerrit-Reviewer: Shahar Havivi Gerrit-Reviewer: automat...@ovirt.org Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: utils: add CommandStream class
Adam Litke has posted comments on this change. Change subject: utils: add CommandStream class .. Patch Set 10: Code-Review+2 -- To view, visit https://gerrit.ovirt.org/33909 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ie015368bb9c5992e5c73a149277c59fc4ffbd570 Gerrit-PatchSet: 10 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Federico Simoncelli Gerrit-Reviewer: Adam Litke Gerrit-Reviewer: Ala Hino Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Freddy Rolland Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Piotr Kliczewski Gerrit-Reviewer: Saggi Mizrahi Gerrit-Reviewer: Shahar Havivi Gerrit-Reviewer: automat...@ovirt.org Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: utils: add CommandStream class
automat...@ovirt.org has posted comments on this change. Change subject: utils: add CommandStream class .. Patch Set 10: * Update tracker::IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3']) -- To view, visit https://gerrit.ovirt.org/33909 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ie015368bb9c5992e5c73a149277c59fc4ffbd570 Gerrit-PatchSet: 10 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Federico Simoncelli Gerrit-Reviewer: Adam Litke Gerrit-Reviewer: Ala Hino Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Freddy Rolland Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Piotr Kliczewski Gerrit-Reviewer: Saggi Mizrahi Gerrit-Reviewer: Shahar Havivi Gerrit-Reviewer: automat...@ovirt.org Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: utils: add CommandStream class
Adam Litke has posted comments on this change. Change subject: utils: add CommandStream class .. Patch Set 9: Code-Review+2 Looks good. -- To view, visit https://gerrit.ovirt.org/33909 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ie015368bb9c5992e5c73a149277c59fc4ffbd570 Gerrit-PatchSet: 9 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Federico Simoncelli Gerrit-Reviewer: Adam Litke Gerrit-Reviewer: Ala Hino Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Freddy Rolland Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Piotr Kliczewski Gerrit-Reviewer: Saggi Mizrahi Gerrit-Reviewer: Shahar Havivi Gerrit-Reviewer: automat...@ovirt.org Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: utils: add CommandStream class
Nir Soffer has posted comments on this change. Change subject: utils: add CommandStream class .. Patch Set 9: Code-Review+1 -- To view, visit https://gerrit.ovirt.org/33909 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ie015368bb9c5992e5c73a149277c59fc4ffbd570 Gerrit-PatchSet: 9 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Federico Simoncelli Gerrit-Reviewer: Adam Litke Gerrit-Reviewer: Ala Hino Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Freddy Rolland Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Piotr Kliczewski Gerrit-Reviewer: Saggi Mizrahi Gerrit-Reviewer: Shahar Havivi Gerrit-Reviewer: automat...@ovirt.org Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: utils: add CommandStream class
automat...@ovirt.org has posted comments on this change. Change subject: utils: add CommandStream class .. Patch Set 9: * Update tracker::IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3']) -- To view, visit https://gerrit.ovirt.org/33909 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ie015368bb9c5992e5c73a149277c59fc4ffbd570 Gerrit-PatchSet: 9 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Federico Simoncelli Gerrit-Reviewer: Adam Litke Gerrit-Reviewer: Ala Hino Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Freddy Rolland Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Piotr Kliczewski Gerrit-Reviewer: Saggi Mizrahi Gerrit-Reviewer: Shahar Havivi Gerrit-Reviewer: automat...@ovirt.org Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: utils: add CommandStream class
Nir Soffer has posted comments on this change. Change subject: utils: add CommandStream class .. Patch Set 8: (2 comments) https://gerrit.ovirt.org/#/c/33909/8/tests/utilsTests.py File tests/utilsTests.py: Line 777: Line 778: c = self._startCommand(cmd) Line 779: p = utils.CommandStream(c, Line 780: recv_data if recv_out else self.assertUnexpectedCall, Line 781: recv_data if recv_err else self.assertUnexpectedCall) pep8 does not like the indentation here: tests/utilsTests.py:780:13: E128 continuation line under-indented for visual indent tests/utilsTests.py:781:13: E128 continuation line under-indented for visual indent Line 782: Line 783: while not p.closed: Line 784: p.receive() Line 785: Line 803: Line 804: c = self._startCommand(cmd) Line 805: p = utils.CommandStream(c, Line 806: recv_data if recv_out else self.assertUnexpectedCall, Line 807: recv_data if recv_err else self.assertUnexpectedCall) pep8 does not like the indentation here: tests/utilsTests.py:806:13: E128 continuation line under-indented for visual indent tests/utilsTests.py:807:13: E128 continuation line under-indented for visual indent Line 808: Line 809: c.stdin.write(text) Line 810: c.stdin.flush() Line 811: c.stdin.close() -- To view, visit https://gerrit.ovirt.org/33909 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ie015368bb9c5992e5c73a149277c59fc4ffbd570 Gerrit-PatchSet: 8 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Federico Simoncelli Gerrit-Reviewer: Adam Litke Gerrit-Reviewer: Ala Hino Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Freddy Rolland Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Piotr Kliczewski Gerrit-Reviewer: Saggi Mizrahi Gerrit-Reviewer: Shahar Havivi Gerrit-Reviewer: automat...@ovirt.org Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: utils: add CommandStream class
Nir Soffer has posted comments on this change. Change subject: utils: add CommandStream class .. Patch Set 8: Code-Review+1 -- To view, visit https://gerrit.ovirt.org/33909 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ie015368bb9c5992e5c73a149277c59fc4ffbd570 Gerrit-PatchSet: 8 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Federico Simoncelli Gerrit-Reviewer: Adam Litke Gerrit-Reviewer: Ala Hino Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Freddy Rolland Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Piotr Kliczewski Gerrit-Reviewer: Saggi Mizrahi Gerrit-Reviewer: Shahar Havivi Gerrit-Reviewer: automat...@ovirt.org Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: utils: add CommandStream class
oVirt Jenkins CI Server has posted comments on this change. Change subject: utils: add CommandStream class .. Patch Set 8: Code-Review-1 Verified-1 Build Unstable http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/18440/ : UNSTABLE http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/1671/ : SUCCESS -- To view, visit https://gerrit.ovirt.org/33909 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ie015368bb9c5992e5c73a149277c59fc4ffbd570 Gerrit-PatchSet: 8 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Federico Simoncelli Gerrit-Reviewer: Adam Litke Gerrit-Reviewer: Ala Hino Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Freddy Rolland Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Piotr Kliczewski Gerrit-Reviewer: Saggi Mizrahi Gerrit-Reviewer: Shahar Havivi Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: utils: add CommandStream class
oVirt Jenkins CI Server has posted comments on this change. Change subject: utils: add CommandStream class .. Patch Set 8: Build Started (2/2) -> http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/1671/ -- To view, visit https://gerrit.ovirt.org/33909 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ie015368bb9c5992e5c73a149277c59fc4ffbd570 Gerrit-PatchSet: 8 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Federico Simoncelli Gerrit-Reviewer: Adam Litke Gerrit-Reviewer: Ala Hino Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Freddy Rolland Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Piotr Kliczewski Gerrit-Reviewer: Saggi Mizrahi Gerrit-Reviewer: Shahar Havivi Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: utils: add CommandStream class
automat...@ovirt.org has posted comments on this change. Change subject: utils: add CommandStream class .. Patch Set 8: * Update tracker::IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3']) -- To view, visit https://gerrit.ovirt.org/33909 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ie015368bb9c5992e5c73a149277c59fc4ffbd570 Gerrit-PatchSet: 8 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Federico Simoncelli Gerrit-Reviewer: Adam Litke Gerrit-Reviewer: Ala Hino Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Freddy Rolland Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Piotr Kliczewski Gerrit-Reviewer: Saggi Mizrahi Gerrit-Reviewer: Shahar Havivi Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: utils: add CommandStream class
oVirt Jenkins CI Server has posted comments on this change. Change subject: utils: add CommandStream class .. Patch Set 8: Build Started (1/2) -> http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/18440/ -- To view, visit https://gerrit.ovirt.org/33909 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ie015368bb9c5992e5c73a149277c59fc4ffbd570 Gerrit-PatchSet: 8 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Federico Simoncelli Gerrit-Reviewer: Adam Litke Gerrit-Reviewer: Ala Hino Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Freddy Rolland Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Piotr Kliczewski Gerrit-Reviewer: Saggi Mizrahi Gerrit-Reviewer: Shahar Havivi Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: utils: add CommandStream class
Nir Soffer has posted comments on this change.
Change subject: utils: add CommandStream class
..
Patch Set 7:
(4 comments)
https://gerrit.ovirt.org/#/c/33909/7/tests/utilsTests.py
File tests/utilsTests.py:
Line 754: @expandPermutations
Line 755: class CommandStreamTests(TestCaseBase):
Line 756:
Line 757: @contextlib.contextmanager
Line 758: def assertElapsed(self, expected, tolerance=0.5):
This seems like a general utility that should be in testlib.
Line 759: start = utils.monotonic_time()
Line 760:
Line 761: yield
Line 762:
Line 773:
Line 774: @permutations((('stdout',), ('stderr',)))
Line 775: def test_receive(self, output):
Line 776: text = bytes("Hello World")
Line 777: received = bytearray()
We can use
received = [bytearray()]
or
self.received = bytearray()
Line 778:
Line 779: def recv_stdout(buffer):
Line 780: # cannot use received += buffer with a variable
Line 781: # defined in the parent function.
Line 775: def test_receive(self, output):
Line 776: text = bytes("Hello World")
Line 777: received = bytearray()
Line 778:
Line 779: def recv_stdout(buffer):
This should be now recv_data - can come from stderr in the stderr permutation.
Line 780: # cannot use received += buffer with a variable
Line 781: # defined in the parent function.
Line 782: operator.iadd(received, buffer)
Line 783:
Line 778:
Line 779: def recv_stdout(buffer):
Line 780: # cannot use received += buffer with a variable
Line 781: # defined in the parent function.
Line 782: operator.iadd(received, buffer)
But we can use received[0] += buffer, or self.received += bufffer
Line 783:
Line 784: if output == 'stdout':
Line 785: c = self._startCommand(["echo", "-n", text])
Line 786: p = utils.CommandStream(c, recv_stdout,
self.assertUnexpectedCall)
--
To view, visit https://gerrit.ovirt.org/33909
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie015368bb9c5992e5c73a149277c59fc4ffbd570
Gerrit-PatchSet: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli
Gerrit-Reviewer: Adam Litke
Gerrit-Reviewer: Dan Kenigsberg
Gerrit-Reviewer: Federico Simoncelli
Gerrit-Reviewer: Francesco Romani
Gerrit-Reviewer: Nir Soffer
Gerrit-Reviewer: Piotr Kliczewski
Gerrit-Reviewer: Saggi Mizrahi
Gerrit-Reviewer: Shahar Havivi
Gerrit-Reviewer: automat...@ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: utils: add CommandStream class
Nir Soffer has posted comments on this change.
Change subject: utils: add CommandStream class
..
Patch Set 7:
(2 comments)
https://gerrit.ovirt.org/#/c/33909/7/tests/utilsTests.py
File tests/utilsTests.py:
Line 770:
Line 771: def _startCommand(self, command):
Line 772: return cpopen.CPopen(command)
Line 773:
Line 774: @permutations((('stdout',), ('stderr',)))
Lets put the command and cb spec here:
@permutations([
# command, recv_out, recv_err
(["echo", "-n", "%s"], True, False),
(["sh", "-c", 'echo -n "%s" >&2'], False, True),
])
def test_receive(self, cmd, recv_out, recv_err):
So the code is more generic, and we don't need error handling for unsupported
output:
cmd[-1] = cmd[-1] % text
c = self._startCommand(cmd)
p = utils.CommandStream(c,
recv_data if recv_out else self.assertUnexpectedCall,
recv_data if recv_err else self.assertUnexpectedCall)
And later we can easily add more tests, for example, receiving multiple lines,
large amount of data, no data, without changing the code.
Line 775: def test_receive(self, output):
Line 776: text = bytes("Hello World")
Line 777: received = bytearray()
Line 778:
Line 797:
Line 798: self.assertEqual(retcode, 0)
Line 799: self.assertEqual(text, received)
Line 800:
Line 801: @permutations((('stdout',), ('stderr',)))
Same as for test_receive
Line 802: def test_write(self, output):
Line 803: text = "Hello World"
Line 804: received = bytearray()
Line 805:
--
To view, visit https://gerrit.ovirt.org/33909
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie015368bb9c5992e5c73a149277c59fc4ffbd570
Gerrit-PatchSet: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli
Gerrit-Reviewer: Adam Litke
Gerrit-Reviewer: Dan Kenigsberg
Gerrit-Reviewer: Federico Simoncelli
Gerrit-Reviewer: Francesco Romani
Gerrit-Reviewer: Nir Soffer
Gerrit-Reviewer: Piotr Kliczewski
Gerrit-Reviewer: Saggi Mizrahi
Gerrit-Reviewer: Shahar Havivi
Gerrit-Reviewer: automat...@ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: utils: add CommandStream class
oVirt Jenkins CI Server has posted comments on this change. Change subject: utils: add CommandStream class .. Patch Set 7: Build Successful http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/18265/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/1495/ : 0 -- To view, visit https://gerrit.ovirt.org/33909 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ie015368bb9c5992e5c73a149277c59fc4ffbd570 Gerrit-PatchSet: 7 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Federico Simoncelli Gerrit-Reviewer: Adam Litke Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Piotr Kliczewski Gerrit-Reviewer: Saggi Mizrahi Gerrit-Reviewer: Shahar Havivi Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: utils: add CommandStream class
oVirt Jenkins CI Server has posted comments on this change. Change subject: utils: add CommandStream class .. Patch Set 7: Build Started (2/2) 0 -> http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/1495/ -- To view, visit https://gerrit.ovirt.org/33909 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ie015368bb9c5992e5c73a149277c59fc4ffbd570 Gerrit-PatchSet: 7 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Federico Simoncelli Gerrit-Reviewer: Adam Litke Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Piotr Kliczewski Gerrit-Reviewer: Saggi Mizrahi Gerrit-Reviewer: Shahar Havivi Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: utils: add CommandStream class
oVirt Jenkins CI Server has posted comments on this change. Change subject: utils: add CommandStream class .. Patch Set 7: Build Started (1/2) -> http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/18265/ -- To view, visit https://gerrit.ovirt.org/33909 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ie015368bb9c5992e5c73a149277c59fc4ffbd570 Gerrit-PatchSet: 7 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Federico Simoncelli Gerrit-Reviewer: Adam Litke Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Piotr Kliczewski Gerrit-Reviewer: Saggi Mizrahi Gerrit-Reviewer: Shahar Havivi Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: utils: add CommandStream class
automat...@ovirt.org has posted comments on this change. Change subject: utils: add CommandStream class .. Patch Set 7: * Update tracker::IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3']) -- To view, visit https://gerrit.ovirt.org/33909 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ie015368bb9c5992e5c73a149277c59fc4ffbd570 Gerrit-PatchSet: 7 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Federico Simoncelli Gerrit-Reviewer: Adam Litke Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Piotr Kliczewski Gerrit-Reviewer: Saggi Mizrahi Gerrit-Reviewer: Shahar Havivi Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: utils: add CommandStream class
oVirt Jenkins CI Server has posted comments on this change. Change subject: utils: add CommandStream class .. Patch Set 6: Build Successful http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/16903/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/17075/ : SUCCESS -- To view, visit https://gerrit.ovirt.org/33909 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ie015368bb9c5992e5c73a149277c59fc4ffbd570 Gerrit-PatchSet: 6 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Federico Simoncelli Gerrit-Reviewer: Adam Litke Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Piotr Kliczewski Gerrit-Reviewer: Saggi Mizrahi Gerrit-Reviewer: Shahar Havivi Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: utils: add CommandStream class
oVirt Jenkins CI Server has posted comments on this change. Change subject: utils: add CommandStream class .. Patch Set 6: Build Started (2/2) -> http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/17075/ -- To view, visit https://gerrit.ovirt.org/33909 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ie015368bb9c5992e5c73a149277c59fc4ffbd570 Gerrit-PatchSet: 6 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Federico Simoncelli Gerrit-Reviewer: Adam Litke Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Piotr Kliczewski Gerrit-Reviewer: Saggi Mizrahi Gerrit-Reviewer: Shahar Havivi Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: utils: add CommandStream class
oVirt Jenkins CI Server has posted comments on this change. Change subject: utils: add CommandStream class .. Patch Set 6: Build Started (1/2) -> http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/16903/ -- To view, visit https://gerrit.ovirt.org/33909 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ie015368bb9c5992e5c73a149277c59fc4ffbd570 Gerrit-PatchSet: 6 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Federico Simoncelli Gerrit-Reviewer: Adam Litke Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Piotr Kliczewski Gerrit-Reviewer: Saggi Mizrahi Gerrit-Reviewer: Shahar Havivi Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: utils: add CommandStream class
automat...@ovirt.org has posted comments on this change. Change subject: utils: add CommandStream class .. Patch Set 6: * Update tracker::IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3']) -- To view, visit https://gerrit.ovirt.org/33909 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ie015368bb9c5992e5c73a149277c59fc4ffbd570 Gerrit-PatchSet: 6 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Federico Simoncelli Gerrit-Reviewer: Adam Litke Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Piotr Kliczewski Gerrit-Reviewer: Saggi Mizrahi Gerrit-Reviewer: Shahar Havivi Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: utils: add CommandStream class
Nir Soffer has posted comments on this change. Change subject: utils: add CommandStream class .. Patch Set 5: Would you like to check my comments on the tests in version 3? https://gerrit.ovirt.org/#/c/33909/3..5/tests/utilsTests.py,unified -- To view, visit https://gerrit.ovirt.org/33909 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ie015368bb9c5992e5c73a149277c59fc4ffbd570 Gerrit-PatchSet: 5 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Federico Simoncelli Gerrit-Reviewer: Adam Litke Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Piotr Kliczewski Gerrit-Reviewer: Saggi Mizrahi Gerrit-Reviewer: Shahar Havivi Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: utils: add CommandStream class
Nir Soffer has posted comments on this change. Change subject: utils: add CommandStream class .. Patch Set 4: (1 comment) https://gerrit.ovirt.org/#/c/33909/4/lib/vdsm/utils.py File lib/vdsm/utils.py: Line 362: for fd in self._iocb: Line 363: self._poll.register(fd, select.EPOLLIN) Line 364: Line 365: def _poll_input(self, fileno): Line 366: self._iocb[fileno](os.read(fileno, io.DEFAULT_BUFFER_SIZE)) > If you have signal handlers that potentially take a long time to execute yo It seems that fd spuriously reported as ready is an issue only with select and no such issue is documented for poll or epoll, so this usage should be safe. Since we don't need currently to stop watching a running command, it is good enough to terminate the command. Line 367: Line 368: def _poll_event(self, fileno): Line 369: self._poll.unregister(fileno) Line 370: del self._iocb[fileno] -- To view, visit https://gerrit.ovirt.org/33909 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ie015368bb9c5992e5c73a149277c59fc4ffbd570 Gerrit-PatchSet: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Federico Simoncelli Gerrit-Reviewer: Adam Litke Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Piotr Kliczewski Gerrit-Reviewer: Saggi Mizrahi Gerrit-Reviewer: Shahar Havivi Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: utils: add CommandStream class
oVirt Jenkins CI Server has posted comments on this change. Change subject: utils: add CommandStream class .. Patch Set 5: Build Successful http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/16807/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/16979/ : SUCCESS -- To view, visit https://gerrit.ovirt.org/33909 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ie015368bb9c5992e5c73a149277c59fc4ffbd570 Gerrit-PatchSet: 5 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Federico Simoncelli Gerrit-Reviewer: Adam Litke Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Saggi Mizrahi Gerrit-Reviewer: Shahar Havivi Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: utils: add CommandStream class
automat...@ovirt.org has posted comments on this change. Change subject: utils: add CommandStream class .. Patch Set 5: * Update tracker::IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3']) -- To view, visit https://gerrit.ovirt.org/33909 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ie015368bb9c5992e5c73a149277c59fc4ffbd570 Gerrit-PatchSet: 5 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Federico Simoncelli Gerrit-Reviewer: Adam Litke Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Saggi Mizrahi Gerrit-Reviewer: Shahar Havivi Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: utils: add CommandStream class
Federico Simoncelli has posted comments on this change. Change subject: utils: add CommandStream class .. Patch Set 4: (1 comment) https://gerrit.ovirt.org/#/c/33909/4/lib/vdsm/utils.py File lib/vdsm/utils.py: Line 362: for fd in self._iocb: Line 363: self._poll.register(fd, select.EPOLLIN) Line 364: Line 365: def _poll_input(self, fileno): Line 366: self._iocb[fileno](os.read(fileno, io.DEFAULT_BUFFER_SIZE)) > We don't set the descriptors to non-blocking mode, so this may block when h If you have signal handlers that potentially take a long time to execute you have much more serious problems than CommandStream. I don't plan to make this non-blocking out of the box (the consumer can do that on his own since CommandStream is fully composable with a Popen object). Anyway it is obviously useful to document that receive may raise the same exceptions of read. At the moment the only interesting use-case is to stop the command (and not stop listening for events) which is accomplished by the Popen method terminate/kill. That will cause the process to exit and the file-descriptors to be closed. Line 367: Line 368: def _poll_event(self, fileno): Line 369: self._poll.unregister(fileno) Line 370: del self._iocb[fileno] -- To view, visit https://gerrit.ovirt.org/33909 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ie015368bb9c5992e5c73a149277c59fc4ffbd570 Gerrit-PatchSet: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Federico Simoncelli Gerrit-Reviewer: Adam Litke Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Saggi Mizrahi Gerrit-Reviewer: Shahar Havivi Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: utils: add CommandStream class
automat...@ovirt.org has posted comments on this change. Change subject: utils: add CommandStream class .. Patch Set 4: * Update tracker::IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3']) -- 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: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Federico Simoncelli Gerrit-Reviewer: Adam Litke Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Saggi Mizrahi Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: utils: add CommandStream class
Nir Soffer has posted comments on this change.
Change subject: utils: add CommandStream class
..
Patch Set 4:
(2 comments)
http://gerrit.ovirt.org/#/c/33909/4/lib/vdsm/utils.py
File lib/vdsm/utils.py:
Line 356: # entries in the dictionary)
Line 357: self._iocb = {
Line 358: self._command.stderr.fileno(): stderrcb,
Line 359: self._command.stdout.fileno(): stdoutcb,
Line 360: }
This practically works, but I'm not sure the behavior is defined. It would be
better to have a method to add a descriptor and invoke it in a well defined
order, with the comment explaining the correct order.
Line 361:
Line 362: for fd in self._iocb:
Line 363: self._poll.register(fd, select.EPOLLIN)
Line 364:
Line 362: for fd in self._iocb:
Line 363: self._poll.register(fd, select.EPOLLIN)
Line 364:
Line 365: def _poll_input(self, fileno):
Line 366: self._iocb[fileno](os.read(fileno, io.DEFAULT_BUFFER_SIZE))
We don't set the descriptors to non-blocking mode, so this may block when
handling an event.
We should set the descriptors to non-blocking when command is given, or maybe
dup them and set the dup as non-blocking. Then we should handle EAGAIN here.
Another issue is handling errors - if os.read raises, the caller should get the
exception and handle it. We can have an errorcb called when os.read() fails
with the fd and the exception.
Or if we choose to let the caller of receive handle such errors, we should
document the expected errors that received may raise.
Finally, if the caller wants to stop listening for events - how can you "close"
this stream?
Line 367:
Line 368: def _poll_event(self, fileno):
Line 369: self._poll.unregister(fileno)
Line 370: del self._iocb[fileno]
--
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: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli
Gerrit-Reviewer: Adam Litke
Gerrit-Reviewer: Dan Kenigsberg
Gerrit-Reviewer: Federico Simoncelli
Gerrit-Reviewer: Francesco Romani
Gerrit-Reviewer: Nir Soffer
Gerrit-Reviewer: Saggi Mizrahi
Gerrit-Reviewer: automat...@ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: utils: add CommandStream class
Nir Soffer has posted comments on this change. Change subject: utils: add CommandStream class .. Patch Set 3: (2 comments) http://gerrit.ovirt.org/#/c/33909/3/lib/vdsm/utils.py File lib/vdsm/utils.py: 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): > This method is handling the poll input event, the name seems consistent. These names are consistent, but they are not good method names. They do not describe what the method does, and do not help readability. The method that does polling is not the same thing as the method that handle certain event, so their names should not be "consistent". Having epoll in the name adds no value, because we don't have several method of polling, one with epoll and one without another type. It would be better if we can replace epoll with poll without modifying these names. I would expect to have names like: - _poll - wait for events. This is typically done with a timeout, and we don't have another method for polling without a timeouts, so the _timeout suffix is not needed. - _handle_input - handle input from a watched fd - _handle_close - handle closed fd 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 363: return len(self.iocb) == 0 Line 364: Line 365: def receive(self, timeout=None): Line 366: if timeout is None: Line 367: epoll_remaining = -1 > If we do it in the loop it will be set at every cycle unless we rely on som I'm not worried about setting this each iteration. Line 368: else: Line 369: endtime = monotonic_time() + timeout Line 370: Line 371: while not self.closed: -- 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: 3 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Federico Simoncelli Gerrit-Reviewer: Adam Litke Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Saggi Mizrahi Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: utils: add CommandStream class
oVirt Jenkins CI Server has posted comments on this change. Change subject: utils: add CommandStream class .. Patch Set 4: Build Failed http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/15357/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/15188/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/14400/ : FAILURE -- 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: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Federico Simoncelli Gerrit-Reviewer: Adam Litke Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Saggi Mizrahi Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: utils: add CommandStream class
Federico Simoncelli has posted comments on this change.
Change subject: utils: add CommandStream class
..
Patch Set 3:
(5 comments)
http://gerrit.ovirt.org/#/c/33909/3/lib/vdsm/utils.py
File lib/vdsm/utils.py:
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 fo
Yes. Currently everything is squashed to the stderrcb, I'd better invert them
and add a comment.
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.
This method is handling the poll input event, the name seems consistent.
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.
This method is handling the poll events, the name seems consistent.
Line 349: self.epoll.unregister(fileno)
Line 350: del self.iocb[fileno]
Line 351:
Line 352: def _epoll_timeout(self, timeout):
Line 348: def _epoll_event(self, fileno):
Line 349: self.epoll.unregister(fileno)
Line 350: del self.iocb[fileno]
Line 351:
Line 352: def _epoll_timeout(self, timeout):
> Rename to _poll?
This method is polling with timeout, the name seems consistent.
Line 353: fdevents = NoIntrPoll(self.epoll.poll, timeout)
Line 354:
Line 355: for fileno, event in fdevents:
Line 356: if event & select.EPOLLIN:
Line 363: return len(self.iocb) == 0
Line 364:
Line 365: def receive(self, timeout=None):
Line 366: if timeout is None:
Line 367: epoll_remaining = -1
> We can do this in the loop bellow instead.
If we do it in the loop it will be set at every cycle unless we rely on some
implicit python optimization.
Line 368: else:
Line 369: endtime = monotonic_time() + timeout
Line 370:
Line 371: while not self.closed:
--
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: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli
Gerrit-Reviewer: Adam Litke
Gerrit-Reviewer: Dan Kenigsberg
Gerrit-Reviewer: Federico Simoncelli
Gerrit-Reviewer: Francesco Romani
Gerrit-Reviewer: Nir Soffer
Gerrit-Reviewer: Saggi Mizrahi
Gerrit-Reviewer: automat...@ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: utils: add CommandStream class
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
Line 5: CommitDate: 2014-12-17 21:53:58 +
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
Change in vdsm[master]: utils: add CommandStream class
Nir Soffer has posted comments on this change.
Change subject: utils: add CommandStream class
..
Patch Set 3:
(7 comments)
http://gerrit.ovirt.org/#/c/33909/3/lib/vdsm/utils.py
File lib/vdsm/utils.py:
Line 331:
Line 332: class CommandStream(object):
Line 333: def __init__(self, command, stdoutcb, stderrcb):
Line 334: self._command = command
Line 335: self.epoll = select.epoll()
Should be private.
Line 336:
Line 337: self.iocb = {
Line 338: self._command.stdout.fileno(): stdoutcb,
Line 339: self._command.stderr.fileno(): stderrcb,
Line 333: def __init__(self, command, stdoutcb, stderrcb):
Line 334: self._command = command
Line 335: self.epoll = select.epoll()
Line 336:
Line 337: self.iocb = {
Same - private
Line 338: self._command.stdout.fileno(): stdoutcb,
Line 339: self._command.stderr.fileno(): stderrcb,
Line 340: }
Line 341:
Line 348: def _epoll_event(self, fileno):
Line 349: self.epoll.unregister(fileno)
Line 350: del self.iocb[fileno]
Line 351:
Line 352: def _epoll_timeout(self, timeout):
Rename to _poll?
Line 353: fdevents = NoIntrPoll(self.epoll.poll, timeout)
Line 354:
Line 355: for fileno, event in fdevents:
Line 356: if event & select.EPOLLIN:
Line 363: return len(self.iocb) == 0
Line 364:
Line 365: def receive(self, timeout=None):
Line 366: if timeout is None:
Line 367: epoll_remaining = -1
We can do this in the loop bellow instead.
Line 368: else:
Line 369: endtime = monotonic_time() + timeout
Line 370:
Line 371: while not self.closed:
Line 365: def receive(self, timeout=None):
Line 366: if timeout is None:
Line 367: epoll_remaining = -1
Line 368: else:
Line 369: endtime = monotonic_time() + timeout
I like to call this "deadline", in case you are not attached to "endtime".
Line 370:
Line 371: while not self.closed:
Line 372: if timeout is not None:
Line 373: epoll_remaining = endtime - monotonic_time()
Line 368: else:
Line 369: endtime = monotonic_time() + timeout
Line 370:
Line 371: while not self.closed:
Line 372: if timeout is not None:
It is little ugly to check every time for timeout is None.
Line 373: epoll_remaining = endtime - monotonic_time()
Line 374: if epoll_remaining <= 0:
Line 375: break
Line 376:
Line 371: while not self.closed:
Line 372: if timeout is not None:
Line 373: epoll_remaining = endtime - monotonic_time()
Line 374: if epoll_remaining <= 0:
Line 375: break
But since we do check, lets put the -1 here?
else:
epoll_remaining = -1
We can simplify more if we use some big default timeout:
def receive(self, timeout=sys.maxint):
now = monotonic_time()
deadline = now + timeout
while not self.closed and now < deadline:
self._poll(deadline - now)
now = monotonic_time()
Line 376:
Line 377: self._epoll_timeout(epoll_remaining)
Line 378:
Line 379:
--
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: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli
Gerrit-Reviewer: Adam Litke
Gerrit-Reviewer: Dan Kenigsberg
Gerrit-Reviewer: Federico Simoncelli
Gerrit-Reviewer: Francesco Romani
Gerrit-Reviewer: Nir Soffer
Gerrit-Reviewer: Saggi Mizrahi
Gerrit-Reviewer: automat...@ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: utils: add CommandStream class
oVirt Jenkins CI Server has posted comments on this change. Change subject: utils: add CommandStream class .. Patch Set 3: Build Failed http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/13568/ : FAILURE http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/14525/ : FAILURE http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/14357/ : SUCCESS -- 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: 3 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Federico Simoncelli Gerrit-Reviewer: Adam Litke Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Saggi Mizrahi Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: utils: add CommandStream class
Federico Simoncelli has posted comments on this change. Change subject: utils: add CommandStream class .. Patch Set 2: (2 comments) http://gerrit.ovirt.org/#/c/33909/2/lib/vdsm/utils.py File lib/vdsm/utils.py: 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 ? Done Line 348: self.epoll.register(fd, select.EPOLLIN) Line 349: Line 350: def terminate(self): Line 351: self.child.terminate() 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 > +1 Done Line 390: Line 391: while self.returncode is None: Line 392: if timeout is not None: Line 393: epoll_remaining = endtime - os.times()[4] -- 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 Gerrit-Reviewer: Adam Litke Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Saggi Mizrahi Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: utils: add CommandStream class
Nir Soffer has posted comments on this change.
Change subject: utils: add CommandStream class
..
Patch Set 2:
(12 comments)
I like the direction.
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):
The name is not very good, as this does not have a stream like interface. Maybe
just "Command"?
And we should move this out of utils to the commands module.
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
+1 - if not everyone will start to access these.
Do we need to keep self.command?
Line 335: self.returncode = None
Line 336:
Line 337: self.child = CPopen(self.command, cwd=cwd, close_fds=True,
Line 338: deathSignal=deathSignal)
Line 331: class CommandStream(object):
Line 332: def __init__(self, command, stdoutcb, stderrcb, cwd=None,
Line 333: deathSignal=0):
Line 334: self.command = command
Line 335: self.returncode = None
Why do we need self.returncode? we can have a property returning
self.child.returncode.
Line 336:
Line 337: self.child = CPopen(self.command, cwd=cwd, close_fds=True,
Line 338: deathSignal=deathSignal)
Line 339:
Line 336:
Line 337: self.child = CPopen(self.command, cwd=cwd, close_fds=True,
Line 338: deathSignal=deathSignal)
Line 339:
Line 340: self.epoll = select.epoll()
I don't see a need for epoll. We are polling on 2 file descriptors, why do we
need epoll for that?
Line 341:
Line 342: self.iocb = {
Line 343: self.child.stdout.fileno(): stdoutcb,
Line 344: self.child.stderr.fileno(): stderrcb,
Line 359: def flush(self):
Line 360: self.child.stdin.flush()
Line 361:
Line 362: def close(self):
Line 363: self.child.stdin.close()
Both flush() and close() are not clear for this class. Accessing stdin directly
is better.
Line 364:
Line 365: def _epoll_input(self, fileno):
Line 366: self.iocb[fileno](os.read(fileno, io.DEFAULT_BUFFER_SIZE))
Line 367:
Line 361:
Line 362: def close(self):
Line 363: self.child.stdin.close()
Line 364:
Line 365: def _epoll_input(self, fileno):
I don't understand this name - maybe _call_iocb()?
Line 366: self.iocb[fileno](os.read(fileno, io.DEFAULT_BUFFER_SIZE))
Line 367:
Line 368: def _epoll_event(self, fileno):
Line 369: self.epoll.unregister(fileno)
Line 364:
Line 365: def _epoll_input(self, fileno):
Line 366: self.iocb[fileno](os.read(fileno, io.DEFAULT_BUFFER_SIZE))
Line 367:
Line 368: def _epoll_event(self, fileno):
This name is also strange - maybe _unregister_iocb()?
Line 369: self.epoll.unregister(fileno)
Line 370: del self.iocb[fileno]
Line 371:
Line 372: def _epoll_timeout(self, timeout):
Line 368: def _epoll_event(self, fileno):
Line 369: self.epoll.unregister(fileno)
Line 370: del self.iocb[fileno]
Line 371:
Line 372: def _epoll_timeout(self, timeout):
This should be something like _wait_for_io()
Line 373: fdevents = NoIntrPoll(self.epoll.poll, timeout)
Line 374:
Line 375: for fileno, event in fdevents:
Line 376: if event & select.EPOLLIN:
Line 379: self._epoll_event(fileno)
Line 380: # Trying to collect the child status in case the
Line 381: # file descriptor was closed because the process
Line 382: # terminated.
Line 383: self.returncode = self.child.poll()
We don't need to keep the return code, as the poll() update the
child.returncode.
Line 384:
Line 385: def wait(self, timeout=None):
Line 386: if timeout is None:
Line 387: epoll_remaining = -1
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
+1
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 387: epoll_remaining = -1
Line 388: else:
Line 389: endtime = os.times()[4] + tim
Change in vdsm[master]: utils: add CommandStream class
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 Gerrit-Reviewer: Adam Litke Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Fe
Change in vdsm[master]: utils: add CommandStream class
Francesco Romani has posted comments on this change. Change subject: utils: add CommandStream class .. Patch Set 2: (1 comment) http://gerrit.ovirt.org/#/c/33909/2//COMMIT_MSG Commit Message: Line 4: Commit: Federico Simoncelli Line 5: CommitDate: 2014-11-13 12:37:07 + Line 6: Line 7: utils: add CommandStream class Line 8: > Still a draft. Commit message to come. Waiting for it because I'd like to know more about the rationale and the benefits of this change. Line 9: Change-Id: Ie015368bb9c5992e5c73a149277c59fc4ffbd570 -- 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 Gerrit-Reviewer: Adam Litke Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Saggi Mizrahi Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: utils: add CommandStream class
oVirt Jenkins CI Server has posted comments on this change. Change subject: utils: add CommandStream class .. Patch Set 2: Build Failed http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/13388/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/12598/ : FAILURE http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created_staging/164/ : FAILURE http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/13550/ : FAILURE -- 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 Gerrit-Reviewer: Adam Litke Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Saggi Mizrahi Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: utils: add CommandStream class
Federico Simoncelli has posted comments on this change. Change subject: utils: add CommandStream class .. Patch Set 2: (1 comment) http://gerrit.ovirt.org/#/c/33909/2//COMMIT_MSG Commit Message: Line 4: Commit: Federico Simoncelli Line 5: CommitDate: 2014-11-13 12:37:07 + Line 6: Line 7: utils: add CommandStream class Line 8: Still a draft. Commit message to come. Line 9: Change-Id: Ie015368bb9c5992e5c73a149277c59fc4ffbd570 -- 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 Gerrit-Reviewer: Adam Litke Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: utils: add CommandStream class
oVirt Jenkins CI Server has posted comments on this change. Change subject: utils: add CommandStream class .. Patch Set 1: Build Failed http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/12827/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/11879/ : FAILURE http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/12670/ : SUCCESS -- 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: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Federico Simoncelli Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: utils: add CommandStream class
Federico Simoncelli has posted comments on this change. Change subject: utils: add CommandStream class .. Patch Set 1: (1 comment) http://gerrit.ovirt.org/#/c/33909/1/tests/utilsTests.py File tests/utilsTests.py: Line 736: Line 737: with self.assertElapsed(2): Line 738: retcode = p.wait() Line 739: Line 740: self.assertEqual(retcode, 0) Add test_early_close_timeout -- 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: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Federico Simoncelli Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: automat...@ovirt.org Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: utils: add CommandStream class
Federico Simoncelli has uploaded a new change for review.
Change subject: utils: add CommandStream class
..
utils: add CommandStream class
Change-Id: Ie015368bb9c5992e5c73a149277c59fc4ffbd570
Signed-off-by: Federico Simoncelli
---
M lib/vdsm/utils.py
M tests/utilsTests.py
2 files changed, 188 insertions(+), 0 deletions(-)
git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/09/33909/1
diff --git a/lib/vdsm/utils.py b/lib/vdsm/utils.py
index 244f0e2..4b0ccb7 100644
--- a/lib/vdsm/utils.py
+++ b/lib/vdsm/utils.py
@@ -313,6 +313,90 @@
timeout = max(0, endtime - time.time())
+class CommandStream(object):
+def __init__(self, command, stdoutcb, stderrcb, cwd=None,
+ deathSignal=0):
+self.command = command
+self.returncode = None
+
+self.child = CPopen(self.command, cwd=cwd, close_fds=True,
+deathSignal=deathSignal)
+
+self.epoll = select.epoll()
+
+self.iocb = {
+self.child.stdout.fileno(): stdoutcb,
+self.child.stderr.fileno(): stderrcb,
+}
+
+for fd in self.iocb.keys():
+self.epoll.register(fd, select.EPOLLIN)
+
+def terminate(self):
+self.child.terminate()
+
+def kill(self):
+self.child.kill()
+
+def write(self, data):
+self.child.stdin.write(data)
+
+def flush(self):
+self.child.stdin.flush()
+
+def close(self):
+self.child.stdin.close()
+
+def _epoll_input(self, fileno):
+self.iocb[fileno](os.read(fileno, io.DEFAULT_BUFFER_SIZE))
+
+def _epoll_event(self, fileno):
+self.epoll.unregister(fileno)
+del self.iocb[fileno]
+
+def _epoll_timeout(self, timeout):
+fdevents = NoIntrPoll(self.epoll.poll, timeout)
+
+for fileno, event in fdevents:
+if event & select.EPOLLIN:
+self._epoll_input(fileno)
+elif event & (select.EPOLLHUP | select.EPOLLERR):
+self._epoll_event(fileno)
+# Trying to collect the child status in case the
+# file descriptor was closed because the process
+# terminated.
+self.returncode = self.child.poll()
+
+def wait(self, timeout=None):
+if timeout is None:
+epoll_remaining = -1
+else:
+endtime = os.times()[4] + timeout
+
+while self.returncode is None:
+if timeout is not None:
+epoll_remaining = endtime - os.times()[4]
+if epoll_remaining <= 0:
+break
+
+if len(self.iocb):
+self._epoll_timeout(epoll_remaining)
+else:
+# This is a busy-loop taken from issue5673, and
+# python 3.4 still uses this implementation.
+# A smarter solution would be using signalfd or
+# sigtimedwait but they don't seem to mix well
+# with multithreading (especially under heavy
+# load: tens of children from tens of threads).
+# Anyway we reach this only when both stdout and
+# stderr are closed, which means that in most of
+# the cases the child is about to die.
+time.sleep(0.0005)
+self.returncode = self.child.poll()
+
+return self.returncode
+
+
class AsyncProc(object):
"""
AsyncProc is a funky class. It wraps a standard subprocess.Popen
diff --git a/tests/utilsTests.py b/tests/utilsTests.py
index 6e66c02..c279f04 100644
--- a/tests/utilsTests.py
+++ b/tests/utilsTests.py
@@ -22,8 +22,12 @@
import contextlib
import errno
import logging
+import operator
+import signal
import sys
import threading
+
+from contextlib import contextmanager
from testlib import VdsmTestCase as TestCaseBase
from testlib import permutations, expandPermutations
@@ -634,3 +638,103 @@
def test_empty(self):
self.assertEquals(utils._list2cmdline([]), "")
+
+
+class CommandStreamTests(TestCaseBase):
+
+@contextmanager
+def assertElapsed(self, limit):
+start = os.times()[4]
+
+yield
+
+elapsed = os.times()[4] - start
+
+if elapsed < limit:
+raise AssertionError("Operation time: %s" % elapsed)
+
+def assertNoOutput(self, data):
+raise AssertionError("Unexpected data: " + repr(data))
+
+def test_output(self):
+text = "Hello World"
+received = bytearray()
+
+def recv_stdout(buffer):
+# cannot use received += buffer with a variable
+# defined in the parent function.
+operator.iadd(received, buffer)
+
+p = utils.CommandStream(["echo", "-n", text],
+recv_stdout,
+self.assertNoOutput)
+
+retcode = p.wait()
+
+self.ass
