Danek,
Thanks for taking a look at this.
> pspawn.c:
>
> - line 52: sometimes you use this, sometimes you use "rc != 0".
Ugh, yes. I don't know that I have a choice about this; however, if
there's a clearer way to write this -- (rc == 0) instead of (!rc),
perhaps -- I'd be happy to change it.
The problem is that the Python C API methods either return False (0) or
NULL when a method fails. However, the posix_spawn methods return 0
on success, and an positive integer that maps to errors defined in errno
on success.
> - line 105: Is a mode_t really an unsigned long? I'm not sure how to
> read the #ifdefs in sys/types.h, so I can't tell. Should there be a
> size test, or will the uint_t version never be used?
According to types.h(3HEAD), mode_t is a ulong_t on 32-bit Solaris and a
uint_t on 64-bit Solaris. The manpage has this to say about int and
long:
For 32-bit programs, pointers and the C data types int and
long are all 32-bit quantities. For 64-bit programs,
pointers and the C data type long are defined as 64-bit
quantities.
It sounds to me like we should always be treating mode_t as an unsigned
32-bit quantity. This means I should change the k to an I. Good catch.
> - line 192: Shouldn't this be "pkg.pspawn.FileAction"?
If I've read this correctly
http://www.python.org/doc/2.4.4/api/type-structs.html, then yes, it
should be changed.
As far as I can tell, this is the only difference.
Before:
>>> import pkg.pspawn
>>> fa = pkg.pspawn.SpawnFileAction()
>>> print fa
<pspawn.FileAction object at 0x809c040>
After:
>>> import pkg.pspawn
>>> fa = pkg.pspawn.SpawnFileAction()
>>> print fa
<pkg.pspawn.FileAction object at 0x809c040>
> - line 210: I don't think this is right, as many of the things this
> implies aren't true.
According to the extending & embedding tutorial, all Python types need
to have Py_TPFLAGS_DEFAULT.
http://www.python.org/doc/2.4.4/ext/dnt-basics.html
> - line 264: if args_seq is NULL, you should return, or the TypeError
> won't be thrown, and things could get screwy.
Thanks. I caught that this morning while running lint. Same thing
applies for env_seq, where I also need to return NULL to raise the
TypeError.
> - line 288: if we get here, you won't Py_DECREF(env_seq).
Good catch. That should be goto out_env, not out_args. Same problem
exists at 268, and fixed there too.
> - line 357, 407: I wonder if closefds / closeall_fds should be renamed
> not to imply that they actually close the file descriptors, but to
> imply that they set up file descriptors for closure at spawn time.
I'm happy to pick a different name, if that would be clearer. I may end
up moving this out of C and back into Python so we can have a closefds
that works on any platform with procfs. Feel free to jump in on that
thread if you have an opinion.
> - line 386: do you ever actually use except_fd? It seems like you should
> either remove it or allow it to be a sequence of fds.
The new code doesn't need to make use of except_fd. It looks like part of
the reason it existed was so we could keep the pipe used for reporting exec
errors in the child open. However, since we don't need to do that in
our code, it's not used. I kept it in place in case the underlying
Popen code changed and wanted to use close_fds(execpt) somewhere else.
If you think this is overkill, I can take it out.
> - line 413: Why do you need this block? Our other modules seem to get
> along fine without it, but maybe they're wrong.
The extending/embedding tutorial seemed to imply that this was needed
for Windows.
http://www.python.org/doc/2.4.4/ext/dnt-basics.html
Since we're not going to build this on Windows, it's probably safe to
remove it.
Thanks for the review.
-j
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
