On Sat, Jul 13, 2002 at 01:11:00AM -0500, jerenkrantz@tigris.org wrote:
> Author: jerenkrantz
> Date: Sat, 13 Jul 2002 01:10:44 -0500
> New Revision: 2504
>
> Modified:
> trunk/subversion/libsvn_client/commit.c
> Log:
> Fix for the first of Pilchie's segfaults.
>
> * subversion/libsvn_client/commit.c
> (svn_client_commit): If we did not have a lock on the current admin
> directory, obtain the lock on it.
This commit fixes the immediate segfault that caused Pilchie's bad wc
state.
There is still a condition where the lock could be held on the
.svn dirs (if we segfaulted somewhere else or exited abnormally),
but we won't test for locks until *after* commits. That can be
recovered by 'svn cleanup' followed by 'svn update'.
However, with this patch, we acquire the locks *before* the commit,
so that we can't make any bogus commits where we can't update the
WC. I really think this patch needs to make it into Alpha or we
will end up with inconsistent WCs.
The only problem I have right now is in svn_wc_process_modified().
This patch attempts to work around it by unlocking directories as
we go so that svn_wc_process_modified() enters with zero locks held.
Ideally, this constraint will be removed and all of the unlock code
can be removed from commit_util.c.
With this patch, 'make check' passes and I can't get the WC in
a bad state. The worst thing I can get it to do is have a stale
lock, and it will refuse to make any commits until 'svn cleanup'
is run. (Right now, it will do the commit even with the lock
held.)
Please test if you can. -- justin
* subversion/libsvn_client/commit_util.c
(lock_dir): Return retrieved/created admin_baton.
(unlock_dir): New function to unlock an associated admin_baton.
(harvest_committables): Take admin_baton parameter. Do not need to
lock the entry if we are adding it to the committables list. Create
a subdir baton or use the current one (if we are dealing with a file)
when we recursively call harvest_committables. Change the pool for
the recursive call to loop_pool instead of subpool.
(svn_client__harvest_committables): Canonicalize the parent directory
upon entry to this function. Obtain the wc admin baton. Pass the
baton to harvest_commitables() function.
(svn_client_get_copy_committables): Ditto except no canonicalization.
Index: subversion/libsvn_client/commit_util.c
===================================================================
--- subversion/libsvn_client/commit_util.c
+++ subversion/libsvn_client/commit_util.c 2002-07-13 00:41:15.000000000 -0700
@@ -53,22 +53,59 @@
allocations. */
static svn_error_t *
lock_dir (apr_hash_t *locked_dirs,
+ svn_wc_adm_access_t **admin_baton,
const char *dir,
apr_pool_t *pool)
{
+ void *value;
apr_pool_t *hash_pool = apr_hash_pool_get (locked_dirs);
- if (! apr_hash_get (locked_dirs, dir, APR_HASH_KEY_STRING))
+ value = apr_hash_get (locked_dirs, dir, APR_HASH_KEY_STRING);
+
+ if (! value )
{
svn_wc_adm_access_t *adm_access;
SVN_ERR (svn_wc_adm_open (&adm_access, dir, TRUE, hash_pool));
apr_hash_set (locked_dirs, apr_pstrdup (hash_pool, dir),
APR_HASH_KEY_STRING, adm_access);
+ value = adm_access;
}
+
+ *admin_baton = (svn_wc_adm_access_t*)value;
+
return SVN_NO_ERROR;
}
+/* Removes ADMIN_BATON from LOCKED_DIRS hash. Closes the ADMIN_BATON. */
+static svn_error_t *
+unlock_dir (apr_hash_t *locked_dirs,
+ svn_wc_adm_access_t *admin_baton,
+ apr_pool_t *pool)
+{
+ apr_hash_index_t *hi;
+
+ /* Find and clear our lock. */
+ /* FIXME: Need a svn_wc_adm_access_t function to retrieve the path. */
+ for (hi = apr_hash_first (pool, locked_dirs); hi; hi = apr_hash_next (hi))
+ {
+ const void *key;
+ apr_ssize_t klen;
+ void *val;
+
+ apr_hash_this (hi, &key, &klen, &val);
+ if (admin_baton == (svn_wc_adm_access_t*)val)
+ {
+ apr_hash_set (locked_dirs, key, klen, NULL);
+ break;
+ }
+ }
+
+ SVN_ERR(svn_wc_adm_close(admin_baton));
+
+ return SVN_NO_ERROR;
+}
+
/* Add a new commit candidate (described by all parameters except
`COMMITTABLES') to the COMMITABLES hash. */
static void
@@ -131,6 +168,7 @@
static svn_error_t *
harvest_committables (apr_hash_t *committables,
apr_hash_t *locked_dirs,
+ svn_wc_adm_access_t *admin_baton,
const char *path,
const char *url,
const char *copyfrom_url,
@@ -290,12 +328,6 @@
/* Now, if this is something to commit, add it to our list. */
if (state_flags)
{
- /* If the commit item is a directory, lock it, else lock its parent. */
- if (entry->kind == svn_node_dir)
- lock_dir (locked_dirs, path, pool);
- else
- lock_dir (locked_dirs, p_path, pool);
-
/* Finally, add the committable item. */
add_committable (committables, path, entry->kind, url,
cf_url ? entry->copyfrom_rev : entry->revision,
@@ -330,6 +362,8 @@
const char *this_cf_url
= cf_url ? apr_pstrdup (loop_pool, cf_url) : NULL;
+ svn_wc_adm_access_t *subdir_admin_baton;
+
/* Get the next entry */
apr_hash_this (hi, &key, &klen, &val);
name = (const char *) key;
@@ -366,9 +400,19 @@
used_url = this_url;
}
+ if (this_entry->kind == svn_node_dir)
+ {
+ SVN_ERR (lock_dir (locked_dirs, &subdir_admin_baton, full_path,
+ loop_pool));
+ }
+ else
+ {
+ subdir_admin_baton = admin_baton;
+ }
+
/* Recurse. */
SVN_ERR (harvest_committables
- (committables, locked_dirs, full_path,
+ (committables, locked_dirs, subdir_admin_baton, full_path,
used_url ? used_url : this_entry->url,
this_cf_url,
(svn_wc_entry_t *)val,
@@ -376,7 +420,14 @@
adds_only,
copy_mode,
FALSE,
- subpool));
+ loop_pool));
+
+ /* FIXME: Temporary hack until svn_wc_process_committed is fixed. */
+ if ( subdir_admin_baton != admin_baton )
+ {
+ SVN_ERR (unlock_dir (locked_dirs, subdir_admin_baton,
+ loop_pool));
+ }
svn_pool_clear (loop_pool);
}
@@ -397,13 +448,20 @@
apr_pool_t *pool)
{
int i = 0;
-
+ svn_wc_adm_access_t *admin_baton;
+
/* Create the COMMITTABLES hash. */
*committables = apr_hash_make (pool);
/* Create the LOCKED_DIRS hash. */
*locked_dirs = apr_hash_make (pool);
+ /* Normalize parent dir to not have /. */
+ parent_dir = svn_path_canonicalize_nts (parent_dir, pool);
+
+ /* Obtain the admin_baton, so we can be cacheable. */
+ SVN_ERR (lock_dir (*locked_dirs, &admin_baton, parent_dir, pool));
+
/* ### Would be nice to use an iteration pool here, but need to
first look into lifetime issues w/ anything passed to
harvest_committables() and possibly stored by it. */
@@ -490,14 +548,17 @@
target);
/* Handle our TARGET. */
- SVN_ERR (harvest_committables (*committables, *locked_dirs, target,
- url, NULL, entry, NULL, FALSE, FALSE,
- nonrecursive, pool));
+ SVN_ERR (harvest_committables (*committables, *locked_dirs, admin_baton,
+ target, url, NULL, entry, NULL, FALSE,
+ FALSE, nonrecursive, pool));
i++;
}
while (i < targets->nelts);
+ /* FIXME: Temporary hack until svn_wc_process_committed is fixed. */
+ SVN_ERR (unlock_dir (*locked_dirs, admin_baton, pool));
+
return SVN_NO_ERROR;
}
@@ -510,6 +571,7 @@
apr_pool_t *pool)
{
svn_wc_entry_t *entry;
+ svn_wc_adm_access_t *admin_baton;
/* Create the COMMITTABLES hash. */
*committables = apr_hash_make (pool);
@@ -517,16 +579,22 @@
/* Create the LOCKED_DIRS hash. */
*locked_dirs = apr_hash_make (pool);
+ SVN_ERR (lock_dir (*locked_dirs, &admin_baton, target, pool));
+
/* Read the entry for TARGET. */
SVN_ERR (svn_wc_entry (&entry, target, FALSE, pool));
+
if (! entry)
return svn_error_create
(SVN_ERR_ENTRY_NOT_FOUND, 0, NULL, pool, target);
/* Handle our TARGET. */
- SVN_ERR (harvest_committables (*committables, *locked_dirs, target,
- new_url, entry->url, entry, NULL,
- FALSE, TRUE, FALSE, pool));
+ SVN_ERR (harvest_committables (*committables, *locked_dirs, admin_baton,
+ target, new_url, entry->url, entry,
+ NULL, FALSE, TRUE, FALSE, pool));
+
+ /* FIXME: Temporary hack until svn_wc_process_committed is fixed. */
+ SVN_ERR (unlock_dir (*locked_dirs, admin_baton, pool));
return SVN_NO_ERROR;
}
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Sat Jul 13 10:26:13 2002