Re: [Patch] Fix multiple reporting of the same lock in FSFS.
Sergey Raevskiy sergey.raevs...@visualsvn.com writes: Follow-up to r1658482: Refactor walk_locks() function: remove the redundant complexity of walk_digest_files() / walk_digests_callback_t. Committed in r1659217. Thanks! -- Philip Martin | Subversion Committer WANdisco // *Non-Stop Data*
Re: [Patch] Fix multiple reporting of the same lock in FSFS.
I suspect the implementation is now more complicated than necessary.
walk_locks and walk_locks_baton could be removed, walk_digest_files
could be renamed to indicate that only a single digest file is accessed.
The callers of walk_locks would call the renamed function directly.
Yes. I'm going to look into this and maybe will come up with a new patch.
I preferred to keep the walk_locks() function and inline walk_digest_files()
and locks_walker() instead. The patch is attached.
Log message:
[[[
Follow-up to r1658482: Refactor walk_locks() function: remove the redundant
complexity of walk_digest_files() / walk_digests_callback_t.
* subversion/libsvn_fs_fs/lock.c
(lock_expired): New helper function.
(get_lock): Use the new function.
(struct walk_locks_baton,
walk_digests_callback_t,
locks_walker,
walk_digest_files): Remove.
(walk_locks): Inline code from locks_walker() and walk_digest_files() with
use of new helper function.
Patch by: sergey.raevskiy{_AT_}visualsvn.com
]]]
Index: subversion/libsvn_fs_fs/lock.c
===
--- subversion/libsvn_fs_fs/lock.c (revision 1658488)
+++ subversion/libsvn_fs_fs/lock.c (working copy)
@@ -451,6 +451,12 @@ unlock_single(svn_fs_t *fs,
svn_lock_t *lock,
apr_pool_t *pool);
+/* Check if LOCK has been already expired. */
+static svn_boolean_t lock_expired(const svn_lock_t *lock)
+{
+ return lock-expiration_date (apr_time_now() lock-expiration_date);
+}
+
/* Set *LOCK_P to the lock for PATH in FS. HAVE_WRITE_LOCK should be
TRUE if the caller (or one of its callers) has taken out the
repository-wide write lock, FALSE otherwise. If MUST_EXIST is
@@ -480,7 +486,7 @@ get_lock(svn_lock_t **lock_p,
return must_exist ? SVN_FS__ERR_NO_SUCH_LOCK(fs, path) : SVN_NO_ERROR;
/* Don't return an expired lock. */
- if (lock-expiration_date (apr_time_now() lock-expiration_date))
+ if (lock_expired(lock))
{
/* Only remove the lock if we have the write lock.
Read operations shouldn't change the filesystem. */
@@ -527,67 +533,17 @@ get_lock_helper(svn_fs_t *fs,
}
-/* Baton for locks_walker(). */
-struct walk_locks_baton {
- svn_fs_get_locks_callback_t get_locks_func;
- void *get_locks_baton;
- svn_fs_t *fs;
-};
-
-/* Implements walk_digests_callback_t. */
-static svn_error_t *
-locks_walker(void *baton,
- const char *fs_path,
- const char *digest_path,
- svn_lock_t *lock,
- svn_boolean_t have_write_lock,
- apr_pool_t *pool)
-{
- struct walk_locks_baton *wlb = baton;
-
- if (lock)
-{
- /* Don't report an expired lock. */
- if (lock-expiration_date == 0
- || (apr_time_now() = lock-expiration_date))
-{
- if (wlb-get_locks_func)
-SVN_ERR(wlb-get_locks_func(wlb-get_locks_baton, lock, pool));
-}
- else
-{
- /* Only remove the lock if we have the write lock.
- Read operations shouldn't change the filesystem. */
- if (have_write_lock)
-SVN_ERR(unlock_single(wlb-fs, lock, pool));
-}
-}
-
- return SVN_NO_ERROR;
-}
-
-/* Callback type for walk_digest_files().
- *
- * LOCK come from a read_digest_file(digest_path) call.
- */
-typedef svn_error_t *(*walk_digests_callback_t)(void *baton,
-const char *fs_path,
-const char *digest_path,
-svn_lock_t *lock,
-svn_boolean_t have_write_lock,
-apr_pool_t *pool);
-
-/* A function that calls WALK_DIGESTS_FUNC/WALK_DIGESTS_BATON for
- all lock digest files in and under PATH in FS.
+/* A function that calls GET_LOCKS_FUNC/GET_LOCKS_BATON for
+ all locks in and under PATH in FS.
HAVE_WRITE_LOCK should be true if the caller (directly or indirectly)
has the FS write lock. */
static svn_error_t *
-walk_digest_files(const char *fs_path,
- const char *digest_path,
- walk_digests_callback_t walk_digests_func,
- void *walk_digests_baton,
- svn_boolean_t have_write_lock,
- apr_pool_t *pool)
+walk_locks(svn_fs_t *fs,
+ const char *digest_path,
+ svn_fs_get_locks_callback_t get_locks_func,
+ void *get_locks_baton,
+ svn_boolean_t have_write_lock,
+ apr_pool_t *pool)
{
apr_hash_index_t *hi;
apr_hash_t *children;
@@ -595,10 +551,19 @@ static svn_error_t *
svn_lock_t *lock;
/* First, send up any locks in the current digest file. */
- SVN_ERR(read_digest_file(children, lock, fs_path, digest_path, pool));
+ SVN_ERR(read_digest_file(children, lock, fs-path, digest_path, pool));
-
Re: [Patch] Fix multiple reporting of the same lock in FSFS.
This is issue 2507
http://subversion.tigris.org/issues/show_bug.cgi?id=2507
Commit does not remove locks, lock removal is a separate step after
commit. However that doesn't work well when a commit removes a locked
item, after such a commit the remaining lock is invalid and should not
apply.
This is another issue. My initial post is about a bug in FSFS that could lead
to unexpected behavior of svn_fs_get_locks() function. What I'm talking about
is:
However, the walk_digest_files() function wasn't updated to reflect the
changes from [1] and still iterates the locks in a 'recursive' way. I
suggest removing recursion from this function, since it can produce
unexpected results for the caller. I've attached a patch with the test
and the fix for this issue.
To reproduce this bug in test, I've emulated nested locks by creating a file
('/A'), locking it and then replacing by directory with same name. This looks
like a 'directory lock', and I am trying to say that if this happens in real
life, the FS API would behave incorrectly.
What happens if you remove /A and resinstate /A as a file, does the lock
come back?
Yes.
Re: [Patch] Fix multiple reporting of the same lock in FSFS.
Sergey Raevskiy sergey.raevs...@visualsvn.com writes: + /* Make a file '/A'. */ + SVN_ERR(svn_fs_begin_txn(txn, fs, 0, pool)); + SVN_ERR(svn_fs_txn_root(root, txn, pool)); + SVN_ERR(svn_fs_make_file(root, /A, pool)); + SVN_ERR(svn_fs_commit_txn(conflict, newrev, txn, pool)); + + /* Obtain a lock on '/A'. */ + SVN_ERR(svn_fs_lock(lock, fs, /A, NULL, NULL, FALSE, 0, newrev, FALSE, + pool)); + + /* Add a lock token to FS access context. */ + SVN_ERR(svn_fs_access_add_lock_token(access, lock-token)); + + /* Make some weird change: replace file '/A' by a directory with a child. */ + SVN_ERR(svn_fs_begin_txn(txn, fs, newrev, pool)); + SVN_ERR(svn_fs_txn_root(root, txn, pool)); + SVN_ERR(svn_fs_delete(root, /A, pool)); + SVN_ERR(svn_fs_make_dir(root, /A, pool)); + SVN_ERR(svn_fs_make_file(root, /A/b, pool)); + SVN_ERR(svn_fs_commit_txn(conflict, newrev, txn, pool)); + + /* Obtain a lock on '/A/b'. */ + SVN_ERR(svn_fs_lock(lock, fs, /A/b, NULL, NULL, FALSE, 0, newrev, FALSE, + pool)); This is issue 2507 http://subversion.tigris.org/issues/show_bug.cgi?id=2507 Commit does not remove locks, lock removal is a separate step after commit. However that doesn't work well when a commit removes a locked item, after such a commit the remaining lock is invalid and should not apply. What happens if you remove /A and resinstate /A as a file, does the lock come back? -- Philip Martin | Subversion Committer WANdisco // *Non-Stop Data*
Re: [Patch] Fix multiple reporting of the same lock in FSFS.
I suspect the implementation is now more complicated than necessary. walk_locks and walk_locks_baton could be removed, walk_digest_files could be renamed to indicate that only a single digest file is accessed. The callers of walk_locks would call the renamed function directly. Yes. I'm going to look into this and maybe will come up with a new patch.
Re: [Patch] Fix multiple reporting of the same lock in FSFS.
Sergey Raevskiy sergey.raevs...@visualsvn.com writes:
To reproduce this bug in test, I've emulated nested locks by creating a file
('/A'), locking it and then replacing by directory with same name. This looks
like a 'directory lock', and I am trying to say that if this happens in real
life, the FS API would behave incorrectly.
I added a few extra comments to the test and committed, thanks!
I suspect the implementation is now more complicated than necessary.
walk_locks and walk_locks_baton could be removed, walk_digest_files
could be renamed to indicate that only a single digest file is accessed.
The callers of walk_locks would call the renamed function directly.
--
Philip Martin | Subversion Committer
WANdisco // *Non-Stop Data*
