You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Greg Hudson <gh...@MIT.EDU> on 2005/10/21 23:48:50 UTC
Re: svn commit: r16891 - trunk/subversion/libsvn_subr
On Fri, 2005-10-21 at 08:28 -0500, lundblad@tigris.org wrote:
> Print APR child process error messages to stderr, where the caller can
> fetch them.
Um, isn't that a pretty big behavior change for a low-level function?
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r16891 - trunk/subversion/libsvn_subr
Posted by Greg Hudson <gh...@MIT.EDU>.
On Sun, 2005-10-23 at 00:05 +0200, Peter N. Lundblad wrote:
> This is a valid point, but I think it is acceptable in this case. A caller
> will always need to be prepared for unexpected things on stderr. The
> executed application might write to stderr for various reasons, for
> example failure to open a shared lib that the program needs.
I guess that's true. I thought all such messages actually came from the
shell, but apparently they can come from the runtime linker as well.
So, adding a note to the function's documentation should probably be
sufficient.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Child process start-up errors [was: svn commit: r16891 - trunk/subversion/libsvn_subr]
Posted by Julian Foad <ju...@btopenworld.com>.
Peter N. Lundblad wrote:
> On Tue, 25 Oct 2005, Julian Foad wrote:
>
>>non-zero return code. Does your patch do that? I think it should.
>
> That's taken care of by APR, i.e. exiting after the errhandler is called.
> It is returning -1 (!). That hasn't changed with my commit. The only
> change is that the reason for termination is available through stderr.
OK, good.
>>Well, isn't shared memory (in the pool) an easier kind of IPC in this
>>situation?
>
> And a mutex, or condition variable shared among processes, which isn't
> available everywhere.
[...]
> Of course it gets a copy of the pool. A fork doesn't leave any memory
> shared.
OK, thanks for explaining.
[...]
> No, I'm talking of two commands started in sequence. NOte that the output
> from the child is asynchronous and may well happen after svn_io_start_cmd
> returns.
Ah, yes. I had completely failed to see that.
>>if that's the case, I'm happy with the stderr-plus-exit-code solution.
>
> OK. I'll fix the problem philip pointed out (and add a note to the
> docstring). If someone comes up with a better solution that isn't too
> complex and that is portable, of course I wouldn't object:-)
>
> Thanks for the discussion,
Thank you too, for your patience. I (obviously) knew almost nothing about it,
but now I know a tiny bit more. :-)
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Child process start-up errors [was: svn commit: r16891 -
trunk/subversion/libsvn_subr]
Posted by "Peter N. Lundblad" <pe...@famlundblad.se>.
On Tue, 25 Oct 2005, Julian Foad wrote:
> Peter N. Lundblad wrote:
> > On Sun, 23 Oct 2005, Julian Foad wrote:
> >
> >>It seems to me that if svn_io_start_cmd fails to start the command, it
> >>would be better to return an svn_error_t rather than a message on
> >>stderr.
> >
> > That would indeed be the most ellegant and consistent solution. But, as I
> > pointed out previously, failure to execute a program canstill be reported
> > on stderr, say by the dynamic linker or a shell. So, a caller already has
> > to take care of that.
>
> Oh yes, I forgot about that. (FWIW, we explicitly avoid using a shell in this
> case.)
>
> If the caller is starting a program that (the caller knows) never writes on
> stderr, the caller will not check stderr. The caller might check the program's
> return code. A shell or dynamic linker failure is presumably accompanied by a
> non-zero return code. Does your patch do that? I think it should. (I think
> 127 is typically used as the return code for failure to start a program.)
>
That's taken care of by APR, i.e. exiting after the errhandler is called.
It is returning -1 (!). That hasn't changed with my commit. The only
change is that the reason for termination is available through stderr.
>
> >>>typedef void (apr_child_errfn_t)(apr_pool_t *proc, apr_status_t err,
> >>> const char *description);
> >>
> > You'd have to use some kind of IPC for this, say a pipe as ghudson
>
> Well, isn't shared memory (in the pool) an easier kind of IPC in this
> situation?
And a mutex, or condition variable shared among processes, which isn't
available everywhere.
>
> I tried it. I can get pool user-data in to the child process from the
> parent, but not back out again. It seems as if the child receives a
> copy of the parent's pool, so that any writes into it are not seen by
> the parent.
>
Of course it gets a copy of the pool. A fork doesn't leave any memory
shared.
> > suggested. But I don't see how the pool userdata should work. What if one
> > pool is used for more than one comman?
>
> One thread cannot start more than one command simultaneously. If commands are
> started sequentially there is no problem, because the user data is only needed
> for the brief period of starting a command. Are you thinking of multiple
> threads sharing a pool?
>
No, I'm talking of two commands started in sequence. NOte that the output
from the child is asynchronous and may well happen after svn_io_start_cmd
returns.
> > To be honest, I feel a solution like this is too complex for this kind of
> > problem. And since the child process can output to stderr anyway, it
> > wouldn't buy us that much in practice.
>
> Well, I don't know how complex it is. If a few lines of code can copy
> the data into pool user-data and back out of it in the parent, that
> would be simple. I couldn't make it work, and maybe it's not possible;
> if that's the case, I'm happy with the stderr-plus-exit-code solution.
OK. I'll fix the problem philip pointed out (and add a note to the
docstring). If someone comes up with a better solution that isn't too
complex and that is portable, of course I wouldn't object:-)
Thanks for the discussion,
//Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Child process start-up errors [was: svn commit: r16891 - trunk/subversion/libsvn_subr]
Posted by Julian Foad <ju...@btopenworld.com>.
Peter N. Lundblad wrote:
> On Sun, 23 Oct 2005, Julian Foad wrote:
>
>>It seems to me that if svn_io_start_cmd fails to start the command, it
>>would be better to return an svn_error_t rather than a message on
>>stderr.
>
> That would indeed be the most ellegant and consistent solution. But, as I
> pointed out previously, failure to execute a program canstill be reported
> on stderr, say by the dynamic linker or a shell. So, a caller already has
> to take care of that.
Oh yes, I forgot about that. (FWIW, we explicitly avoid using a shell in this
case.)
If the caller is starting a program that (the caller knows) never writes on
stderr, the caller will not check stderr. The caller might check the program's
return code. A shell or dynamic linker failure is presumably accompanied by a
non-zero return code. Does your patch do that? I think it should. (I think
127 is typically used as the return code for failure to start a program.)
>>> * @param pool Pool associated with the apr_proc_t. If your child
>>> * error function needs user data, associate it with this
>>> * pool.
[...]
>>>typedef void (apr_child_errfn_t)(apr_pool_t *proc, apr_status_t err,
>>> const char *description);
>>
> You'd have to use some kind of IPC for this, say a pipe as ghudson
Well, isn't shared memory (in the pool) an easier kind of IPC in this situation?
I tried it. I can get pool user-data in to the child process from the parent,
but not back out again. It seems as if the child receives a copy of the
parent's pool, so that any writes into it are not seen by the parent.
> suggested. But I don't see how the pool userdata should work. What if one
> pool is used for more than one comman?
One thread cannot start more than one command simultaneously. If commands are
started sequentially there is no problem, because the user data is only needed
for the brief period of starting a command. Are you thinking of multiple
threads sharing a pool?
> To be honest, I feel a solution like this is too complex for this kind of
> problem. And since the child process can output to stderr anyway, it
> wouldn't buy us that much in practice.
Well, I don't know how complex it is. If a few lines of code can copy the data
into pool user-data and back out of it in the parent, that would be simple. I
couldn't make it work, and maybe it's not possible; if that's the case, I'm
happy with the stderr-plus-exit-code solution.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Child process start-up errors [was: svn commit: r16891 -
trunk/subversion/libsvn_subr]
Posted by "Peter N. Lundblad" <pe...@famlundblad.se>.
On Sun, 23 Oct 2005, Julian Foad wrote:
> Peter N. Lundblad wrote:
> > On Sun, 23 Oct 2005, Philip Martin wrote:
> >
> >>Ah yes, you are right. Hmm, what happens if errfile is null and
> >>stderr has been closed, I think the fprintf is undefined behaviour?
> >
> > Now, *that's* a real problem! I'll use APR files instead.
>
> It seems to me that if svn_io_start_cmd fails to start the command, it
> would be better to return an svn_error_t rather than a message on
> stderr.
>
That would indeed be the most ellegant and consistent solution. But, as I
pointed out previously, failure to execute a program canstill be reported
on stderr, say by the dynamic linker or a shell. So, a caller already has
to take care of that.
> This seems like a design flaw in APR: different interfaces on different
> platforms. So, the most consistent thing to do would be to make the
> errors on "fork" platforms end up being reported the same way as on
> non-"fork" platforms, by a simple status code. This should be done in
> APR, and this "errfn_set" function should be removed. However, let's
> see if the approach is feasible and desirable first.
>
I don't know if the APR people took this route because it was simple (for
them), or they found it hard to find a portable solution.
> If my approach makes sense, and we implement it in Subversion rather
> than APR, the only remaining difficulty is getting the error information
> back from the static callback function to our "svn_io_start_cmd". This
> is done by associating user data with the pool, as described here:
>
> > /**
> > * The prototype for APR child errfn functions. (See the description
> > * of apr_procattr_child_errfn_set() for more information.)
> > * It is passed the following parameters:
> > * @param pool Pool associated with the apr_proc_t. If your child
> > * error function needs user data, associate it with this
> > * pool.
> > * @param err APR error code describing the error
> > * @param description Text description of type of processing which failed
> > */
> > typedef void (apr_child_errfn_t)(apr_pool_t *proc, apr_status_t err,
> > const char *description);
>
You'd have to use some kind of IPC for this, say a pipe as ghudson
suggested. But I don't see how the pool userdata should work. What if one
pool is used for more than one comman?
To be honest, I feel a solution like this is too complex for this kind of
problem. And since the child process can output to stderr anyway, it
wouldn't buy us that much in practice.
Thanks,
//Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Child process start-up errors [was: svn commit: r16891 - trunk/subversion/libsvn_subr]
Posted by Julian Foad <ju...@btopenworld.com>.
Peter N. Lundblad wrote:
> On Sun, 23 Oct 2005, Philip Martin wrote:
>
>>Ah yes, you are right. Hmm, what happens if errfile is null and
>>stderr has been closed, I think the fprintf is undefined behaviour?
>
> Now, *that's* a real problem! I'll use APR files instead.
It seems to me that if svn_io_start_cmd fails to start the command, it would be
better to return an svn_error_t rather than a message on stderr.
OK, strictly speaking, a sub-process has already been created by the time the
case that you are considering occurs; but only on platforms that use "fork" to
create the process. According to the docs, on other platforms an error in
attempting to run the command will be reported as a normal APR error (in the
parent process):
> /**
> * Specify an error function to be called in the child process if APR
> * encounters an error in the child prior to running the specified program.
> * @param attr The procattr describing the child process to be created.
> * @param errfn The function to call in the child process.
> * @remark At the present time, it will only be called from apr_proc_create()
> * on platforms where fork() is used. It will never be called on other
> * platforms, on those platforms apr_proc_create() will return the error
> * in the parent process rather than invoke the callback in the now-forked
> * child process.
> */
> APR_DECLARE(apr_status_t) apr_procattr_child_errfn_set(apr_procattr_t *attr,
> apr_child_errfn_t *errfn);
This seems like a design flaw in APR: different interfaces on different
platforms. So, the most consistent thing to do would be to make the errors on
"fork" platforms end up being reported the same way as on non-"fork" platforms,
by a simple status code. This should be done in APR, and this "errfn_set"
function should be removed. However, let's see if the approach is feasible and
desirable first.
If my approach makes sense, and we implement it in Subversion rather than APR,
the only remaining difficulty is getting the error information back from the
static callback function to our "svn_io_start_cmd". This is done by
associating user data with the pool, as described here:
> /**
> * The prototype for APR child errfn functions. (See the description
> * of apr_procattr_child_errfn_set() for more information.)
> * It is passed the following parameters:
> * @param pool Pool associated with the apr_proc_t. If your child
> * error function needs user data, associate it with this
> * pool.
> * @param err APR error code describing the error
> * @param description Text description of type of processing which failed
> */
> typedef void (apr_child_errfn_t)(apr_pool_t *proc, apr_status_t err,
> const char *description);
What say you?
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r16891 - trunk/subversion/libsvn_subr
Posted by "Peter N. Lundblad" <pe...@famlundblad.se>.
On Sun, 23 Oct 2005, Philip Martin wrote:
> "Peter N. Lundblad" <pe...@famlundblad.se> writes:
>
> > On Sat, 22 Oct 2005, Philip Martin wrote:
> >
> >> Any reason not to use the errfile parameter? That's where the caller
> >> expects to see error output.
> >>
> > What do you mean? In the revision at hand, errors between fork and exec
> > are written to the childs stderr, which is the same as the errfile.
>
> Ah yes, you are right. Hmm, what happens if errfile is null and
> stderr has been closed, I think the fprintf is undefined behaviour?
>
>
Now, *that's* a real problem! I'll use APR files instead.
Thanks,
//Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r16891 - trunk/subversion/libsvn_subr
Posted by Philip Martin <ph...@codematters.co.uk>.
"Peter N. Lundblad" <pe...@famlundblad.se> writes:
> On Sat, 22 Oct 2005, Philip Martin wrote:
>
>> Any reason not to use the errfile parameter? That's where the caller
>> expects to see error output.
>>
> What do you mean? In the revision at hand, errors between fork and exec
> are written to the childs stderr, which is the same as the errfile.
Ah yes, you are right. Hmm, what happens if errfile is null and
stderr has been closed, I think the fprintf is undefined behaviour?
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r16891 - trunk/subversion/libsvn_subr
Posted by "Peter N. Lundblad" <pe...@famlundblad.se>.
On Sat, 22 Oct 2005, Philip Martin wrote:
> "Peter N. Lundblad" <pe...@famlundblad.se> writes:
>
> > On Sat, 22 Oct 2005, Greg Hudson wrote:
> >
> >> On Sat, 2005-10-22 at 22:20 +0200, Peter N. Lundblad wrote:
> >
> >> An alternative--though a rather more complicated one--would be to open a
> >> pipe (set FD_CLOEXEC in the child; dunno if you can do that with APR)
> >> and marshal the error information through it back to the parent, where
> >> it could be returned to the caller programmatically.
> >>
> > I guess this might work as well, it just seems to be overkill. But if you
> > or others feel strongly about it, I could give it a try.
>
> Any reason not to use the errfile parameter? That's where the caller
> expects to see error output.
>
What do you mean? In the revision at hand, errors between fork and exec
are written to the childs stderr, which is the same as the errfile.
Best,
//Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r16891 - trunk/subversion/libsvn_subr
Posted by Philip Martin <ph...@codematters.co.uk>.
"Peter N. Lundblad" <pe...@famlundblad.se> writes:
> On Sat, 22 Oct 2005, Greg Hudson wrote:
>
>> On Sat, 2005-10-22 at 22:20 +0200, Peter N. Lundblad wrote:
>
>> An alternative--though a rather more complicated one--would be to open a
>> pipe (set FD_CLOEXEC in the child; dunno if you can do that with APR)
>> and marshal the error information through it back to the parent, where
>> it could be returned to the caller programmatically.
>>
> I guess this might work as well, it just seems to be overkill. But if you
> or others feel strongly about it, I could give it a try.
Any reason not to use the errfile parameter? That's where the caller
expects to see error output.
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r16891 - trunk/subversion/libsvn_subr
Posted by "Peter N. Lundblad" <pe...@famlundblad.se>.
On Sat, 22 Oct 2005, Greg Hudson wrote:
> On Sat, 2005-10-22 at 22:20 +0200, Peter N. Lundblad wrote:
> > It might be seen as such, although I think it is a bug that those error
> > messages were just dropped on the floor. Do you object to the change, or
> > do you just think it needs to be documented in the public docstring (the
> > latter, I could agree with in afterthought:).
>
> It seems wrong for a low-level, programmatic API to be interacting with
> stderr.
>
This is a valid point, but I think it is acceptable in this case. A caller
will always need to be prepared for unexpected things on stderr. The
executed application might write to stderr for various reasons, for
example failure to open a shared lib that the program needs.
> An alternative--though a rather more complicated one--would be to open a
> pipe (set FD_CLOEXEC in the child; dunno if you can do that with APR)
> and marshal the error information through it back to the parent, where
> it could be returned to the caller programmatically.
>
I guess this might work as well, it just seems to be overkill. But if you
or others feel strongly about it, I could give it a try.
Thanks,
//Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r16891 - trunk/subversion/libsvn_subr
Posted by Greg Hudson <gh...@MIT.EDU>.
On Sat, 2005-10-22 at 22:20 +0200, Peter N. Lundblad wrote:
> It might be seen as such, although I think it is a bug that those error
> messages were just dropped on the floor. Do you object to the change, or
> do you just think it needs to be documented in the public docstring (the
> latter, I could agree with in afterthought:).
It seems wrong for a low-level, programmatic API to be interacting with
stderr.
An alternative--though a rather more complicated one--would be to open a
pipe (set FD_CLOEXEC in the child; dunno if you can do that with APR)
and marshal the error information through it back to the parent, where
it could be returned to the caller programmatically.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r16891 - trunk/subversion/libsvn_subr
Posted by "Peter N. Lundblad" <pe...@famlundblad.se>.
On Fri, 21 Oct 2005, Greg Hudson wrote:
> On Fri, 2005-10-21 at 08:28 -0500, lundblad@tigris.org wrote:
> > Print APR child process error messages to stderr, where the caller can
> > fetch them.
>
> Um, isn't that a pretty big behavior change for a low-level function?
>
It might be seen as such, although I think it is a bug that those error
messages were just dropped on the floor. Do you object to the change, or
do you just think it needs to be documented in the public docstring (the
latter, I could agree with in afterthought:).
Regards,
//Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org