PTAL.
I also added test coverage for both the pure python implementation and the new
implementation as well as some previously untested code paths. found & fixed
some bugs with this change as a result. yay tests!
http://codereview.appspot.com/223077/diff/25/1007
File Doc/library/subprocess.rst (right):
http://codereview.appspot.com/223077/diff/25/1007#newcode110
Doc/library/subprocess.rst:110: If *preexec_fn* is set to a callable object,
this object will be called in the
On 2010年03月02日 02:17:12, Jeffrey Yasskin wrote:
> Could you add the "not thread-safe" comment here too?
Done. What do you think of my wording?
http://codereview.appspot.com/223077/diff/25/1009
File Lib/subprocess.py (right):
http://codereview.appspot.com/223077/diff/25/1009#newcode75
Lib/subprocess.py:75: If preexec_fn is set to a callable object, this object
will be called
On 2010年03月02日 02:17:12, Jeffrey Yasskin wrote:
> Please mention here that passing preexec_fn makes POpen not thread-safe.
Done.
http://codereview.appspot.com/223077/diff/25/1009#newcode649
Lib/subprocess.py:649: # are None when not using PIPEs. The child objects are
None
On 2010年03月02日 02:17:12, Jeffrey Yasskin wrote:
> Please update this comment to reflect that they're now -1.
Done.
http://codereview.appspot.com/223077/diff/25/1009#newcode1079
Lib/subprocess.py:1079: env_list = []
On 2010年03月02日 02:17:12, Jeffrey Yasskin wrote:
> This line is useless, right?
Done.
http://codereview.appspot.com/223077/diff/25/1009#newcode1102
Lib/subprocess.py:1102: # Python code running in the child process after fork,
On 2010年03月02日 02:17:12, Jeffrey Yasskin wrote:
> This isn't a complete sentence.
heh must've been a leftover from some previous wording I was trying out.
http://codereview.appspot.com/223077/diff/25/1009#newcode1195
Lib/subprocess.py:1195: data = os.read(errpipe_read, 1048576)
On 2010年03月01日 04:45:44, gregory.p.smith wrote:
> svn up and merge, this is called using _eintr_retry_call() now. same with the
> waitpid below.
Done.
http://codereview.appspot.com/223077/diff/25/1008
File Lib/test/test_subprocess.py (right):
http://codereview.appspot.com/223077/diff/25/1008#newcode563
Lib/test/test_subprocess.py:563: # Its the perror no such file or directory
message.
On 2010年03月02日 02:17:12, Jeffrey Yasskin wrote:
> Can you word this more clearly?
Done.
http://codereview.appspot.com/223077/diff/25/1008#newcode564
Lib/test/test_subprocess.py:564: # Can we even reliably test for this as it is a
On 2010年03月02日 02:17:12, Jeffrey Yasskin wrote:
> Assert the errno value? Trigger another copy of the same error message and
check
> that they're equal?
Good idea. done.
http://codereview.appspot.com/223077/diff/25/1011
File Modules/_posixsubprocess.c (right):
http://codereview.appspot.com/223077/diff/25/1011#newcode4
Modules/_posixsubprocess.c:4: * Licensed under the Apache License, Version 2.0
(the "License");
On 2010年03月02日 02:17:12, Jeffrey Yasskin wrote:
> Do we really need this? Most of the other modules don't include it.
Probably not. I'll remove it.
This is the boilerplate I put on code when I'm releasing it not as part of the
python project so that it could later be adopted if desired.
http://codereview.appspot.com/223077/diff/25/1011#newcode31
Modules/_posixsubprocess.c:31: static void child_exec(char *const exec_array[],
On 2010年03月02日 02:17:12, Jeffrey Yasskin wrote:
> Please comment that all of the code in this function must only use
> async-signal-safe functions, listed at `man 7 signal` or
> http://www.opengroup.org/onlinepubs/009695399/functions/xsh_chap02_04.html. 
> This restriction is documented at
> http://www.opengroup.org/onlinepubs/009695399/functions/fork.html.
Good references. Done.
http://codereview.appspot.com/223077/diff/25/1011#newcode102
Modules/_posixsubprocess.c:102: if (preexec_fn != Py_None &&
preexec_fn_args_tuple) {
On 2010年03月02日 02:17:12, Jeffrey Yasskin wrote:
> Please comment that this block is where the user asks us to deadlock her
> program.
Done. :)
http://codereview.appspot.com/223077/diff/25/1011#newcode106
Modules/_posixsubprocess.c:106: /* memory allocation and thus potential for
deadlock. */
On 2010年03月02日 02:17:12, Jeffrey Yasskin wrote:
> Consistentize your comment style?
Done.
http://codereview.appspot.com/223077/diff/25/1011#newcode138
Modules/_posixsubprocess.c:138: err_msg = strerror(saved_errno);
On 2010年03月02日 02:17:12, Jeffrey Yasskin wrote:
> strerror() is not async-signal-safe (wow). You might use the deprecated
> sys_errlist directly ... or map the errno back to a string in the parent
> process?
Right, I'll just defer that to the parent.
http://codereview.appspot.com/223077/diff/25/1011#newcode139
Modules/_posixsubprocess.c:139: snprintf(hex_errno, sizeof(hex_errno), "%x",
saved_errno);
On 2010年03月02日 02:17:12, Jeffrey Yasskin wrote:
> snprintf is not async-signal-safe. You'll need a loop:
> 
> char *cur = hex_errno + sizeof(hex_errno) - 1;
> while (saved_errno != 0 && cur > hex_errno) {
> *cur-- = "0123456789ABCDEF"[saved_errno % 16];
> saved_errno /= 16;
> }
> write(errpipe_write, cur, hex_errno + sizeof(hex_errno) - cur);
Thanks. cur + 1 in the write due the -- in the loop. Done.
http://codereview.appspot.com/223077/diff/25/1011#newcode168
Modules/_posixsubprocess.c:168: args, "OOOzOiiiiiiiiiO:fork_exec",
On 2010年03月02日 02:17:12, Jeffrey Yasskin wrote:
> AAAEEEEiiiiOOOOOIIIIIzzzz!!
> 
> (sorry)
hehehe :)
side note: I wonder how much time in python is spend in repeatedly parsing these
getargs strings. probably negligible. but it has always annoyed me that it is
yet another big switch statement.
http://codereview.appspot.com/223077/diff/25/1011#newcode212
Modules/_posixsubprocess.c:212: if (!converted_args)
On 2010年03月02日 02:17:12, Jeffrey Yasskin wrote:
> You use ==NULL elsewhere in this function.
Done.
http://codereview.appspot.com/223077/diff/25/1011#newcode217
Modules/_posixsubprocess.c:217: if (!borrowed_arg) {
On 2010年03月02日 02:17:12, Jeffrey Yasskin wrote:
> _GET_ITEM can't fail.
Done.
http://codereview.appspot.com/223077/diff/25/1011#newcode218
Modules/_posixsubprocess.c:218: Py_DECREF(converted_args);
On 2010年03月02日 02:17:12, Jeffrey Yasskin wrote:
> I would probably initialize these to NULL, Py_CLEAR them on the successful
path,
> and have cleanup: XDECREF them.
Done.
http://codereview.appspot.com/223077/diff/25/1011#newcode249
Modules/_posixsubprocess.c:249: pid = fork();
On 2010年03月02日 02:17:12, Jeffrey Yasskin wrote:
> On 2010年03月01日 04:45:44, gregory.p.smith wrote:
> > If a preexec_fn exists:
> > * I need to acquire the import lock before calling fork.
> > * in the child process call PyOS_AfterFork.
> > * in the parent process release the import lock after the fork.
> > 
> > (see posixmodule.c)
> > And call PyOS_AfterFork() in the child process afterwards 
> 
> Is this a TODO?
yep. I didn't have the computer with my sandbox on it in front of me at the
time.
Done.
http://codereview.appspot.com/223077/diff/25/1011#newcode251
Modules/_posixsubprocess.c:251: /* Child process */
On 2010年03月02日 02:17:12, Jeffrey Yasskin wrote:
> Please comment that code from here to _exit() must only use async-signal-safe
> functions, listed at `man 7 signal` or
> http://www.opengroup.org/onlinepubs/009695399/functions/xsh_chap02_04.html.
Done.
http://codereview.appspot.com/223077/diff/25/1011#newcode269
Modules/_posixsubprocess.c:269: /* Reenable gc in the parent process (or if fork
failed). */
On 2010年03月02日 02:17:12, Jeffrey Yasskin wrote:
> This needs to happen in cleanup too, right? Since there are several branches
to
> there after gc is disabled.
Right. I've refactored the enable call out to a function and called it where
needed.
http://codereview.appspot.com/223077/diff/25/1011#newcode284
Modules/_posixsubprocess.c:284: return PyErr_SetFromErrno(PyExc_OSError); /*
fork() failed */
On 2010年03月02日 02:17:12, Jeffrey Yasskin wrote:
> You should collect errno immediately after the fork() because all the work
since
> then could change it.
Done.
http://codereview.appspot.com/223077/diff/25/1011#newcode330
Modules/_posixsubprocess.c:330: {"fork_exec",	subprocess_fork_exec, 
METH_VARARGS, subprocess_fork_exec_doc},
On 2010年03月01日 04:45:44, gregory.p.smith wrote:
> line length
Done.
http://codereview.appspot.com/223077/diff/25/1011#newcode339
Modules/_posixsubprocess.c:339: -1, /* TODO(gps): double check if this could be
0 */
On 2010年03月02日 02:17:12, Jeffrey Yasskin wrote:
> http://docs.python.org/3.1/c-api/module.html?highlight=pymoduledef#PyModuleDef
> says "If no memory is needed, set this to -1."
Done.
http://codereview.appspot.com/223077/diff/25/1011#newcode354
Modules/_posixsubprocess.c:354: if (m == NULL)
On 2010年03月02日 02:17:12, Jeffrey Yasskin wrote:
> You can just return m here.
Done.
http://codereview.appspot.com/223077/diff/25/1006
File Objects/abstract.c (right):
http://codereview.appspot.com/223077/diff/25/1006#newcode2733
Objects/abstract.c:2733: * be freed by the caller using free().
On 2010年03月02日 02:17:12, Jeffrey Yasskin wrote:
> Or using _Py_FreeCharPArray()?
Done.
http://codereview.appspot.com/223077/diff/25/1006#newcode2735
Objects/abstract.c:2735: char ** _PySequence_BytesToCharpArray(PyObject* self)
On 2010年03月02日 02:17:12, Jeffrey Yasskin wrote:
> Python style puts a newline after the return type.
Done.
http://codereview.appspot.com/223077/diff/25/1006#newcode2737
Objects/abstract.c:2737: const char ** array;
On 2010年03月02日 02:17:12, Jeffrey Yasskin wrote:
> Your consts are inconsistent between here, the return type, and
> _Py_FreeCharPArray()'s argument.
Here I'm mallocing and assigning to it so no const really makes sense. But I've
updated the return type and the uses elsewhere to be "char *const *"
I'm really not used to using const so I rely on the compiler to whine when I
misuse it. No warnings with the updated code. (I had to cast the pointer
passed to the final free to a void* to avoid a warning about discarding the
qualifier; annoying, but I don't mind doing that on free).
http://codereview.appspot.com/223077/diff/25/1006#newcode2744
Objects/abstract.c:2744: array = calloc(argc+1, sizeof(char *));
On 2010年03月02日 02:17:12, Jeffrey Yasskin wrote:
> Why calloc instead of malloc? It looks like you initialize the memory
yourself.
Good point. Done.
http://codereview.appspot.com/223077/diff/25/1001
File Python/pythonrun.c (right):
http://codereview.appspot.com/223077/diff/25/1001#newcode2142
Python/pythonrun.c:2142: _Py_RestoreSignals(void)
On 2010年03月02日 02:17:12, Jeffrey Yasskin wrote:
> Please document that this and PyOS_setsig are (and must remain)
> async-signal-safe.
Done.
http://codereview.appspot.com/223077/diff/25/1001#newcode2264
Python/pythonrun.c:2264: siginterrupt(sig, 1);
On 2010年03月02日 02:17:12, Jeffrey Yasskin wrote:
> siginterrupt isn't listed in the async-signal-safe function list at
> http://www.opengroup.org/onlinepubs/009695399/functions/xsh_chap02_04.html,
but
> sigaction() is, and siginterrupt is specified to act like a series of calls to
> sigaction, so it's probably safe for after-fork use, even though it may have a
> race when used inside a signal handler.
Yeah thats my thought as well.
Regardless, I doubt many systems even compile this code. I expect everything 
in use today will use the HAVE_SIGACTION code above.

Sign in to reply to this message.

Issue 223077: Replace subprocess internals with _posixsubprocess extension module
Created 15 years, 10 months ago by gregory.p.smith
Modified 15 years, 10 months ago
Reviewers: Jeffrey Yasskin, guido
Base URL: http://svn.python.org/view/*checkout*/python/branches/py3k/
Comments: 93

Powered by Google App Engine