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 Tue, 02 Oct 2007 13:59:19 GMT
On Fri, Sep 28, 2007 at 06:26:23PM -0500, William Rowe wrote:
> Can someone please review the original state of this code and let
> me know why there is an assumption here that we even have an
> attr->parent_in to dup2 to?  It seems bogus; we should be presuming
> a simple dup always, no?
> 
> 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.

joe

> 
> Bill
> 
> wrowe@apache.org wrote:
> > Author: wrowe
> > Date: Fri Sep 28 16:21:46 2007
> > New Revision: 580515
> > 
> > URL: http://svn.apache.org/viewvc?rev=580515&view=rev
> > Log:
> > Undo the 'fix' to the unix flaw.  Yes, there still are flaws;
> > if we use apr_procattr_stderr_set() it will not close out the
> > previous handle parked there by _io_set().  But it also does
> > not attempt to touch the _io_set() no_file STATIC apr_file_t's
> > so there is nothing to otherwise fix here immediately.
> > 
> > Modified:
> >     apr/apr/trunk/threadproc/unix/proc.c
> > 
> > Modified: apr/apr/trunk/threadproc/unix/proc.c
> > URL: http://svn.apache.org/viewvc/apr/apr/trunk/threadproc/unix/proc.c?rev=580515&r1=580514&r2=580515&view=diff
> > ==============================================================================
> > --- apr/apr/trunk/threadproc/unix/proc.c (original)
> > +++ apr/apr/trunk/threadproc/unix/proc.c Fri Sep 28 16:21:46 2007
> > @@ -130,19 +130,11 @@
> >      if (attr->child_in == NULL && attr->parent_in == NULL)
> >          rv = apr_file_pipe_create(&attr->child_in, &attr->parent_in,
attr->pool);
> >      
> > -    if (child_in != NULL && rv == APR_SUCCESS) {
> > -        if (!attr->child_in || attr->child_in->filedes == -1)
> > -            rv = apr_file_dup(&attr->child_in, child_in, attr->pool);
> > -        else
> > -            rv = apr_file_dup2(attr->child_in, child_in, attr->pool);
> > -    }
> > -
> > -    if (parent_in != NULL && rv == APR_SUCCESS) {
> > -        if (!attr->parent_in || attr->parent_in->filedes == -1)
> > -            rv = apr_file_dup(&attr->parent_in, parent_in, attr->pool);
> > -        else
> > -            rv = apr_file_dup2(attr->parent_in, parent_in, attr->pool);
> > -    }
> > +    if (child_in != NULL && rv == APR_SUCCESS)
> > +        rv = apr_file_dup2(attr->child_in, child_in, attr->pool);
> > +
> > +    if (parent_in != NULL && rv == APR_SUCCESS)
> > +        rv = apr_file_dup2(attr->parent_in, parent_in, attr->pool);
> >  
> >      return rv;
> >  }
> > @@ -157,19 +149,11 @@
> >      if (attr->child_out == NULL && attr->parent_out == NULL)
> >          rv = apr_file_pipe_create(&attr->child_out, &attr->parent_out,
attr->pool);
> >  
> > -    if (child_out != NULL && rv == APR_SUCCESS) {
> > -        if (!attr->child_out || attr->child_out->filedes == -1)
> > -            rv = apr_file_dup(&attr->child_out, child_out, attr->pool);
> > -        else
> > -            rv = apr_file_dup2(attr->child_out, child_out, attr->pool);
> > -    }
> > -
> > -    if (parent_out != NULL && rv == APR_SUCCESS) {
> > -        if (!attr->parent_out || attr->parent_out->filedes == -1)
> > -            rv = apr_file_dup(&attr->parent_out, parent_out, attr->pool);
> > -        else
> > -            rv = apr_file_dup2(attr->parent_out, parent_out, attr->pool);
> > -    }
> > +    if (child_out != NULL && rv == APR_SUCCESS)
> > +        rv = apr_file_dup2(attr->child_out, child_out, attr->pool);
> > +
> > +    if (parent_out != NULL && rv == APR_SUCCESS)
> > +        rv = apr_file_dup2(attr->parent_out, parent_out, attr->pool);
> >  
> >      return rv;
> >  }
> > @@ -184,19 +168,11 @@
> >      if (attr->child_err == NULL && attr->parent_err == NULL)
> >          rv = apr_file_pipe_create(&attr->child_err, &attr->parent_err,
attr->pool);
> >  
> > -    if (child_out != NULL && rv == APR_SUCCESS) {
> > -        if (!attr->child_out || attr->child_out->filedes == -1)
> > -            rv = apr_file_dup(&attr->child_out, child_out, attr->pool);
> > -        else
> > -            rv = apr_file_dup2(attr->child_out, child_out, attr->pool);
> > -    }
> > -
> > -    if (parent_out != NULL && rv == APR_SUCCESS) {
> > -        if (!attr->parent_out || attr->parent_out->filedes == -1)
> > -            rv = apr_file_dup(&attr->parent_out, parent_out, attr->pool);
> > -        else
> > -            rv = apr_file_dup2(attr->parent_out, parent_out, attr->pool);
> > -    }
> > +    if (child_err != NULL && rv == APR_SUCCESS)
> > +        rv = apr_file_dup2(attr->child_err, child_err, attr->pool);
> > +
> > +    if (parent_err != NULL && rv == APR_SUCCESS)
> > +        rv = apr_file_dup2(attr->parent_err, parent_err, attr->pool);
> >  
> >      return rv;
> >  }
> > 
> > 
> > 
> > 

Mime
View raw message