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 Fri, 28 Sep 2007 23:26:23 GMT
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

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