apr-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "William A. Rowe, Jr." <wr...@rowe-clan.net>
Subject Re: svn commit: r580515 - /apr/apr/trunk/threadproc/unix/proc.c
Date Thu, 04 Oct 2007 20:04:30 GMT
Joe Orton wrote:
> On Tue, Oct 02, 2007 at 01:48:58PM -0500, William Rowe wrote:
>> Joe Orton wrote:
>>>> The whole code seems fragile, why we even allow (NULL, NULL) syntax
>>>> which is not supported on any other platform.  If a pipe is desired,
>>>> that is what apr_procattr_io_set is for.
>>> The interface guarantees of this code escape me too.  I certainly agree 
>>> that the the way this works in the common case by creating a pipe() then 
>>> dup2's the passed-in fds onto each end of that pipe is very silly, not 
>>> to mention inefficient.
>>> Do you mean NULL child_in and parent_in arguments, or attr fields?  It's 
>>> not clear to me why NULL arguments are handled given that the API 
>>> description explicitly states all such arguments "Must be a valid file", 
>>> though I note the trunk log.c has been changed to rely on that working.
>> It (log.c) was?
> r568326 introduced a call with a NULL parent_err:
> +                rc = apr_procattr_child_err_set(procattr, errfile, NULL);

I'm sorry the issue is confused.  THREE cases, this is a third that should
be no problem...

> Also perhaps (not very) interesting to note that of the three 
> _child_*_set() functions, _in_set says "Set the child_in and/or 
> parent_in" whereas the other two lack the "or" clause... blech!

It turns out that all flavors have always been willing to take a parent
NULL handle.

I'm speaking of the NULL+NULL case, where the combination of double nulls,
instead of assigning a file handle, create a pipe on the fly...

>> IMHO, apr_procattr_io_set() provides the mechanism for creating child pipes,
>> while apr_procattr_stdxxx_set() provides the mechanism for injecting the
>> programmer's choice of API's.
>> I'd suggest we document NULL,NULL as a supported option if it is, turn around
>> and mark it deprecated, and plan to remove that interface in APR 2.0.
> Given that the NULL, NULL case already has different behaviour across 
> platforms (Win32: noop?, Unix: creates a pipe) I'd just leave this as 
> undocumented.

In APR 1.3?  No, let's decide on a behavior, and implement it consistently.

(I'm *not* talking about what APR 1.2.x should/should-not be doing).


View raw message