Ayal Baron has posted comments on this change.
Change subject: Implement Popen without forking back to python
......................................................................
Patch Set 1: I would prefer that you didn't submit this
(11 inline comments)
....................................................
Commit Message
Line 10: Forking a python proc is a very copmlex and voletile process and we
copmlex/complex
voletile/volatile
Line 13: This is a simpler method of execing that doesn't go back to python after
Why does python's implementation fork back to python?
i.e. what are we giving up in order to get a simpler implementation? (besides cross
platform support)
....................................................
File vdsm/storage/betterPopen/createprocess.c
Line 23: return;
what's the difference in behaviour whether it is NULL or not? you return in any event?
(if so, why bother with the 'if'?)
Line 84: argsize = sizeof(char*) * (argn + 1);
why argn + 1? your 'for' loop runs from 0 to argn-1? (i.e. argn slots needed)
Line 90: memset(argv, 0, argsize);
why not just use calloc above?
Line 100: if (PyList_Check(pyEnvList)) {
this function is too long as it is, why not move this part out to a different function and
call it once for pyArgList and once for pyEnvList?
(just move the arg len validation out)
Line 102: argsize = sizeof(char*) * (argn + 1);
same as above (argn+1, calloc)
Line 119: /* This will faild if the hosting process is out of open fds */
s/faild/fail/
Line 167: /* I is there to stop the compile warnings */
s/I/'i'/
Line 186: /* From this point errors shouldn't occure, if they do something is
very
s/occure/occur/
Line 198: return Py_BuildValue("(iiii)", cpid, outfd[1], in1fd[0],
in2fd[0]);
how about an empty line here?
--
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: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Saggi Mizrahi <smizrahi(a)redhat.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>