Re: svn commit: r12773 - branches/locking/subversion/libsvn_fs_fs

["Peter N. Lundblad" <pe...@famlundblad.se>]

On Mon, 17 Jan 2005 fitz@tigris.org wrote:

> Modified: branches/locking/subversion/libsvn_fs_fs/lock.c
> Url: http://svn.collab.net/viewcvs/svn/branches/locking/subversion/libsvn_fs_fs/lock.c?view=diff&rev=12773&p1=branches/locking/subversion/libsvn_fs_fs/lock.c&r1=12772&p2=branches/locking/subversion/libsvn_fs_fs/lock.c&r2=12773
> ==============================================================================
> --- branches/locking/subversion/libsvn_fs_fs/lock.c	(original)
> +++ branches/locking/subversion/libsvn_fs_fs/lock.c	Mon Jan 17 23:09:32 2005
> @@ -756,6 +756,7 @@
>    svn_lock_t *new_lock;
>    svn_fs_root_t *root;
>    svn_revnum_t youngest;
> +  apr_pool_t *subpool = svn_pool_create (pool);
>
>    SVN_ERR (svn_fs_fs__check_fs (fs));
>
> @@ -795,6 +796,8 @@
>                                    path);
>      }
>
> +  SVN_ERR (svn_fs_fs__get_write_lock (fs, subpool));
> +
>    /* Is the path already locked?
>
>       Note that this next function call will automatically ignore any
> @@ -826,6 +829,9 @@
>    SVN_ERR (write_lock_to_file (fs, new_lock, pool));
>    *lock_p = new_lock;
>
> +  /* Destroy our subpool and release the lock. */

I understand what lock you are referring to, but it is a little confusing
to talk about "the lock" when we are taking a lock and holding a write
lock. Maybe "... release the write lock" instead? Same applies to all
three cases.

> @@ -938,6 +948,7 @@
>                     svn_boolean_t force,
>                     apr_pool_t *pool)
>  {
> +  apr_pool_t *subpool = svn_pool_create (pool);
>    svn_lock_t *existing_lock;
>
>    /* Sanity check:  we don't want to lookup a NULL path. */
> @@ -964,8 +975,12 @@
>                                                 fs->access_ctx->username,
>                                                 existing_lock->owner);
>
> +  SVN_ERR (svn_fs_fs__get_write_lock (fs, subpool));

There is a race condition here. The checks must be done after grabbing the
write lock, so the path doesn't get locked by someone else in between.

Also, but not part of this commit, I don't see where you check the lock
token so it is the token of the actual lock in the filesystem. Did that
get lost when you switched from looking up a lock by token to looking it
up by path?

Regards,
//Peter

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org