[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

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

From: <kfogel_at_collab.net>
Date: 2005-05-27 17:45:35 CEST

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
Received on Fri May 27 18:23:57 2005

This is an archived mail posted to the Subversion Dev mailing list.