You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Stefan Sperling <st...@elego.de> on 2011/07/13 15:59:36 UTC
svnadmin verify + rep-cache.db
Hi all,
while repairing some bad repository breakage we noticed that
svnadmin verify does not check whether offsets mentioned in
rep-cache.db point to valid reps.
In some cases I had to append repaired reps after the changed-paths
section in revision files and leave broken reps in place to avoid
adjusting offsets everywhere.
So now 'svnadmin verify' runs fine. But when the rep-cache is
referred to during commit we can still run into reps that are broken.
Obviously we'll either have to fix up the rep-cache as well,
or simply delete it. But the point is that 'svnadmin verify'
does not complain.
Would it be feasable to add this as a separate pass of 'svnadmin verify'?
Should I file an issue about this?
For instance, good output could look like this:
$ svnadmin verify repos
* Verified revision 1.
...
* Verified revision N.
* Verified rep-cache.
$
And errors like this:
$ svnadmin verify repos
* Verified revision 1.
...
* Verified revision N.
svnamdin: rep-cache entry '8fe47fa166154fc6e7f2e78366591c470ba75cf7' is invalid
svnadmin: Malformed representation header
$
Re: svnadmin verify + rep-cache.db
Posted by Daniel Shahaf <da...@elego.de>.
Daniel Shahaf wrote on Wed, Jul 13, 2011 at 17:11:34 +0300:
> Stefan Sperling wrote on Wed, Jul 13, 2011 at 15:59:36 +0200:
> > while repairing some bad repository breakage we noticed that
> > svnadmin verify does not check whether offsets mentioned in
> > rep-cache.db point to valid reps.
>
> Does implementing this require API changes? (extending the fs-loader.h
> vtables)
>
> There is currently no svn_fs_verify() API.
Well, here it is. I modeled it after svn_fs_upgrade() --- i.e., put it
in the library vtable and have the public API take an FS path rather
than an FS object --- if that's not right someone please let me know.
[[[
Add an svn_fs_verify() function to the public API, and implement
it as a no-op in both backends.
* subversion/include/svn_fs.h
(svn_fs_verify): New.
* subversion/libsvn_fs/fs-loader.h
(fs_library_vtable_t): Add 'fs_verify' member.
* subversion/libsvn_fs/fs-loader.c
(svn_fs_verify): Implement
* subversion/libsvn_fs_base/fs.c
(base_verify): New no-op.
* subversion/libsvn_fs_fs/fs.c
(fs_verify): New, prepares svn_fs_t and forwards to...
* subversion/libsvn_fs_fs/fs_fs.c
(svn_fs_fs__verify): .. this new function. Currently no-op.
* subversion/libsvn_fs_fs/fs_fs.h
(svn_fs_fs__verify): New function.
]]]
[[[
Index: subversion/include/svn_fs.h
===================================================================
--- subversion/include/svn_fs.h (revision 1145819)
+++ subversion/include/svn_fs.h (working copy)
@@ -246,6 +246,21 @@ svn_fs_upgrade(const char *path,
apr_pool_t *pool);
/**
+ * Perform backend-specific data consistency and correctness validations
+ * to the Subversion filesystem located in the directory @a path.
+ * Use @a pool for necessary allocations.
+ *
+ * @note You probably don't want to use this directly. Take a look at
+ * svn_repos_verify_fs2() instead, which does non-backend-specific
+ * verificatiosn as well.
+ *
+ * @since New in 1.7.
+ */
+svn_error_t *
+svn_fs_verify(const char *path,
+ apr_pool_t *pool);
+
+/**
* Return, in @a *fs_type, a string identifying the back-end type of
* the Subversion filesystem located in @a path. Allocate @a *fs_type
* in @a pool.
Index: subversion/libsvn_fs_fs/fs.c
===================================================================
--- subversion/libsvn_fs_fs/fs.c (revision 1145819)
+++ subversion/libsvn_fs_fs/fs.c (working copy)
@@ -256,6 +256,18 @@ fs_upgrade(svn_fs_t *fs, const char *path, apr_poo
}
static svn_error_t *
+fs_verify(svn_fs_t *fs, const char *path, apr_pool_t *pool,
+ apr_pool_t *common_pool)
+{
+ SVN_ERR(svn_fs__check_fs(fs, FALSE));
+ SVN_ERR(initialize_fs_struct(fs));
+ SVN_ERR(svn_fs_fs__open(fs, path, pool));
+ SVN_ERR(svn_fs_fs__initialize_caches(fs, pool));
+ SVN_ERR(fs_serialized_init(fs, common_pool, pool));
+ return svn_fs_fs__verify(fs, pool);
+}
+
+static svn_error_t *
fs_pack(svn_fs_t *fs,
const char *path,
svn_fs_pack_notify_t notify_func,
@@ -342,6 +354,7 @@ static fs_library_vtable_t library_vtable = {
fs_open,
fs_open_for_recovery,
fs_upgrade,
+ fs_verify,
fs_delete_fs,
fs_hotcopy,
fs_get_description,
Index: subversion/libsvn_fs_fs/fs_fs.c
===================================================================
--- subversion/libsvn_fs_fs/fs_fs.c (revision 1145819)
+++ subversion/libsvn_fs_fs/fs_fs.c (working copy)
@@ -1323,6 +1323,14 @@ svn_fs_fs__upgrade(svn_fs_t *fs, apr_pool_t *pool)
}
+/** Verifying. **/
+svn_error_t *
+svn_fs_fs__verify(svn_fs_t *fs, apr_pool_t *pool)
+{
+ return SVN_NO_ERROR; /* ### Not implemented. Should dereference rep-cache */
+}
+
+
/* SVN_ERR-like macros for dealing with recoverable errors on mutable files
*
* Revprops, current, and txn-current files are mutable; that is, they
Index: subversion/libsvn_fs_fs/fs_fs.h
===================================================================
--- subversion/libsvn_fs_fs/fs_fs.h (revision 1145819)
+++ subversion/libsvn_fs_fs/fs_fs.h (working copy)
@@ -38,6 +38,10 @@ svn_error_t *svn_fs_fs__open(svn_fs_t *fs,
svn_error_t *svn_fs_fs__upgrade(svn_fs_t *fs,
apr_pool_t *pool);
+/* Verify the fsfs filesystem FS. Use POOL for temporary allocations. */
+svn_error_t *svn_fs_fs__verify(svn_fs_t *fs,
+ apr_pool_t *pool);
+
/* Copy the fsfs filesystem at SRC_PATH into a new copy at DST_PATH.
Use POOL for temporary allocations. */
svn_error_t *svn_fs_fs__hotcopy(const char *src_path,
Index: subversion/libsvn_fs/fs-loader.h
===================================================================
--- subversion/libsvn_fs/fs-loader.h (revision 1145819)
+++ subversion/libsvn_fs/fs-loader.h (working copy)
@@ -86,6 +86,10 @@ typedef struct fs_library_vtable_t
apr_pool_t *common_pool);
svn_error_t *(*upgrade_fs)(svn_fs_t *fs, const char *path, apr_pool_t *pool,
apr_pool_t *common_pool);
+ svn_error_t *(*verify_fs)(svn_fs_t *fs, const char *path,
+ /* ### notification? */
+ svn_cancel_func_t cancel_func, void *cancel_baton,
+ apr_pool_t *pool);
svn_error_t *(*delete_fs)(const char *path, apr_pool_t *pool);
svn_error_t *(*hotcopy)(const char *src_path, const char *dest_path,
svn_boolean_t clean, apr_pool_t *pool);
Index: subversion/libsvn_fs/fs-loader.c
===================================================================
--- subversion/libsvn_fs/fs-loader.c (revision 1145819)
+++ subversion/libsvn_fs/fs-loader.c (working copy)
@@ -459,6 +459,27 @@ svn_fs_upgrade(const char *path, apr_pool_t *pool)
return svn_error_trace(err2);
}
+svn_error_t *
+svn_fs_verify(const char *path, apr_pool_t *pool)
+{
+ svn_error_t *err;
+ svn_error_t *err2;
+ fs_library_vtable_t *vtable;
+ svn_fs_t *fs;
+
+ SVN_ERR(fs_library_vtable(&vtable, path, pool));
+ fs = fs_new(NULL, pool);
+ SVN_ERR(acquire_fs_mutex());
+ err = vtable->verify_fs(fs, path, pool, common_pool);
+ err2 = release_fs_mutex();
+ if (err)
+ {
+ svn_error_clear(err2);
+ return svn_error_trace(err);
+ }
+ return svn_error_trace(err2);
+}
+
const char *
svn_fs_path(svn_fs_t *fs, apr_pool_t *pool)
{
Index: subversion/libsvn_fs_base/fs.c
===================================================================
--- subversion/libsvn_fs_base/fs.c (revision 1145819)
+++ subversion/libsvn_fs_base/fs.c (working copy)
@@ -874,6 +874,15 @@ base_upgrade(svn_fs_t *fs, const char *path, apr_p
}
static svn_error_t *
+base_verify(svn_fs_t *fs, const char *path, apr_pool_t *pool,
+ apr_pool_t *common_pool)
+{
+ /* ### Any boilerplate needed here? */
+ /* Verifying is currently a no op for BDB. */
+ return SVN_NO_ERROR;
+}
+
+static svn_error_t *
base_bdb_recover(svn_fs_t *fs,
svn_cancel_func_t cancel_func, void *cancel_baton,
apr_pool_t *pool)
@@ -1342,6 +1351,7 @@ static fs_library_vtable_t library_vtable = {
base_open,
base_open_for_recovery,
base_upgrade,
+ base_verify,
base_delete_fs,
base_hotcopy,
base_get_description,
]]]
Re: svnadmin verify + rep-cache.db
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Does implementing this require API changes? (extending the fs-loader.h
vtables)
There is currently no svn_fs_verify() API.
Stefan Sperling wrote on Wed, Jul 13, 2011 at 15:59:36 +0200:
> Hi all,
>
> while repairing some bad repository breakage we noticed that
> svnadmin verify does not check whether offsets mentioned in
> rep-cache.db point to valid reps.
>
> In some cases I had to append repaired reps after the changed-paths
> section in revision files and leave broken reps in place to avoid
> adjusting offsets everywhere.
>
> So now 'svnadmin verify' runs fine. But when the rep-cache is
> referred to during commit we can still run into reps that are broken.
>
> Obviously we'll either have to fix up the rep-cache as well,
> or simply delete it. But the point is that 'svnadmin verify'
> does not complain.
>
> Would it be feasable to add this as a separate pass of 'svnadmin verify'?
> Should I file an issue about this?
>
> For instance, good output could look like this:
>
> $ svnadmin verify repos
> * Verified revision 1.
> ...
> * Verified revision N.
> * Verified rep-cache.
>
> $
>
> And errors like this:
>
> $ svnadmin verify repos
> * Verified revision 1.
> ...
> * Verified revision N.
> svnamdin: rep-cache entry '8fe47fa166154fc6e7f2e78366591c470ba75cf7' is invalid
> svnadmin: Malformed representation header
>
> $
Re: svnadmin verify + rep-cache.db
Posted by Peter Samuelson <pe...@p12n.org>.
[Stefan Sperling]
> So now 'svnadmin verify' runs fine. But when the rep-cache is
> referred to during commit we can still run into reps that are broken.
>
> Obviously we'll either have to fix up the rep-cache as well,
> or simply delete it. But the point is that 'svnadmin verify'
> does not complain.
>
> Would it be feasable to add this as a separate pass of 'svnadmin verify'?
> Should I file an issue about this?
+1 - a tool like 'svnadmin verify' should have as many integrity checks
as possible. It should verify anything that _can_ be verified.
Followup: shouldn't we have a tool, similar to
tools/server-side/svn-populate-node-origins-index.c, to rebuild
rep-cache.db from scratch? In fact, I believe it would be useful to
rebuild both at once, along with anything else we decide to cache now
or in the future. Perhaps 'svnadmin verify --rebuild-caches', but
could be prototyped as a separate executable under tools/server-side/.
--
Peter Samuelson | org-tld!p12n!peter | http://p12n.org/
Re: svnadmin verify + rep-cache.db
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Stefan Sperling wrote on Wed, Jul 13, 2011 at 15:59:36 +0200:
> Hi all,
>
> while repairing some bad repository breakage we noticed that
> svnadmin verify does not check whether offsets mentioned in
> rep-cache.db point to valid reps.
>
> In some cases I had to append repaired reps after the changed-paths
> section in revision files and leave broken reps in place to avoid
> adjusting offsets everywhere.
>
> So now 'svnadmin verify' runs fine. But when the rep-cache is
> referred to during commit we can still run into reps that are broken.
>
> Obviously we'll either have to fix up the rep-cache as well,
> or simply delete it. But the point is that 'svnadmin verify'
> does not complain.
>
> Would it be feasable to add this as a separate pass of 'svnadmin verify'?
> Should I file an issue about this?
>
> For instance, good output could look like this:
>
> $ svnadmin verify repos
> * Verified revision 1.
> ...
> * Verified revision N.
> * Verified rep-cache.
>
> $
>
> And errors like this:
>
> $ svnadmin verify repos
> * Verified revision 1.
> ...
> * Verified revision N.
> svnamdin: rep-cache entry '8fe47fa166154fc6e7f2e78366591c470ba75cf7' is invalid
> svnadmin: Malformed representation header
>
> $
Implemented and committed. Output looks like this:
[[[
% sqlite3 r1/db/rep-cache.db "update rep_cache set offset = offset + 1 where offset = 550;"
% $svnadmin verify r1
* Verified revision 0.
* Verified revision 1.
DBG: fs_fs.c:7789: verify_walker(): got r1/0
DBG: fs_fs.c:7789: verify_walker(): got r1/551
svnadmin: E160004: Corrupt representation '1 551 42 30 (null) e2ca5cc8621b52accf604fbf1ef27144bdf89542 (null)'
svnadmin: E160004: Malformed representation header at r1/db/revs/0/1:556
]]]
Notification will follow someday --- see issue #3958.
Re: svnadmin verify + rep-cache.db
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
+1; we already validate reps given in node-revision headers.
(could you CC me on the issue?)
Stefan Sperling wrote on Wed, Jul 13, 2011 at 15:59:36 +0200:
> Hi all,
>
> while repairing some bad repository breakage we noticed that
> svnadmin verify does not check whether offsets mentioned in
> rep-cache.db point to valid reps.
>
> In some cases I had to append repaired reps after the changed-paths
> section in revision files and leave broken reps in place to avoid
> adjusting offsets everywhere.
>
> So now 'svnadmin verify' runs fine. But when the rep-cache is
> referred to during commit we can still run into reps that are broken.
>
> Obviously we'll either have to fix up the rep-cache as well,
> or simply delete it. But the point is that 'svnadmin verify'
> does not complain.
>
> Would it be feasable to add this as a separate pass of 'svnadmin verify'?
> Should I file an issue about this?
>
> For instance, good output could look like this:
>
> $ svnadmin verify repos
> * Verified revision 1.
> ...
> * Verified revision N.
> * Verified rep-cache.
>
> $
>
> And errors like this:
>
> $ svnadmin verify repos
> * Verified revision 1.
> ...
> * Verified revision N.
> svnamdin: rep-cache entry '8fe47fa166154fc6e7f2e78366591c470ba75cf7' is invalid
> svnadmin: Malformed representation header
>
> $