apr-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Joe Orton <jor...@redhat.com>
Subject Re: [PATCH] inheriting pipes
Date Wed, 05 Mar 2003 19:54:01 GMT
On Wed, Mar 05, 2003 at 08:03:51PM +0100, Christian Kratzer wrote:
> Hi,
> 
> On Wed, 5 Mar 2003, Joe Orton wrote:
> 
> > Pipes created by the Unix implementation of apr_file_pipe_create are
> > inherited by default, but you can't make them uninherited without
> > registering a custom cleanup handler.  Any objections to this patch,
> > which lets apr_file_inherit_unset work on a pipe for Unix? (it looks
> > like it might already work on Win32).
> >
> > Alternatively I wonder whether it would be better to not leak pipes by
> > default, though presumably it was done this way for a reason...
> [snipp]
> > +    (*in)->flags = (*out)->flags = APR_INHERIT;
> 
> With this you are setting the inherit flag instead of clearing it.
> Should this not rather be the following
> 
> +    (*in)->flags &= ~APR_INHERIT);
> +    (*out)->flags &= ~APR_INHERIT);
> 
> which would clear APR_INHERIT in both flags.

The APR_INHERIT flag needs to be set, since the current code does allow
pipes to be inherited by default.  If the APR_INHERIT flag is not set,
apr_file_inherit_unset has no effect.  To answer Bill's question as
well, flags is zero at this point in the function, so a bitop is not
need, just an assignment.

> But why not just register apr_unix_file_cleanup for cleanup like in

Indeed, I wonder it was implement this way too.  I think the change you
suggest is a little more intrusive than mine, as currently some callers
are compensating for the fact that no child_cleanup is registered.
 
I also wonder why the code goes to these lengths when on Unix setting
the CLOEXEC flag would probably suffice.

> --- httpd-2.0.44/srclib/apr/file_io/unix/pipe.c.orig    Sun Mar  2 00:54:10
> 2003
> +++ httpd-2.0.44/srclib/apr/file_io/unix/pipe.c Sun Mar  2 00:54:36 2003
> @@ -225,9 +225,9 @@
>  #endif
> 
>      apr_pool_cleanup_register((*in)->pool, (void *)(*in), apr_unix_file_cleanup,
> -                         apr_pool_cleanup_null);
> +                         apr_unix_file_cleanup);
>      apr_pool_cleanup_register((*out)->pool, (void *)(*out), apr_unix_file_cleanup,
> -                         apr_pool_cleanup_null);
> +                         apr_unix_file_cleanup);
>      return APR_SUCCESS;
>  }
> 
> would this not be more to the point of getting the descriptors closed on exec.
> 
> Greetings
> Christian
> 
> -- 
> CK Software GmbH
> Christian Kratzer,         Schwarzwaldstr. 31, 71131 Jettingen
> Email: ck@cksoft.de
> Phone: +49 7452 889-135    Open Software Solutions, Network Security
> Fax:   +49 7452 889-136    FreeBSD spoken here!
> 

Mime
View raw message