On Sun, Sep 19, 2010 at 10:29:58AM +0100, Daniel Shahaf wrote:
> artag...@apache.org wrote on Sat, Sep 18, 2010 at 17:54:50 -0000:
> > Author: artagnon
> > Date: Sat Sep 18 17:54:50 2010
> > New Revision: 998502
> >
> > URL: http://svn.apache.org/viewvc?rev=998502&view=rev
> > Log:
> > * subversion/svnrdump/load_editor.c: Attempt to acquire a lock of
> > sorts before attempting to load. This also ensures that
> > pre-revprop-change is enabled before it's too late.
> > +static svn_error_t *
> > +get_lock(svn_ra_session_t *session, apr_pool_t *pool)
> > +{
> > + char hostname_str[APRMAXHOSTLEN + 1] = { 0 };
> > + svn_string_t *mylocktoken, *reposlocktoken;
> > + apr_status_t apr_err;
> > + apr_pool_t *subpool;
> > + int i;
> > +
> > + apr_err = apr_gethostname(hostname_str, sizeof(hostname_str), pool);
> > + if (apr_err)
> > + return svn_error_wrap_apr(apr_err, _("Can't get local hostname"));
> > +
> > + mylocktoken = svn_string_createf(pool, "%s:%s", hostname_str,
> > + svn_uuid_generate(pool));
> > +
> > + subpool = svn_pool_create(pool);
> > +
> > + for (i = 0; i < LOCK_RETRIES; ++i)
> > + {
> > + svn_pool_clear(subpool);
> > +
> > + SVN_ERR(svn_ra_rev_prop(session, 0, SVNRDUMP_PROP_LOCK,
> > &reposlocktoken,
> > + subpool));
> > +
> > + if (reposlocktoken)
> > + {
> > + /* Did we get it? If so, we're done, otherwise we sleep. */
> > + if (strcmp(reposlocktoken->data, mylocktoken->data) == 0)
> > + return SVN_NO_ERROR;
> > + else
> > + {
> > + SVN_ERR(svn_cmdline_printf
> > + (pool, _("Failed to get lock on destination "
> > + "repos, currently held by '%s'\n"),
> > + reposlocktoken->data));
> > +
> > + apr_sleep(apr_time_from_sec(1));
> > + }
> > + }
> > + else if (i < LOCK_RETRIES - 1)
> > + {
> > + /* Except in the very last iteration, try to set the lock. */
> > + SVN_ERR(svn_ra_change_rev_prop(session, 0, SVNRDUMP_PROP_LOCK,
> > + mylocktoken, subpool));
> > + }
> > + }
> > +
> > + return svn_error_createf(APR_EINVAL, NULL,
> > + _("Couldn't get lock on destination repos "
> > + "after %d attempts"), i);
> > +}
>
> 1. Please don't duplicate code.
I think it's fine for svnrdump to have its own copy of this for now.
We could at some point merge them into libsvn_subr/ of course, but I
don't see a huge problem with just 2 instances.
> 2. If you do duplicate code, then add big comments (in all instances of
> the duplication) pointing to the other instances.
+1
> 3. Incidentally, I have modified the svnsync instance of this function
> on the atomic-revprops branch. So the desire to avoid duplication isn't
> just theoretical in this case...
We should fix the race in svnrdump on the atomic-revprop branch as well.
Stefan
