apr-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Joe Orton <jor...@redhat.com>
Subject Re: svn commit: r580515 - /apr/apr/trunk/threadproc/unix/proc.c
Date Thu, 04 Oct 2007 15:52:02 GMT
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);

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!

> 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.

joe

Mime
View raw message