On Fri, Nov 07, 2008 at 03:20:14PM -0800, [EMAIL PROTECTED] wrote: > > 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.
In both cases isn't (rc == 0) equivalent to (!rc) and (rc != 0) equivalent to (rc)? Or you could test rc against NULL if you're worried about working on platforms where the NULL pointer isn't (void *)0. Or am I missing something? I don't care all that much; it was just a consistency nit that made it look like you'd decided on one way of doing it, and then changed your mind in midstream without going back and correcting. > > - 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 Hm. At http://www.python.org/doc/2.4.4/api/type-structs.html It says this means that the following flags are set: Py_TPFLAGS_HAVE_GETCHARBUFFER Py_TPFLAGS_HAVE_SEQUENCE_IN Py_TPFLAGS_HAVE_INPLACEOPS Py_TPFLAGS_HAVE_RICHCOMPARE Py_TPFLAGS_HAVE_WEAKREFS Py_TPFLAGS_HAVE_ITER Py_TPFLAGS_HAVE_CLASS and all but the last one of those seems to imply that one of the members of the type structure is set to something other than NULL. If you could check to make sure that something like iter(sfa) and buffer(sfa) don't crash, that's probably good enough. > > - 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. Having closefds() in Python would be nice, if not strictly necessary for this wad. Speaking of which, did you ever find a bug open against Python for spawn support on non-MS platforms? If there isn't one, could you file it? Providing the code in the report might even help, though perhaps this is something we should give to Laca for the set of Solaris patches he keeps (and hopefully is trying to get upstream). Obviously, this is asynchronous from everything else. > > - 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. This implies that except_fd is somewhere in the subprocess or Popen API, but I can't find it. Danek _______________________________________________ pkg-discuss mailing list [email protected] http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
