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