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

Re: [PATCH] Implement svnadmin verify --force

From: Stefan Sperling <stsp_at_elego.de>
Date: Mon, 29 Oct 2012 19:23:46 +0100

On Mon, Oct 29, 2012 at 10:45:19PM +0530, Prabhu Gnana Sundar wrote:
>
> Thank you so much Stefan for your patience in reviewing and guiding
> me through.
>
> I have addressed your points in the patch attached in this mail. I
> hope I addressed all the suggestions given by you :)
> Please share your views.

Sure, see below.

I don't think we're quite done yet but we're getting close :)

> Index: subversion/svnadmin/main.c
> ===================================================================
> --- subversion/svnadmin/main.c (revision 1402414)
> +++ subversion/svnadmin/main.c (working copy)

> @@ -738,6 +743,11 @@
> notify->warning_str));
> return;
>
> + case svn_repos_notify_failure:
> + svn_handle_error2(notify->err, stderr, FALSE /* non-fatal */,
> + "svnadmin: ");
> + return;
> +

Running this on a repository with 5 revisions, where r3 is corrupt,
the output looks as follows.

$ svnadmin verify --keep-going repos
* Verified revision 0.
* Verified revision 1.
* Verified revision 2.
svnadmin: E160004: Missing node-id in node-rev at r3 (offset 787)
svnadmin: E140001: zlib (uncompress): corrupt data: Decompression of svndiff data failed
* Verified revision 5.
$

I've removed error tracing output as shown by a maintainer-mode build
to match what it would look like in a release build.

What I don't like is that there is no visual indication at all that
tells me which revision an error corresponds to.
The first error message is from r3, which is obvious only because the
error message text itself happens to mention the revision number.
The second error message is from r4, but I know that only because I
already know what the corruption is because I've deliberately corrupted
the repository myself :)

So I'd suggest printing the revision number as part of the error message.
Something like this should do:

$ svnadmin verify --keep-going repos
* Verified revision 0.
* Verified revision 1.
* Verified revision 2.
svnadmin: Error verifying revision 3: E160004: Missing node-id in node-rev at r3 (offset 787)
svnadmin: Error verifying revision 4: E140001: zlib (uncompress): corrupt data: Decompression of svndiff data failed
* Verified revision 5.
$

And while we're here, it would be great to do the same if the user did
not pass the --keep-going option:

$ svnadmin verify repos
* Verified revision 0.
* Verified revision 1.
* Verified revision 2.
svnadmin: Error verifying revision 3: E160004: Missing node-id in node-rev at r3 (offset 787)
$

Do you agree that this is better? If so, please amend the patch accordingly.
You can use the existing 'revision' field in svn_repos_notify_t to communicate
a revision number to the notification handler.

> Index: subversion/libsvn_repos/dump.c
> ===================================================================
> --- subversion/libsvn_repos/dump.c (revision 1402414)
> +++ subversion/libsvn_repos/dump.c (working copy)
> @@ -1360,9 +1360,10 @@
> }
>
> svn_error_t *
> -svn_repos_verify_fs2(svn_repos_t *repos,
> +svn_repos_verify_fs3(svn_repos_t *repos,
> svn_revnum_t start_rev,
> svn_revnum_t end_rev,
> + svn_boolean_t keep_going,
> svn_repos_notify_func_t notify_func,
> void *notify_baton,
> svn_cancel_func_t cancel_func,
> @@ -1374,6 +1375,7 @@
> svn_revnum_t rev;
> apr_pool_t *iterpool = svn_pool_create(pool);
> svn_repos_notify_t *notify;
> + svn_error_t *error;

We usually call these variables 'err', not 'error'.
Please follow this stylistic pattern.

>
> /* Determine the current youngest revision of the filesystem. */
> SVN_ERR(svn_fs_youngest_rev(&youngest, fs, pool));
> @@ -1397,9 +1399,17 @@
> end_rev, youngest);
>
> /* Verify global/auxiliary data and backend-specific data first. */
> - SVN_ERR(svn_fs_verify(svn_fs_path(fs, pool), cancel_func, cancel_baton,
> - start_rev, end_rev, pool));
> + error = svn_fs_verify(svn_fs_path(fs, pool), cancel_func, cancel_baton,
> + start_rev, end_rev, pool);
> + if(error && !keep_going)

Please always add a space between 'if' and the opening brace, like this:

  if (err && !keep_going)

We do the same for 'while', 'for', and a bunch of other C keywords.
The goal here is consistency, there's no point in arguing which style
is better. We're just following the style that the project has agreed
on using.

> + SVN_ERR(error);

If an error is known to be non-NULL (i.e. it is not SVN_NO_ERROR),
you can use svn_error_trace() instead of SVN_ERR():

  return svn_error_trace(err);

>
> + /* We don't have to notify this failure when keep_going is TRUE since
> + * that is taken care by the svn_repos_notify_failure notification.
> + * Notifying it here too would make it notified twice when keep_going
> + * is TRUE. */
> + svn_error_clear(error);
> +

Hmm... does this mean errors from svn_fs_verify() are never reported
to the notification handler? That doesn't seem right.

I think you should report this error because svn_fs_verify() function
can catch corruption outside of the revision number space. For example,
it can catch a corrupt rep-cache.db file, which the user would surely
like to know about!

To tell the notification handler that this error does not relate
to any particular revision, you can set the 'revision' field of
svn_repos_notify_t to SVN_INVALID_REVNUM. In which case the handler
can simply omit the "Error verifying revision X" annotation I suggested
above, and print the error as usual...

  svnadmin: E12345: blah blah blah

... and then we keep going if --keep-going was passed.

> Index: subversion/include/svn_repos.h
> ===================================================================
> --- subversion/include/svn_repos.h (revision 1402414)
> +++ subversion/include/svn_repos.h (working copy)

> @@ -318,6 +321,10 @@
> /** For #svn_repos_notify_load_node_start, the path of the node. */
> const char *path;
>
> + /** For #svn_repos_notify_failure, this error chain indicates what
> + went wrong during verification. */
> + svn_error_t *err;
> +

Note this comment which is part of the existing context:

> /* NOTE: Add new fields at the end to preserve binary compatibility.
> Also, if you add fields here, you have to update
> svn_repos_notify_create(). */

It doesn't look like you adjusted svn_repos_notify_create(), did you?
I think you should initialise the 'err' chain to SVN_NO_ERROR in
svn_repos_notify_create().

> Implement svnadmin verify --keep-going, which would continue the verification
> even if there is some corruption, after printing the errors to stderr.
>
> * subversion/svnadmin/main.c
> (svnadmin__cmdline_options_t):
> (svnadmin_opt_state): Add keep-going option.
> (subcommand_verify): Switch to the new svn_repos_verify_fs3 function instead
> of svn_repos_verify_fs2, and pass the keep-going option.

I'd intend multi-line log entries like this to provide more horizontal
space for the second and consecutive lines:

  (subcommand_verify): Switch to the new svn_repos_verify_fs3 function instead
   of svn_repos_verify_fs2, and pass the keep-going option.

Looks great otherwise!
Received on 2012-10-29 19:24:35 CET

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