<quote who="Philip Martin">
> Vladimir Berezniker <vmpn@tigris.org> writes:
[snip]
> I agree with Julian, "archive" is not a good term for this behaviour.
Could the option be called --clean-logs or --cleanup-logs? Because to
clean means to get rid of unwanted matter. In this case copied, unused
logs.
>
>> * subversion/include/svn_error.h
>> (SVN_ERR_POOL): Implemented macro to handle subversion errors in
code that uses subpools.
>
> Why is this necessary? If an error is returned then the place that
handles the error is responsible for clearing the pool, this should take
care of any subpools.
>
I felt that since the subpool is allocated by the function, it should be
destroyed but the time it is complete. Behavior of the caller is not
known. Caller is not required to clear its pool if it receives an error.
Caller could handle the returned error gracefully and continues execution
without destroying its pool. As a result there will be memory allocated
but not used.
[snip body of (parse_local_repos_path)]
> Static functions should have a documentation string describing the
function and the parameters.
>
Fixed in the upcoming patch.
[snip]
>> @@ -229,6 +270,7 @@
>> struct svnadmin_opt_state
>> {
>> const char *repository_path;
>> + const char *new_repository_path;
>
> Odd whitespace, this appears in quite a few places.
I just checked my original email and it seems to have correct whitespace
when I view it. Maybe its my email client. I'll look more into this.
[snip]
>
> I don't like svn_pool_clear in the for(), I'd prefer a separate line as
per the HACKING file.
>
Fixed.
>> + {
>
> The { should be indented, this comment applies in quite a few places.
>
Fixed. I did not even realize this and the other whitespace mistakes I
made. Thank you.
>
>> +
>> + { /* Compare files. No point in using MD5 and waisting CPU
cycles
>> as we
>> + got full copies of both logs */
>> + svn_boolean_t files_match = FALSE;
>> + svn_error_t * err = svn_io_files_contents_same_p(&files_match,
+
live_log_path,
>> +
>> backup_log_path,
>> + sub_pool); +
if (err) {
>> + svn_error_clear(err);
>> + files_match = FALSE;
>
> I'd like a bit of explanation here. Why is it acceptable to ignore
errors?
A case when unused log cannot be compared to its copy did not seem to
qualify as fatal error. A recovery is executed on a copy to verify that it
is working state. Main case was when there is a new unused log after the
logs were copied, but before log cleaning is done. I beleive in this case
an error will be returned indicating that one of the files is missing.
Maybe a warrning should be issued to a client in such case? How would one
do that?
Here is the comment from the updated patch:
/* If for some reason we failed to compare log files.
Say file was not copied or unreadable, behave as if log file
was not copied and keep it. */
[snip code performing unused log deletion]
> Does this assume that the log files are always located directly within
the db/ directory? BDB has set_lg_dir that allows a DB admin to change that.
[snip code doing database and log copying]
> This appears to assume that the data files are located directly in the
db/ dir. BDB has set_data_dir that allows the DB admin to change the
location.
The answer to both questions is *yes*. However, the code is very easy to
change to accomodate custom directories. There is one *catch* though.
BDB does not seem to have either get_lg_dir or get_data_dir, so there is
no way for the code to know where the logs and data are. The only way I
see possible to make it more flexible is introduce a subversion config
file inside repository to store setting for the two locations and modify
libsvn_fs to call set_lg_dir and set_data_dir. Please correct me if I am
wrong about this.
[snip badly idented code]
> The { should be indented.
Fixed.
[snip]
>> +svn_error_t *svn_io_file_create (const char *file,
>> + const char *contents,
>> + apr_pool_t *pool)
>> +{
>> + apr_status_t apr_err;
>> + apr_file_t *f = NULL;
>
> Why is f initialised?
>
For no good reason. Fixed.
[snip a lot of inconsistently idented code]
Fixed.
>> +
>> +static svn_error_t *hotcopy_structure (void *baton,
>> + const char *path,
>> + const apr_finfo_t *finfo, +
apr_pool_t *pool)
>
> Could use a documentation comment for this function.
>
Fixed.
>> +{
>> + const struct hotcopy_ctx_t *ctx = ((struct hotcopy_ctx_t *) baton);
+ const char *sub_path;
>> + const char *target;
>> + svn_boolean_t fs_dir = FALSE;
>> +
>> + if(strlen(path) == ctx->src_len)
>> + {
>> + sub_path = &"";
>
> Huh? Do you mean sub_path = ""? (I have to guess since path isn't
documented ;)
>
Is there any difference between sub_path = "" and sub_path = &""? Since
sub_path is a pointer are the two statements not equivalent?
>> +
>> +
>> +static svn_error_t *
>> +lock_db_logs_file (svn_
repos_t *repos,
>> + svn_boolean_t exclusive,
>> + apr_pool_t *
pool)
>> +{
>> + const char * lock_file = svn_repos_db_logs_lockfile(repos, pool); +
svn_node_kind_t kind;
>> + SVN_ERR(svn_io_check_path(lock_file, &kind, pool));
>> +
>> + /* Try to create a lock file if it is missing.
>> + Ignore creation errors just in case file got created by another
process
>> + while we were checking. */
>> + if(kind == svn_node_none) {
>> + svn_error_t * err = create_db_logs_lock(repos, pool);
>> + if(err) {
>> + svn_error_clear(err);
>> + }
>
> To ignore an error unconditionally use
>
> svn_error_clear (create_db_logs_lock (...));
Thank you.
>
> There is a race between svn_io_check_path and create_db_logs
> _lock.
> Since you are ignoring errors, why bother with svn_io_check_path at all?
>
>> + }
>> +
>> + SVN_ERR(svn_io_file_lock(lock_file, exclusive, pool));
>> +
>> + return SVN_NO_ERROR;
>> +}
The db logs lock file will get created once and only for repositories
created before this patch. So I threw in an optimization that will stop
the application from recreating an existing lock file every time its run.
In case of a race when lock file is missing, one of the processes will
create a lock file and the other(s) will lock it. I do not see anything
bad happening there.
>
>> +
>> +
>> +/* Make a copy of a repository with hot backup of fs. */
>> +svn_error_t *
>> +svn_repos_hotcopy (const char *src_path,
>> + const char *dst_path,
>> + svn_boolean_t archive_logs,
>> + apr_pool_t *pool)
>> +{
>> + svn_repos_t *src_repos = NULL;
>> + svn_repos_t *dst_repos = NULL;
>
> Do these need to be initialised?
No. Fixed.
>
[snip]
>> +
>> + /* Allocate a repository object. */
>> + dst_repos = apr
_pcalloc (pool, sizeof (*dst_repos));
>
> Comment is redundant.
>
>> +
>> + /* Initialize the repository paths. */
>> + init_repos_dirs (dst_repos, dst_path, pool);
>
> Comment is redundant.
>
Fixed. Since I swipped that code from (get_repos), should I remove those
comments there too?
>> +
>> + /* Create the lock directory. */
>> + SVN_ERR (create_locks (dst_repos, pool));
>
> Comment is redundant.
>
Fixed.
>> +
>> + SVN_ERR (create_repos_dir(dst_repos->db_path, pool));
>> + /* Reopen repository the right way. Since above is just work around
+ util we can create locks without an open repository */
>> +
>> + /* Exclusevly lock the new repository.
>
> Typo: "Exclusively".
Oops. Spelling was never my strong suit. :) Fixed.
>
[snip more badly formated code]
>
> More odd whitespace. I don't think we use "SVN_ERR ( foo" anywhere in
the code.
>
Fixed.
Thank you for your feedback. I will be sending out the updated patch,
shortly.
Sincerely,
Vladimir Berezniker
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Tue Sep 9 23:41:42 2003