Re: svn commit: r17041 - in trunk/subversion: libsvn_subr

[Philip Martin <ph...@codematters.co.uk>]

lundblad@tigris.org writes:

> Author: lundblad
> Date: Wed Oct 26 14:14:12 2005
> New Revision: 17041

> --- trunk/subversion/libsvn_subr/io.c	(original)
> +++ trunk/subversion/libsvn_subr/io.c	Wed Oct 26 14:14:12 2005
> @@ -1733,10 +1733,14 @@
>                              const char *desc)
>  {
>    char errbuf[256];
> +  apr_file_t *stderr_handle;
> +
> +  apr_file_open_stderr (&stderr_handle, pool);
>  
>    /* What we get from APR is in native encoding. */
> -  fprintf (stderr, "%s: %s", desc, apr_strerror (status, errbuf,
> -                                                 sizeof (errbuf)));
> +  apr_file_printf (stderr_handle, "%s: %s",
> +                   desc, apr_strerror (status, errbuf,
> +                                       sizeof (errbuf)));

The lack of error handling prompted me to look at the implementation
of apr_file_open_stderr, it's basically just a wrapper around a
hard-coded file descriptor 2.  A library cannot use that as there is
no guarantee that 2 will be open, or that is stderr if it is open.
If the application has closed stderr you may be writing to a random
file.

I think svn_io_start_cmd needs to pass errfile (possibly null) via
pool userdata, and handle_child_process_error needs to retrieve it and
then write to it if it's not null.  If errfile is null I think the
error message simply has to be dropped.

-- 
Philip Martin

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: svn commit: r17041 - in trunk/subversion: libsvn_subr

["Peter N. Lundblad" <pe...@famlundblad.se>]

On Thu, 27 Oct 2005, Peter N. Lundblad wrote:

> On Thu, 27 Oct 2005, Philip Martin wrote:
>
>  > I think svn_io_start_cmd needs to pass errfile (possibly null)
> via
> > pool userdata, and handle_child_process_error needs to retrieve it and
> > then write to it if it's not null.  If errfile is null I think the
> > error message simply has to be dropped.
>
>
> This has also been suggested in some form. The problem is that one pool
> could well be used to start two commands in sequence. How do you name the
> userdata then?
>
Well, and since we use fork() (for the relevant cases), the pool is copied
before svn_io_start_cmd returns. Gah!

So, your solution seems the bestin the end. It will make the error message
available only if an errfile was passed, but that's completely reasonable.

So, now the circle is back at your first message asking why not to use the
errfile, and my answer is: that's a good idea. :-)

Thank you,
//Peter

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: svn commit: r17041 - in trunk/subversion: libsvn_subr

["Peter N. Lundblad" <pe...@famlundblad.se>]

On Thu, 27 Oct 2005, Philip Martin wrote:

> lundblad@tigris.org writes:
>
> > --- trunk/subversion/libsvn_subr/io.c	(original)
> > +++ trunk/subversion/libsvn_subr/io.c	Wed Oct 26 14:14:12 2005
> > @@ -1733,10 +1733,14 @@
> >                              const char *desc)
> >  {
> >    char errbuf[256];
> > +  apr_file_t *stderr_handle;
> > +
> > +  apr_file_open_stderr (&stderr_handle, pool);
> >
> >    /* What we get from APR is in native encoding. */
> > -  fprintf (stderr, "%s: %s", desc, apr_strerror (status, errbuf,
> > -                                                 sizeof (errbuf)));
> > +  apr_file_printf (stderr_handle, "%s: %s",
> > +                   desc, apr_strerror (status, errbuf,
> > +                                       sizeof (errbuf)));
>
> The lack of error handling prompted me to look at the implementation
> of apr_file_open_stderr, it's basically just a wrapper around a
> hard-coded file descriptor 2.  A library cannot use that as there is
> no guarantee that 2 will be open, or that is stderr if it is open.
> If the application has closed stderr you may be writing to a random
> file.
>

Funny. We start to go in circles here. As I've pointed out earlier, since
the child process already will write to stderr in some cases (dynamic
linker), so if stderr is closed and another file is open with descriptor
2, then you already have that risk.

You could argue that the caller might really "know" that the called
program will never write to stderr, but I consider that a very special
case that won't happen in practice. I mean that assuming a program will
not under any circumstances write to stderr isn't very portable.

(Regarding the lack of error handling, that's sloppy and actually could be
a problem on Windows. I looked at the libsvn_ra_svn/client.c code and
assumed it was correct, and that were nothing to do in this case, so
writing to the file was not a problem. As it turns out, on error, the file
descriptor is invalid. I'll fix both places separately. Sigh!)

 > I think svn_io_start_cmd needs to pass errfile (possibly null)
via
> pool userdata, and handle_child_process_error needs to retrieve it and
> then write to it if it's not null.  If errfile is null I think the
> error message simply has to be dropped.


This has also been suggested in some form. The problem is that one pool
could well be used to start two commands in sequence. How do you name the
userdata then?

Thansk,
//Peter

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org