You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@apr.apache.org by wr...@apache.org on 2007/09/29 01:21:47 UTC
svn commit: r580515 - /apr/apr/trunk/threadproc/unix/proc.c
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;
}
Re: svn commit: r580515 - /apr/apr/trunk/threadproc/unix/proc.c
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
Joe Orton wrote:
> On Tue, Oct 02, 2007 at 01:48:58PM -0500, William Rowe wrote:
>> Joe Orton wrote:
>>>> 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.
>> It (log.c) was?
>
> r568326 introduced a call with a NULL parent_err:
>
> + rc = apr_procattr_child_err_set(procattr, errfile, NULL);
I'm sorry the issue is confused. THREE cases, this is a third that should
be no problem...
> Also perhaps (not very) interesting to note that of the three
> _child_*_set() functions, _in_set says "Set the child_in and/or
> parent_in" whereas the other two lack the "or" clause... blech!
It turns out that all flavors have always been willing to take a parent
NULL handle.
I'm speaking of the NULL+NULL case, where the combination of double nulls,
instead of assigning a file handle, create a pipe on the fly...
>> IMHO, apr_procattr_io_set() provides the mechanism for creating child pipes,
>> while apr_procattr_stdxxx_set() provides the mechanism for injecting the
>> programmer's choice of API's.
>>
>> I'd suggest we document NULL,NULL as a supported option if it is, turn around
>> and mark it deprecated, and plan to remove that interface in APR 2.0.
>
> Given that the NULL, NULL case already has different behaviour across
> platforms (Win32: noop?, Unix: creates a pipe) I'd just leave this as
> undocumented.
In APR 1.3? No, let's decide on a behavior, and implement it consistently.
(I'm *not* talking about what APR 1.2.x should/should-not be doing).
Bill
Re: svn commit: r580515 - /apr/apr/trunk/threadproc/unix/proc.c
Posted by Joe Orton <jo...@redhat.com>.
On Tue, Oct 02, 2007 at 01:48:58PM -0500, William Rowe wrote:
> Joe Orton wrote:
> >>
> >> 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.
>
> It (log.c) was?
r568326 introduced a call with a NULL parent_err:
+ rc = apr_procattr_child_err_set(procattr, errfile, NULL);
Also perhaps (not very) interesting to note that of the three
_child_*_set() functions, _in_set says "Set the child_in and/or
parent_in" whereas the other two lack the "or" clause... blech!
> IMHO, apr_procattr_io_set() provides the mechanism for creating child pipes,
> while apr_procattr_stdxxx_set() provides the mechanism for injecting the
> programmer's choice of API's.
>
> I'd suggest we document NULL,NULL as a supported option if it is, turn around
> and mark it deprecated, and plan to remove that interface in APR 2.0.
Given that the NULL, NULL case already has different behaviour across
platforms (Win32: noop?, Unix: creates a pipe) I'd just leave this as
undocumented.
joe
Re: svn commit: r580515 - /apr/apr/trunk/threadproc/unix/proc.c
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
Joe Orton wrote:
>>
>> 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.
It (log.c) was?
IMHO, apr_procattr_io_set() provides the mechanism for creating child pipes,
while apr_procattr_stdxxx_set() provides the mechanism for injecting the
programmer's choice of API's.
I'd suggest we document NULL,NULL as a supported option if it is, turn around
and mark it deprecated, and plan to remove that interface in APR 2.0.
Unless there are other thoughts?
Re: svn commit: r580515 - /apr/apr/trunk/threadproc/unix/proc.c
Posted by Joe Orton <jo...@redhat.com>.
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;
> > }
> >
> >
> >
> >
Re: svn commit: r580515 - /apr/apr/trunk/threadproc/unix/proc.c
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
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;
> }
>
>
>
>