Re: svn commit: r14855 - trunk/subversion/svnlook

[kf...@collab.net]

lundblad@tigris.org writes:
> --- trunk/subversion/svnlook/main.c	(original)
> +++ trunk/subversion/svnlook/main.c	Thu May 26 16:42:21 2005
> @@ -538,7 +545,7 @@
>                             apr_pool_t *pool)
>  {
>    apr_array_header_t *path_pieces;
> -  svn_error_t *err;
> +  svn_error_t *err, *err2 = NULL;
>    int i;
>    const char *full_path, *dir;
>    
> @@ -558,7 +565,7 @@
>  
>    path_pieces = svn_path_decompose (dir, pool);
>    if (! path_pieces->nelts)
> -    return SVN_NO_ERROR;
> +    goto cleanup;

See scenario (b) in my comments below.

>    full_path = "";
>    for (i = 0; i < path_pieces->nelts; i++)
> @@ -566,12 +573,14 @@
>        svn_node_kind_t kind;
>        const char *piece = ((const char **) (path_pieces->elts))[i];
>        full_path = svn_path_join (full_path, piece, pool);
> -      SVN_ERR (svn_io_check_resolved_path (full_path, &kind, pool));
> +      if ((err2 = svn_io_check_resolved_path (full_path, &kind, pool)))
> +        goto cleanup;
>  
>        /* Does this path component exist at all? */
>        if (kind == svn_node_none)
>          {
> -          SVN_ERR (svn_io_dir_make (full_path, APR_OS_DEFAULT, pool));
> +          if ((err2 = svn_io_dir_make (full_path, APR_OS_DEFAULT, pool)))
> +            goto cleanup;
>          }
>        else if (kind != svn_node_dir)
>          {
> @@ -583,10 +592,13 @@
>  
>    /* Now that we are ensured that the parent path for this file
>       exists, try once more to open it. */
> -  svn_error_clear (err);
> -  return svn_io_file_open (fh, path, 
> +  err2 = svn_io_file_open (fh, path, 
>                             APR_WRITE | APR_CREATE | APR_TRUNCATE | APR_BINARY,
>                             APR_OS_DEFAULT, pool);
> +
> + cleanup:
> +  svn_error_clear (err);
> +  return err2;
>  }

If this function had a doc string, it would be much easier to
understand the bizarre error behavior going on here.  

It appears that this tries to open the file in a straightforward way
first.  If that succeeds, then it returns success directly, and
returns the filehandle by reference.

If the straightforward open doesn't work, then it tries some other
stuff.  Depending on what happens during that other stuff, one of the
following things will happen:

   a) An error will be returned, and *fh will not be touched.
   b) Success will be returned, but *fh will still not be touched.
   c) Success will be returned, and *fh will contain the filehandle.

I don't see how (b) could be correct, but without a doc string
describing the API, it's hard to tell.

(I understand you were working with what you found, I'm just reviewing
all the code, not only your change.)

Nice change overall, though!

-Karl

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

Re: svn commit: r14855 - trunk/subversion/svnlook

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

On Fri, 27 May 2005 kfogel@collab.net wrote:

> lundblad@tigris.org writes:
> > --- trunk/subversion/svnlook/main.c	(original)
> > +++ trunk/subversion/svnlook/main.c	Thu May 26 16:42:21 2005
> > @@ -538,7 +545,7 @@
> >                             apr_pool_t *pool)
> >  {
> >    apr_array_header_t *path_pieces;
> > -  svn_error_t *err;
> > +  svn_error_t *err, *err2 = NULL;
> >    int i;
> >    const char *full_path, *dir;
> >
> > @@ -558,7 +565,7 @@
> >
> >    path_pieces = svn_path_decompose (dir, pool);
> >    if (! path_pieces->nelts)
> > -    return SVN_NO_ERROR;
> > +    goto cleanup;
>
> See scenario (b) in my comments below.
>
Yeah. Good atch! ACtually, the first test (or the second) is bogus.
svn_path_decompose only returns 0 components in the empty path case.  I've
remvoed the first test and made the second test return err. (And I added a
docustring as well. Didn't notice it was missing; I just stumbled upon
this routine while debugging the rest of this patch...)

Thanks for reviewing,
//Peter

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