> > 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.
Ok, I think I may have misunderstood your original comment. I'll choose something consistent. > > > - 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. > > If you could check to make sure that something like iter(sfa) and > buffer(sfa) don't crash, that's probably good enough. Yes. In both cases, I get the expected TypeError: >>> fa = pspawn.SpawnFileAction() >>> i = iter(fa) Traceback (most recent call last): File "<stdin>", line 1, in ? TypeError: iteration over non-sequence >>> i = buffer(fa) Traceback (most recent call last): File "<stdin>", line 1, in ? TypeError: buffer object expected > > > - 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. Ok. I have everything else, aside from your remaining comments, the way I want it. It shouldn't take too long to make the change for this. I just haven't been able to get a straight answer on when changes need to be done for the respin. > 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. I haven't had a chance to do this yet. :( > > > - 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. Uh, sorry. I should have provided you a reference to begin with. Check out line 869 and 934 of subprocess.py. -j _______________________________________________ pkg-discuss mailing list [email protected] http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
