Change in vdsm[master]: Kill and report asyncProc if fails on first write to stdin

nsoffer at redhat.com nsoffer at redhat.com
Sun Mar 27 11:53:10 UTC 2016


Nir Soffer has posted comments on this change.

Change subject: Kill and report asyncProc if fails on first write to stdin
......................................................................


Patch Set 6:

(1 comment)

https://gerrit.ovirt.org/#/c/51763/6/tests/commands_test.py
File tests/commands_test.py:

Line 73:         self.assertEquals(int(out[0].split()[2]), 0)
Line 74: 
Line 75:     def test_exec_infected_script(self):
Line 76:         script = 'import sys; sys.stdin.close()'
Line 77:         self.assertRaises(commands.execCmd(['python', '-c', script]))
> I did something like :
We should be compatible with subprocess.Popen and subprocess32.Popen - are going to kill AsyncProc soon

If writing to stdin does not try to write to the underlying pipe, and does not raise when the child process close the pipe, this code is broken, and must be fixed.

Do we have code using sync=False and data != None? If we don't, lets stop supporting this - raise RuntimeError if sync=True and data != None.

When you call execCmd to create a process and want to read and write yourself, you are responsible for writing or reading, execCmd should not do anything. data is meaningful only when calling communicate(data) - it should fail correctly if data cannot be written.
Line 78: 
Line 79: 
Line 80: class ExecCmdStressTest(TestCaseBase):
Line 81: 


-- 
To view, visit https://gerrit.ovirt.org/51763
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I57b68742ce1dcae1d9fdad020b7b4a41ca6ad028
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yaniv Bronhaim <ybronhei at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczewski at gmail.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei at redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation at ovirt.org>
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list