djh@tigris.org writes:
> Log:
> Fix hotcopy command's handling of the db/format file and the locks table.
>
> In BDB repositories, the format file was never being copied. In FSFS
> repositories, the format file was always being copied which would
> fail on old repositories where that file did not exist.
>
> Additionally, the locks table was never being copied in FSFS
> repositories.
>
> * subversion/libsvn_fs_base/fs.c
> (base_hotcopy): Copy the db/format file if it exists.
>
> * subversion/libsvn_fs_fs/fs_fs.c
> (copy_locks_table): New function to copy the locks table while holding
> the repository write lock.
> (svn_fs_fs__hotcopy): Copy the db/format file and locks table if they
> exist.
>
> * subversion/tests/clients/cmdline/svnadmin_tests.py
> (hotcopy_format): New test.
>
> --- trunk/subversion/libsvn_fs_base/fs.c (original)
> +++ trunk/subversion/libsvn_fs_base/fs.c Tue May 10 17:03:26 2005
> @@ -1126,6 +1126,8 @@
> svn_error_t *err;
> u_int32_t pagesize;
> svn_boolean_t log_autoremove = FALSE;
> + svn_node_kind_t kind;
> + const char* format_path;
We usually put the star rightwards: "const char *format_path".
(Just for next time; not advocating a new commit to fix it.)
> /* If using DB 4.2 or later, note whether the DB_LOG_AUTOREMOVE
> feature is on. If it is, we have a potential race condition:
> @@ -1136,6 +1138,13 @@
> SVN_ERR (check_env_flags (&log_autoremove, DB_LOG_AUTOREMOVE,
> src_path, pool));
> #endif
> +
> + /* Copy the format file if it exists -- it is assumed to be format 1
> + if it does not exist. */
> + format_path = svn_path_join (src_path, FORMAT_FILE, pool);
> + SVN_ERR (svn_io_check_path (format_path, &kind, pool));
> + if ( kind == svn_node_file )
> + SVN_ERR (svn_io_dir_file_copy (src_path, dest_path, FORMAT_FILE, pool));
Another minor style nit: we don't use the extra spaces in the
condition, just do:
if (kind == svn_node_file)
> --- trunk/subversion/libsvn_fs_fs/fs_fs.c (original)
> +++ trunk/subversion/libsvn_fs_fs/fs_fs.c Tue May 10 17:03:26 2005
> @@ -62,6 +62,7 @@
> #define PATH_REVS_DIR "revs" /* Directory of revisions */
> #define PATH_REVPROPS_DIR "revprops" /* Directory of revprops */
> #define PATH_TXNS_DIR "transactions" /* Directory of transactions */
> +#define PATH_LOCKS_DIR "locks" /* Directory of locks */
>
> /* Names of special files and file extensions for transactions */
> #define PATH_CHANGES "changes" /* Records changes made so far */
> @@ -338,17 +339,45 @@
> return SVN_NO_ERROR;
> }
>
> +/* This is a baton struct for copy_locks_table used by hotcopy */
> +struct copy_locks_baton
> +{
> + const char *src_path;
> + const char *dst_path;
> +};
> +
> +/* Copy the locks table for the hotcopy command if it exists. */
> +static svn_error_t *copy_locks_table (void *baton,
> + apr_pool_t *pool)
In function definitions, newline after the star, so that
"copy_locks_table" starts a new line. Ideally, the function's doc
string should specify the baton's type, but since they're so close
together it's really not a big deal here IMHO.
This doc string is also a little weird because it sounds like it's
questioning the existence of the hotcopy command, rather than the
existence of the locks table. Might want to rearrange it to lose that
ambiguity. Not that anyone would really get thrown by it, but they
might do a double-take.
Speaking of which, are you sure you want to call it a "locks_table"?
I mean, maybe it has some of the semantics of a table, sort of, but
it's really a tree, and after all we are talking about backing it up
here, not about using it as a table.
> +{
> + const char *src_subdir;
> + svn_node_kind_t kind;
> + struct copy_locks_baton *clb = (struct copy_locks_baton *) baton;
> + src_subdir = svn_path_join (clb->src_path, PATH_LOCKS_DIR, pool);
> + SVN_ERR (svn_io_check_path (src_subdir, &kind, pool));
> + if ( kind == svn_node_dir )
> + SVN_ERR (svn_io_copy_dir_recursively (src_subdir, clb->dst_path,
> + PATH_LOCKS_DIR, TRUE, NULL,
> + NULL, pool));
> + return SVN_NO_ERROR;
> +}
Same about the spaces in the condition. (But nice catch, remembering
to check for the src dir's existence!)
> svn_error_t *
> svn_fs_fs__hotcopy (const char *src_path,
> const char *dst_path,
> apr_pool_t *pool)
> {
> - const char *src_subdir, *dst_subdir;
> + const char *src_subdir, *dst_subdir, *format_path;
> svn_revnum_t youngest, rev;
> apr_pool_t *iterpool;
> + svn_node_kind_t kind;
>
> - /* Copy the format file. */
> - SVN_ERR (svn_io_dir_file_copy (src_path, dst_path, PATH_FORMAT, pool));
> + /* Copy the format file if it exists -- it is assumed to be format 1
> + if it does not exist. */
> + format_path = svn_path_join (src_path, PATH_FORMAT, pool);
> + SVN_ERR (svn_io_check_path (format_path, &kind, pool));
> + if ( kind == svn_node_file )
> + SVN_ERR (svn_io_dir_file_copy (src_path, dst_path, PATH_FORMAT, pool));
Blah blah spaces blah :-).
> /* Copy the current file. */
> SVN_ERR (svn_io_dir_file_copy (src_path, dst_path, PATH_CURRENT, pool));
> @@ -396,9 +425,20 @@
> dst_subdir = svn_path_join (dst_path, PATH_TXNS_DIR, pool);
> SVN_ERR (svn_io_make_dir_recursively (dst_subdir, pool));
>
> + /* Now copy the locks table -- we need the repo's exclusive lock
> + to do this safely. */
> + {
> + svn_fs_t *fs;
> + struct copy_locks_baton clb;
> +
> + clb.src_path = src_path;
> + clb.dst_path = dst_path;
> + SVN_ERR (svn_fs_open (&fs, src_path, NULL, pool));
> + SVN_ERR (svn_fs_fs__with_write_lock (fs, copy_locks_table, &clb, pool));
> + }
> +
> return SVN_NO_ERROR;
> }
This is very interesting, that we need the (exclusive) write lock to
copy the locks tree. Nothing else in svn_fs_fs__hotcopy requires the
write lock, and I wonder if that fact shouldn't be considered part of
the design requirements -- or at least the design recommendations.
Is the write lock absolutely necessary? Or, if it is currently
necessary, might there be some way we can change the way locks are
written out, such that we can lose the need for the write lock here?
Bleargh -- too many meanings of the word "lock" running around, but
hopefully the above was clear.
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Wed May 11 06:30:51 2005