Change in vdsm[master]: Implement Popen without forking back to python

xiawenc at linux.vnet.ibm.com xiawenc at linux.vnet.ibm.com
Wed May 9 09:19:45 UTC 2012


Wenchao Xia has posted comments on this change.

Change subject: Implement Popen without forking back to python
......................................................................


Patch Set 12: Verified; I would prefer that you didn't submit this

(4 inline comments)

Skill-full hacking!
Tested on following cases, no major issue found:
1 storage domain creating and deleting.
2 thread and popen race condition case.
  Just a few comments inside the patch.

....................................................
File vdsm/betterPopen/createprocess.c
Line 36:      "Execute a command command."},
on help(createProcess) I noticed "command command", maybe a mis-spell?

....................................................
File vdsm/betterPopen/__init__.py
Line 36:     def __init__(self, args, close_fds=False, cwd=None, env=None):
In my test I found BetterPopen refuse command = "cat", but subprocess.Popen accept it. Better to add a few words here.

Line 48:     def _execute_child(self, args, executable, preexec_fn, close_fds,
I think a check is needed to see whether Popen._init_ does other things before entering  _execute_child, such as watching the child process's fds or lifetime. Actually I am worried about the situation cpython adding some codes between the Popen_init_ and _execute_child and do a check after child process dies in the future, In this case I am afraid a full test are need for each new python version vdsm is going to run on.

....................................................
File vdsm/betterPopen/Makefile.am
Line 27: 
I guess this require building machine is the same with target machine, usually x86-64 to x86-64. But I think it is OK for most use cases.

--
To view, visit http://gerrit.ovirt.org/3944
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ief4ae42eb57551d7f5893bf562da451a9c98f219
Gerrit-PatchSet: 12
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Saggi Mizrahi <smizrahi at redhat.com>
Gerrit-Reviewer: Ayal Baron <abaron at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky at redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi at redhat.com>
Gerrit-Reviewer: Wenchao Xia <xiawenc at linux.vnet.ibm.com>


More information about the vdsm-patches mailing list