John Szakmeister <john@szakmeister.net> writes:
> Resolves Issue #1074 (Need a svnadmin command to verify that the repository
> is not corrupted).
[...]
Nice log message. That's a good start! :-)
> +/* Verify */
> +
> +/** Verify the filesystem data associated with @ fs. */
> +svn_error_t *
> +svn_fs_verify(svn_fs_t *fs,
> + apr_pool_t *pool);
> +
> +
> +
Couple of comments. I notice you used a weird mix of coding styles
through your patch (sometimes doing "fn(args)", sometimes "fn
(args)"). There are lines that won't fit on an 80-column screen, and
some minor alignment issues. Please stick with the coding style
already used in a given module. Sounds picky, I know, but it really
does make life easier for everyone.
Now, about that docstring for svn_fs_verify(). Seems a little
lightweight for a module API docstring, but more than that -- it's
kinda wrong. This function doesn't actually verify all the filesystem
data. It only verifies the file contents, file and directory property
lists, and directory entries lists.
How about if we call the function svn_fs_verify_contents(), and
compose a slightly more revealing docstring for it?
> +svn_error_t *svn_fs__bdb_walk_reps (svn_fs_t *fs,
> + svn_fs__bdb_rep_handler_t rep_handler,
> + trail_t *trail)
> +{
[...]
> + /* Turn the key into a null-terminated string to be used by the handler */
> + rep_key = svn_string_ncreate(key.data, key.size, subpool);
> +
> + /* Run the handler */
> + error = (*rep_handler)(fs, rep_key->data, trail);
We don't really need to build a full string structure here. Consider
using apr_pstrmemdup() to make a regular 'const char *'.
> +/* Verifying data integrity */
> +
> +/* A simple callback function used when walking the reps */
> +svn_error_t *
> +verify_reps_handler (svn_fs_t *fs,
> + const char *rep_key,
> + trail_t *trail)
> +{
> + svn_string_t str;
> +
> + SVN_ERR (svn_fs__rep_contents (&str, fs, rep_key, trail));
> +
> + return SVN_NO_ERROR;
> +}
You know, I'm really thinking it's not such a good idea to hold the
entire contents of a file in memory. I'm thinking about those
gigabyte-sized files some folks are putting into their repositories...
Perhaps we should reconsider this part of the verification plan, hm? :-)
> +
> +/* A simple callback function used when initiating the
> + walking of the repository */
> +svn_error_t *
> +verify_reps (void *baton,
> + trail_t *trail)
> +{
> + svn_fs_t *fs = baton;
> +
> + SVN_ERR (svn_fs__bdb_walk_reps(fs, verify_reps_handler, trail));
> +
> + return SVN_NO_ERROR;
> +}
We have a longstanding convention of naming this svn_fs__retry_txn()
callbacks 'txn_body_function_name' -- so, in this case,
txn_body_verify_reps().
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Tue Jul 22 06:50:52 2003