Thanks to Stefan and Daniel.
Attaching a new patch addressing the suggestions given by Stefan and
Daniel. Hope this is good :)
Edited the log message also.
Thanks and regards
Prabhu
On 10/29/2012 11:53 PM, Stefan Sperling wrote:
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!
Index: subversion/svnadmin/main.c
===================================================================
--- subversion/svnadmin/main.c (revision 1402414)
+++ subversion/svnadmin/main.c (working copy)
@@ -176,6 +176,7 @@
{
svnadmin__version = SVN_OPT_FIRST_LONGOPT_ID,
svnadmin__incremental,
+ svnadmin__keep_going,
svnadmin__deltas,
svnadmin__ignore_uuid,
svnadmin__force_uuid,
@@ -288,6 +289,9 @@
N_("use format compatible with Subversion versions\n"
" earlier than 1.8")},
+ {"keep-going", svnadmin__keep_going, 0,
+ N_("continue verifying even if there is a corruption")},
+
{"memory-cache-size", 'M', 1,
N_("size of the extra in-memory cache in MB used to\n"
" minimize redundant operations. Default: 16.\n"
@@ -475,7 +479,7 @@
{"verify", subcommand_verify, {0}, N_
("usage: svnadmin verify REPOS_PATH\n\n"
"Verifies the data stored in the repository.\n"),
- {'r', 'q', 'M'} },
+ {'r', 'q', svnadmin__keep_going, 'M'} },
{ NULL, NULL, {0}, NULL, {0} }
};
@@ -505,6 +509,7 @@
svn_boolean_t clean_logs; /* --clean-logs */
svn_boolean_t bypass_hooks; /* --bypass-hooks */
svn_boolean_t wait; /* --wait */
+ svn_boolean_t keep_going; /* --keep-going */
svn_boolean_t bypass_prop_validation; /* --bypass-prop-validation */
enum svn_repos_load_uuid uuid_action; /* --ignore-uuid,
--force-uuid */
@@ -738,6 +743,14 @@
notify->warning_str));
return;
+ case svn_repos_notify_failure:
+ svn_error_clear(svn_stream_printf(feedback_stream, scratch_pool,
+ _("svnadmin: Error verifying revision %ld. "),
+ notify->revision));
+ svn_handle_error2(notify->err, stderr, FALSE /* non-fatal */,
+ ("svnadmin: "));
+ return;
+
case svn_repos_notify_dump_rev_end:
svn_error_clear(svn_stream_printf(feedback_stream, scratch_pool,
_("* Dumped revision %ld.\n"),
@@ -1527,7 +1540,8 @@
if (! opt_state->quiet)
progress_stream = recode_stream_create(stderr, pool);
- return svn_repos_verify_fs2(repos, lower, upper,
+ return svn_repos_verify_fs3(repos, lower, upper,
+ opt_state->keep_going,
!opt_state->quiet
? repos_notify_handler : NULL,
progress_stream, check_cancel, NULL, pool);
@@ -1992,6 +2006,9 @@
case svnadmin__pre_1_8_compatible:
opt_state.pre_1_8_compatible = TRUE;
break;
+ case svnadmin__keep_going:
+ opt_state.keep_going = TRUE;
+ break;
case svnadmin__fs_type:
SVN_INT_ERR(svn_utf_cstring_to_utf8(&opt_state.fs_type, opt_arg, pool));
break;
Index: subversion/libsvn_repos/deprecated.c
===================================================================
--- subversion/libsvn_repos/deprecated.c (revision 1402414)
+++ subversion/libsvn_repos/deprecated.c (working copy)
@@ -728,6 +728,27 @@
}
svn_error_t *
+svn_repos_verify_fs2(svn_repos_t *repos,
+ svn_revnum_t start_rev,
+ svn_revnum_t end_rev,
+ svn_repos_notify_func_t notify_func,
+ void *notify_baton,
+ svn_cancel_func_t cancel_func,
+ void *cancel_baton,
+ apr_pool_t *pool)
+{
+ return svn_error_trace(svn_repos_verify_fs3(repos,
+ start_rev,
+ end_rev,
+ FALSE,
+ notify_func,
+ notify_baton,
+ cancel_func,
+ cancel_baton,
+ pool));
+}
+
+svn_error_t *
svn_repos_verify_fs(svn_repos_t *repos,
svn_stream_t *feedback_stream,
svn_revnum_t start_rev,
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 *err;
/* Determine the current youngest revision of the filesystem. */
SVN_ERR(svn_fs_youngest_rev(&youngest, fs, pool));
@@ -1397,8 +1399,19 @@
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));
+ err = svn_fs_verify(svn_fs_path(fs, pool), cancel_func, cancel_baton,
+ start_rev, end_rev, pool);
+ if (err && keep_going)
+ {
+ svn_repos_notify_t *notify_failure;
+ notify_failure = svn_repos_notify_create(svn_repos_notify_failure,
+ iterpool);
+ notify_failure->err = err;
+ notify_func(notify_baton, notify_failure, iterpool);
+ svn_error_clear(err);
+ }
+ else
+ return svn_error_trace(err);
/* Create a notify object that we can reuse within the loop. */
if (notify_func)
@@ -1413,19 +1426,35 @@
void *cancel_edit_baton;
svn_fs_root_t *to_root;
apr_hash_t *props;
+ svn_error_t *err;
svn_pool_clear(iterpool);
/* Get cancellable dump editor, but with our close_directory handler. */
- SVN_ERR(get_dump_editor(&dump_editor, &dump_edit_baton,
- fs, rev, "",
- svn_stream_empty(iterpool),
- NULL, NULL,
- verify_close_directory,
- notify_func, notify_baton,
- start_rev,
- FALSE, TRUE, /* use_deltas, verify */
- iterpool));
+ err = get_dump_editor(&dump_editor, &dump_edit_baton,
+ fs, rev, "",
+ svn_stream_empty(iterpool),
+ NULL, NULL,
+ verify_close_directory,
+ notify_func, notify_baton,
+ start_rev,
+ FALSE, TRUE, /* use_deltas, verify */
+ iterpool);
+
+ if (err && keep_going)
+ {
+ svn_repos_notify_t *notify_failure;
+ notify_failure = svn_repos_notify_create(svn_repos_notify_failure,
+ iterpool);
+ notify_failure->err = err;
+ notify_failure->revision = rev;
+ notify_func(notify_baton, notify_failure, iterpool);
+ svn_error_clear(err);
+ continue;
+ }
+ else
+ SVN_ERR(err);
+
SVN_ERR(svn_delta_get_cancellation_editor(cancel_func, cancel_baton,
dump_editor, dump_edit_baton,
&cancel_editor,
@@ -1433,9 +1462,24 @@
iterpool));
SVN_ERR(svn_fs_revision_root(&to_root, fs, rev, iterpool));
- SVN_ERR(svn_repos_replay2(to_root, "", SVN_INVALID_REVNUM, FALSE,
- cancel_editor, cancel_edit_baton,
- NULL, NULL, iterpool));
+ err = svn_repos_replay2(to_root, "", SVN_INVALID_REVNUM, FALSE,
+ cancel_editor, cancel_edit_baton,
+ NULL, NULL, iterpool);
+
+ if (err && keep_going)
+ {
+ svn_repos_notify_t *notify_failure;
+ notify_failure = svn_repos_notify_create(svn_repos_notify_failure,
+ iterpool);
+ notify_failure->err = err;
+ notify_failure->revision = rev;
+ notify_func(notify_baton, notify_failure, iterpool);
+ svn_error_clear(err);
+ continue;
+ }
+ else
+ SVN_ERR(err);
+
/* While our editor close_edit implementation is a no-op, we still
do this for completeness. */
SVN_ERR(cancel_editor->close_edit(cancel_edit_baton, iterpool));
Index: subversion/libsvn_repos/notify.c
===================================================================
--- subversion/libsvn_repos/notify.c (revision 1402414)
+++ subversion/libsvn_repos/notify.c (working copy)
@@ -39,6 +39,7 @@
svn_repos_notify_t *notify = apr_pcalloc(result_pool, sizeof(*notify));
notify->action = action;
+ notify->err = SVN_NO_ERROR;
return notify;
}
Index: subversion/include/svn_repos.h
===================================================================
--- subversion/include/svn_repos.h (revision 1402414)
+++ subversion/include/svn_repos.h (working copy)
@@ -245,8 +245,11 @@
svn_repos_notify_upgrade_start,
/** A revision was skipped during loading. @since New in 1.8. */
- svn_repos_notify_load_skipped_rev
+ svn_repos_notify_load_skipped_rev,
+ /** A revision is found with corruption/errors. @since New in 1.8. */
+ svn_repos_notify_failure
+
} svn_repos_notify_action_t;
/** The type of error occurring.
@@ -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: Add new fields at the end to preserve binary compatibility.
Also, if you add fields here, you have to update
svn_repos_notify_create(). */
@@ -2517,8 +2524,28 @@
* cancel_baton as argument to see if the caller wishes to cancel the
* verification.
*
+ * If @a keep_going is @c TRUE, the verify process notifies the error message
+ * and continues.
+ *
+ * @since New in 1.8.
+ */
+svn_error_t *
+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,
+ void *cancel_baton,
+ apr_pool_t *scratch_pool);
+
+/**
+ * Like svn_repos_verify_fs3(), but with @a keep_going set to @c FALSE.
* @since New in 1.7.
+ * @deprecated Provided for backward compatibility with the 1.7 API.
*/
+SVN_DEPRECATED
svn_error_t *
svn_repos_verify_fs2(svn_repos_t *repos,
svn_revnum_t start_rev,
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.
(repos_notify_handler): Handle svn_repos_notify_failure notification by
printing warnings to stderr with the respective revision number.
* subversion/include/svn_repos.h
(svn_repos_notify_action_t): Add svn_repos_notify_failure to notify failure.
(svn_repos_verify_fs3): Newly added to handle "keep-going" option.
(svn_repos_notify_t): Add "err", the error chain which indicates what went
wrong during verification.
* subversion/libsvn_repos/dump.c
(svn_repos_verify_fs3): Handle "keep-going". If "keep-going" is TRUE, the
error message is notified and verification process continues.
* subversion/libsvn_repos/deprecated.c
(svn_repos_verify_fs2): Deprecated. Call svn_repos_verify_fs3 with
keep_going as FALSE by default to keep the old default implementation.
* subversion/libsvn_repos/notify.c
(svn_repos_notify_create): Initialise the error chain "err" to SVN_NO_ERROR.
Patch by: Prabhu Gnana Sundar <prabhugs{_AT_}collab.net>
Reviewed by: Stefan Sperling <stsp{_AT_}elego.de>
