Thank you so much Stefan and Daniel.
I learnt so much from you guys during the course of this patch. I
attached a new patch and a log message with this mail. Please let me
know if I missed out on something.
Thanks and regards
Prabhu
On 10/27/2012 09:59 PM, Daniel Shahaf wrote:
Stefan Sperling wrote on Sat, Oct 27, 2012 at 10:46:59 +0200:
I don't think this is a problem at all. If revision A has a broken
rep and revision B refers to that rep, then both A and B are broken.
Even if the source of the corruption is in A only I want to know that
B is affected by it. Because B is essentially just as broken as A from
the user's point of view (somebody who wants to use the repository and
who doesn't understand FSFS internals).
I don't really care about detailed info such as which revision is
actually causing the problem. That would be nice but isn't required.
I see: your underlying use-case is "What revisions cannot be checked
out", not "Where inside the FS implementation are the corruptions".
I agree that for that use-case, Prabhu's patch is pretty much the sanest
implementation.
The patch could be improved still by adding a keep_going parameter to
svn_fs_verify() as well, or at least trapping the returned error when
svn_repos_fs_verify3().keep_going==TRUE. That seems like a low-hanging
fruit.
Pushing the verification logic into a tree crawl (in either libsvn_repos
or dag.c) would be significantly more work (both code-wise and
research-wise -- why did we change the 'verify' implementation in 1.4?),
so I'll consider it a potential future improvement - not a blocker for
this patch.
A proper 'svnadmin verify' loop will also collect errors reported and
show a summary at the end. That's additional overhead if coding this as
a loop on the command line. The repos library can easily collect this
information internally and pass it as the final notification.
I am not sure that I understand this argument.
Output of a quickly written loop on the command line is usually not
going to be as nice and comprehensive as that of a well implemented
core feature. Makes sense?
You mean,
for i in {0..$(<db/current)} ; svnadmin verify -r$i 2>(sed s/^/r$i:\ /) ./
?
But sure, the library implementation is written just once so it can take
the time to go to more effort in presenting the information.
Sounds like you're evaluating Prabhu's patch against a list of features
you'd like 'svnadmin verify' to grow someday. I think it would be
helpful if you shared that list --- it would let people share their
thoughts on the whole context/plan, not just on this patch.
The only item on that list would be the summary report already mentioned.
I just want to know how bad the overall impact of corruption is,
i.e. which revisions are affected by one or more instances of
actual corruption that have occurred in the repository.
In other words, your use case is different from mine, and the patch
addresses your use-case. +1 (concept) to apply.
Thanks,
Daniel
Index: subversion/include/svn_repos.h
===================================================================
--- subversion/include/svn_repos.h (revision 1402414)
+++ subversion/include/svn_repos.h (working copy)
@@ -193,6 +193,9 @@
/** A warning message is waiting. */
svn_repos_notify_warning = 0,
+ /** A revision is found with corruption/errors. */
+ svn_repos_notify_failure,
+
/** A revision has finished being dumped. */
svn_repos_notify_dump_rev_end,
@@ -318,9 +321,13 @@
/** For #svn_repos_notify_load_node_start, the path of the node. */
const char *path;
+ /** The error chain. */
+ 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(). */
+
} svn_repos_notify_t;
/** Callback for providing notification from the repository.
@@ -2517,8 +2524,29 @@
* cancel_baton as argument to see if the caller wishes to cancel the
* verification.
*
+ * If @a keep_going is @c TRUE, the verify process prints the error message
+ * to the stderr 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);
+
+/**
+ * Similar to svn_repos_verify_fs3(), but without force.
+ *
* @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,
Index: subversion/include/svn_fs.h
===================================================================
--- subversion/include/svn_fs.h (revision 1402414)
+++ subversion/include/svn_fs.h (working copy)
@@ -280,6 +280,7 @@
void *cancel_baton,
svn_revnum_t start,
svn_revnum_t end,
+ svn_boolean_t keep_going,
apr_pool_t *scratch_pool);
/**
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,10 @@
notify->warning_str));
return;
+ case svn_repos_notify_failure:
+ 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 +1536,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 +2002,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_fs/fs-loader.c
===================================================================
--- subversion/libsvn_fs/fs-loader.c (revision 1402414)
+++ subversion/libsvn_fs/fs-loader.c (working copy)
@@ -487,17 +487,21 @@
void *cancel_baton,
svn_revnum_t start,
svn_revnum_t end,
+ svn_boolean_t keep_going,
apr_pool_t *pool)
{
- fs_library_vtable_t *vtable;
- svn_fs_t *fs;
+ if (!keep_going)
+ {
+ fs_library_vtable_t *vtable;
+ svn_fs_t *fs;
- SVN_ERR(fs_library_vtable(&vtable, path, pool));
- fs = fs_new(NULL, pool);
+ SVN_ERR(fs_library_vtable(&vtable, path, pool));
+ fs = fs_new(NULL, pool);
- SVN_MUTEX__WITH_LOCK(common_pool_lock,
- vtable->verify_fs(fs, path, cancel_func, cancel_baton,
- start, end, pool, common_pool));
+ SVN_MUTEX__WITH_LOCK(common_pool_lock,
+ vtable->verify_fs(fs, path, cancel_func, cancel_baton,
+ start, end, pool, common_pool));
+ }
return SVN_NO_ERROR;
}
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,
@@ -1398,7 +1399,7 @@
/* 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));
+ start_rev, end_rev, keep_going, pool));
/* Create a notify object that we can reuse within the loop. */
if (notify_func)
@@ -1413,11 +1414,14 @@
void *cancel_edit_baton;
svn_fs_root_t *to_root;
apr_hash_t *props;
+ svn_error_t *err;
+ int i;
+
svn_pool_clear(iterpool);
/* Get cancellable dump editor, but with our close_directory handler. */
- SVN_ERR(get_dump_editor(&dump_editor, &dump_edit_baton,
+ err = get_dump_editor(&dump_editor, &dump_edit_baton,
fs, rev, "",
svn_stream_empty(iterpool),
NULL, NULL,
@@ -1425,7 +1429,19 @@
notify_func, notify_baton,
start_rev,
FALSE, TRUE, /* use_deltas, verify */
- iterpool));
+ iterpool);
+
+ if (err && keep_going)
+ {
+ svn_repos_notify_t *notify2 = svn_repos_notify_create(svn_repos_notify_failure, iterpool);
+ notify2->err = err;
+ notify_func(notify_baton, notify2, 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 +1449,21 @@
iterpool));
SVN_ERR(svn_fs_revision_root(&to_root, fs, rev, iterpool));
- SVN_ERR(svn_repos_replay2(to_root, "", SVN_INVALID_REVNUM, FALSE,
+ err = svn_repos_replay2(to_root, "", SVN_INVALID_REVNUM, FALSE,
cancel_editor, cancel_edit_baton,
- NULL, NULL, iterpool));
+ NULL, NULL, iterpool);
+
+ if (err && keep_going)
+ {
+ svn_repos_notify_t *notify2 = svn_repos_notify_create(svn_repos_notify_failure, iterpool);
+ notify2->err = err;
+ notify_func(notify_baton, notify2, 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));
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) : uses the new svn_repos_verify_fs3 function.
(repos_notify_handler) : svn_repos_notify_failure handles failure and
prints warning to stderr.
* 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.
* subversion/include/svn_fs.h
(svn_fs_verify) : handles keep-going. If keep-going is TRUE, svn_fs_verify
would return no error.
* subversion/libsvn_repos/dump.c
(svn_repos_verify_fs3): now handles "keep-going".
If "keep-going" is TRUE, then the error message is
written to the stderr and verify process continues.
* subversion/libsvn_repos/deprecated.c
(svn_repos_verify_fs2) : deprecated.
Now calls svn_repos_verify_fs3 with keep-going as FALSE.
Patch by: Prabhu Gnana Sundar <prabhugs{_AT_}collab.net>
