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 > > [EMAIL PROTECTED] 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; > > } > > > > > > > >
