9:10 p.m.
Saggi Mizrahi has uploaded a new change for review.
Change subject: Implement Popen without forking back to python
......................................................................
Implement Popen without forking back to python
Python's implementation of Popen forks back to python before execing.
Forking a python proc is a very copmlex and voletile process and we
already have hacks to try and avoid some of the bugs involved with it.
This is a simpler method of execing that doesn't go back to python after
forking. This allows for faster safer exec.
Should solve bugs involving with GIL\GC\Logging deadlocks related to the
python implementation.
Change-Id: Ief4ae42eb57551d7f5893bf562da451a9c98f219
Signed-off-by: Saggi Mizrahi <smizrahi(a)redhat.com>
---
M configure.ac
M tests/Makefile.am
A tests/betterPopenTests.py
M vdsm.spec.in
M vdsm/storage/Makefile.am
A vdsm/storage/betterPopen/.gitignore
A vdsm/storage/betterPopen/Makefile.am
A vdsm/storage/betterPopen/__init__.py
A vdsm/storage/betterPopen/createprocess.c
A vdsm/storage/betterPopen/setup.py
M vdsm/storage/misc.py
11 files changed, 425 insertions(+), 5 deletions(-)
git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/44/3944/1
--
To view, visit http://gerrit.ovirt.org/3944
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ief4ae42eb57551d7f5893bf562da451a9c98f219
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Saggi Mizrahi <smizrahi(a)redhat.com>
6:11 a.m.
Federico Simoncelli has posted comments on this change.
Change subject: Implement Popen without forking back to python
......................................................................
Patch Set 14: (1 inline comment)
....................................................
File vdsm.spec.in
Line 616: %{python_sitelib}/%{vdsm_name}/betterPopen/createprocess.so
The dynamic libs aren't python_sitelib material. They belong to the python_sitearch
path.
--
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: 14
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Saggi Mizrahi <smizrahi(a)redhat.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com>
Gerrit-Reviewer: Wenchao Xia <xiawenc(a)linux.vnet.ibm.com>
4:19 a.m.
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(a)redhat.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com>
Gerrit-Reviewer: Wenchao Xia <xiawenc(a)linux.vnet.ibm.com>
3:10 p.m.
Ayal Baron has posted comments on this change.
Change subject: Implement Popen without forking back to python
......................................................................
Patch Set 12: Looks good to me, but someone else must approve
--
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(a)redhat.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com>
4:16 p.m.
Dan Kenigsberg has posted comments on this change.
Change subject: Implement Popen without forking back to python
......................................................................
Patch Set 12: Looks good to me, but someone else must approve
be a sport a YOU tell folks about the new dependency for testing vdsm, please.
--
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(a)redhat.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com>
3:50 p.m.
Saggi Mizrahi has posted comments on this change.
Change subject: Implement Popen without forking back to python
......................................................................
Patch Set 12: (1 inline comment)
....................................................
Commit Message
Line 20: as the extension needs to be compiled to be tested.
Send an announcement to vdsm-devel when you push it in.
--
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(a)redhat.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com>
1:44 p.m.
Dan Kenigsberg has posted comments on this change.
Change subject: Implement Popen without forking back to python
......................................................................
Patch Set 11: I would prefer that you didn't submit this
(1 inline comment)
please warn (commit message, vdsm-devel) that you'd now need python-devel and gcc just
to test vdsm. And detail your reasons, some people are bound to be annoyed by this.
....................................................
File tests/betterPopenTests.py
Line 28: print p.stderr.read()
intentional? what's the intention?
--
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: 11
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Saggi Mizrahi <smizrahi(a)redhat.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com>
6:13 a.m.
Dan Kenigsberg has submitted this change and it was merged.
Change subject: Implement Popen without forking back to python
......................................................................
Implement Popen without forking back to python
Python's implementation of Popen forks back to python before execing.
Forking a python proc is a very complex and volatile process and we
already have hacks to try and avoid some of the bugs involved with it.
This is a simpler method of execing that doesn't go back to python after
forking. This allows for faster safer exec.
Should solve bugs involving with GIL\GC\Logging deadlocks related to the
python implementation.
This also makes means that python-devel and gcc are needed for testing
as the extension needs to be compiled to be tested.
Change-Id: Ief4ae42eb57551d7f5893bf562da451a9c98f219
Signed-off-by: Saggi Mizrahi <smizrahi(a)redhat.com>
---
M configure.ac
M tests/Makefile.am
A tests/betterPopenTests.py
M tests/testrunner.py
M vdsm.spec.in
M vdsm/Makefile.am
A vdsm/betterPopen/.gitignore
A vdsm/betterPopen/Makefile.am
A vdsm/betterPopen/__init__.py
A vdsm/betterPopen/createprocess.c
A vdsm/betterPopen/setup.py
M vdsm/storage/misc.py
12 files changed, 479 insertions(+), 8 deletions(-)
Approvals:
Ayal Baron: Looks good to me, but someone else must approve
Wenchao Xia: Verified; Looks good to me, but someone else must approve
Dan Kenigsberg: Looks good to me, approved
--
To view, visit http://gerrit.ovirt.org/3944
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: Ief4ae42eb57551d7f5893bf562da451a9c98f219
Gerrit-PatchSet: 14
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Saggi Mizrahi <smizrahi(a)redhat.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com>
Gerrit-Reviewer: Wenchao Xia <xiawenc(a)linux.vnet.ibm.com>
6:13 a.m.
Dan Kenigsberg has posted comments on this change.
Change subject: Implement Popen without forking back to python
......................................................................
Patch Set 14: Looks good to me, approved
--
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: 14
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Saggi Mizrahi <smizrahi(a)redhat.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com>
Gerrit-Reviewer: Wenchao Xia <xiawenc(a)linux.vnet.ibm.com>
4:07 a.m.
Wenchao Xia has posted comments on this change.
Change subject: Implement Popen without forking back to python
......................................................................
Patch Set 14: Verified; Looks good to me, but someone else must approve
Tested again on following cases, seems good:
1 storage domain creating and deleting.
2 thread and popen race condition case.
--
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: 14
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Saggi Mizrahi <smizrahi(a)redhat.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com>
Gerrit-Reviewer: Wenchao Xia <xiawenc(a)linux.vnet.ibm.com>
3:03 p.m.
Ayal Baron has posted comments on this change.
Change subject: Implement Popen without forking back to python
......................................................................
Patch Set 14: Looks good to me, but someone else must approve
--
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: 14
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Saggi Mizrahi <smizrahi(a)redhat.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com>
Gerrit-Reviewer: Wenchao Xia <xiawenc(a)linux.vnet.ibm.com>
3:01 p.m.
Saggi Mizrahi has posted comments on this change.
Change subject: Implement Popen without forking back to python
......................................................................
Patch Set 12: (3 inline comments)
....................................................
File vdsm/betterPopen/__init__.py
Line 36: def __init__(self, args, close_fds=False, cwd=None, env=None):
Yea, I don't support that, only args is supported. you are thinking about the
"executable" arg from the original. Like many other arguemts I removed in sake
of brevity.
Line 48: def _execute_child(self, args, executable, preexec_fn, close_fds,
I have looked at the original code. In any case it doesn't matter as I implement the
_execute_child interface.
Future python versions might change the name of the method and we will have to change as
well, it's better in my opinion then re-implementing everything.
This method is meant to be overriden and it is internally by the various Popen
implementations.
....................................................
File vdsm/betterPopen/Makefile.am
Line 27:
The entire build process requires the same machine
RHEL v FEDORA
Also we don't support any other arch
--
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(a)redhat.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com>
Gerrit-Reviewer: Wenchao Xia <xiawenc(a)linux.vnet.ibm.com>
2:36 p.m.
Saggi Mizrahi has posted comments on this change.
Change subject: Implement Popen without forking back to python
......................................................................
--
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: 13
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Saggi Mizrahi <smizrahi(a)redhat.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com>
Gerrit-Reviewer: Wenchao Xia <xiawenc(a)linux.vnet.ibm.com>
1:01 p.m.
Saggi Mizrahi has posted comments on this change.
Change subject: Implement Popen without forking back to python
......................................................................
Patch Set 9: (1 inline comment)
....................................................
File vdsm.spec.in
Line 539: %{_datadir}/%{vdsm_name}/storage/betterPopen/createprocess.so
Where do you suggest I put it then? It has to be in the import path
--
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: 9
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Saggi Mizrahi <smizrahi(a)redhat.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com>
11:15 a.m.
Dan Kenigsberg has posted comments on this change.
Change subject: Implement Popen without forking back to python
......................................................................
Patch Set 9: I would prefer that you didn't submit this
(3 inline comments)
minor stuff, haven't looked at true code.
....................................................
File vdsm.spec.in
Line 539: %{_datadir}/%{vdsm_name}/storage/betterPopen/createprocess.so
not so good, as /usr/share is supposed to be shared by different archs. not that we
support anything but x86_64, but this is bound to pain some automatic checking tool such
as "danken".
....................................................
File vdsm/storage/betterPopen/__init__.py
Line 1: import os
let's not infuriate our lawyers: state GPLness here, and in every new file you add.
Line 6:
please repeat your commit message's nice docs here, for future casual browsers.
this module may interest folks outside vdsm, make them feel welcome ;-)
these apply to the c code, too.
--
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: 9
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Saggi Mizrahi <smizrahi(a)redhat.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com>
2:07 p.m.
Dan Kenigsberg has posted comments on this change.
Change subject: Implement Popen without forking back to python
......................................................................
Patch Set 10: Fails; I would prefer that you didn't submit this
(2 inline comments)
fails `make rpm`
....................................................
File configure.ac
Line 176: PKG_CHECK_MODULES(GTHREAD2, [gthread-2.0])
hey, that's from another patch!
....................................................
File vdsm.spec.in
Line 542: %{_datadir}/%{vdsm_name}/storage/betterPopen/createprocess.so
how about placing it in /usr/lib64/python2.7/site-packages/vdsm/betterPopen ?
after all, it is not really storage-related... nobody wants to be under storage!
--
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: 10
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Saggi Mizrahi <smizrahi(a)redhat.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com>
6:12 p.m.
Ayal Baron has posted comments on this change.
Change subject: Implement Popen without forking back to python
......................................................................
Patch Set 9: Looks good to me, but someone else must approve
--
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: 9
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Saggi Mizrahi <smizrahi(a)redhat.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com>
6:35 a.m.
Ayal Baron has posted comments on this change.
Change subject: Implement Popen without forking back to python
......................................................................
Patch Set 3: Looks good to me, but someone else must approve
--
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: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Saggi Mizrahi <smizrahi(a)redhat.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com>
4:57 a.m.
Saggi Mizrahi has posted comments on this change.
Change subject: Implement Popen without forking back to python
......................................................................
Patch Set 1: (4 inline comments)
....................................................
Commit Message
Line 13: This is a simpler method of execing that doesn't go back to python after
I honestly don't know, I assume it's because they want to implement all the after
fork logic in python to make it more portable
....................................................
File vdsm/storage/betterPopen/createprocess.c
Line 23: return;
Yea, it's the init func template, I'm supposed to initialize custom exceptions and
other init code after the condition. I left it in so if someone ever adds this s\he
doesn't forget to add this condition. I'm sure the compiler will pull it out.
Line 84: argsize = sizeof(char*) * (argn + 1);
The man page states the the last arg in the array should be null. It's logical as you
don't pass the array size to the api call.
The null part is taken care of by the memset. Where I null everything just to be on the
safe side.
Line 100: if (PyList_Check(pyEnvList)) {
I thought about pulling this out but I've been too lazy, I'll move it out.
--
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>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com>
2:58 a.m.
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>
4381
days inactive
4396
days old
vdsm-patches@lists.fedorahosted.org
19 comments
5 participants
participants (5)
-
abaron＠redhat.com -
Dan Kenigsberg -
Federico Simoncelli -
smizrahi＠redhat.com -
xiawenc＠linux.vnet.ibm.com