[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: svn commit: r11927 - in branches/locking/subversion: include libsvn_fs_base libsvn_fs_base/bdb

From: Branko Čibej <brane_at_xbc.nu>
Date: 2004-11-21 03:40:07 CET

sussman@tigris.org wrote:

>Author: sussman
>Date: Tue Nov 16 14:25:18 2004
>New Revision: 11927
>
>
[...]

> int
>@@ -55,6 +56,7 @@
> svn_error_t *
> svn_fs_bdb__lock_token_add (svn_fs_t *fs,
> const char *path,
>+ const svn_node_kind_t kind,
>
>
The const here is basically useless in C, because scalars are passed by
value. This signature promises that the implmentation of the function
won't change the parameter value inside the function; but since any such
change can't affect the caller, this is an implmentation detail that's
only confusing in the API.

> svn_error_t *
> svn_fs_bdb__lock_token_delete (svn_fs_t *fs,
> const char *path,
>+ const svn_node_kind_t kind,
>
>
Same here.

> svn_fs_bdb__lock_token_get (const char **lock_token_p,
> svn_fs_t *fs,
> const char *path,
>+ const svn_node_kind_t kind,
>
>
And here.

>+ /* If `locks' doesn't have the lock, then we should lose it too. */
>+ SVN_ERR (svn_fs_bdb__lock_token_delete (fs, path, kind, trail));
>+ return err;
>
>
There's a potential error leak here. If svn_fs_bdb__lock_token_delet
fails, you never clear "err". I'd at least link the two errors together,
otherwise it might seem odd that Joe User said "svn lock" but got back
"can't delete the lock token" or some such...

>+
>+ /* Possibly auto-expire the lock. */
>+ if (lock->expiration_date
>+ && (apr_time_now() > lock->expiration_date))
>+ {
>+ SVN_ERR (svn_fs_bdb__lock_delete (fs, lock_token, trail));
>+ return svn_fs_base__err_lock_expired (fs, lock_token);
>+ }
>
>
There's a similar possible error message weirdness here...

>+ /* Get the first matching key that is either equal or greater
>+ * than the one passed in, by passing in the DB_RANGE_SET flag.
>+ */
>+ db_err = cursor->c_get(cursor, &key, svn_fs_base__result_dbt (&value),
>+ DB_SET_RANGE);
>+
>+ /* As long as the prefix of the returned KEY matches LOOKUP_PATH
>+ * we know it is either LOOKUP_PATH or a decendant thereof.
>+ */
>+ while (! db_err && strncmp(lookup_path, key.data, key.size) != 0)
>
>
I'm trying to grok this... say you looked up "foo/bar/", but got back
"foo/bar/baz". If I read this correctly, you're doing a
strncmp("foo/bar/", "foo/bar/baz", strlen("foo/bar/baz"))... haven't you
got your lengths switced around? Also, "while prefix matches" implies
"strncmp(...) == 0", not "strncmp(...) != 0).

>+ /* Figure out the node_kind of this child path. */
>+ child_kind =
>+ (child_path[key.size - 1] == '/') ? svn_node_dir : svn_node_file;
>+
>+ /* If the child_path has a trailing '/', we need to remove it,
>+ to stay compatible with the rest of the fs library. */
>+ if (child_path[key.size - 1] == '/')
>+ child_path[key.size - 1] = '\0';
>
>
Hm. I thought we're not doing directory locks yet. Which means you can't
get a path with a trailing slask from the tokens table.

>+ /* Make sure the token points to an existing, non-expired lock,
>+ by doing a lookup in the `locks' table. */
>+ err = svn_fs_bdb__lock_get (&lock, fs, lock_token, trail);
>+ if (err && ((err->apr_err == SVN_ERR_FS_LOCK_EXPIRED)
>+ || (err->apr_err == SVN_ERR_FS_BAD_LOCK_TOKEN)))
>+ {
>+ svn_error_clear (err);
>+
>+ /* If `locks' doesn't have the lock, then we should lose it
>+ from `lock-tokens' table as well, then skip to the next
>+ matching path-key. */
>+ SVN_ERR (svn_fs_bdb__lock_token_delete (fs, child_path,
>+ child_kind, trail));
>
>
Potential error leak again.

-- Brane

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Sun Nov 21 03:41:01 2004

This is an archived mail posted to the Subversion Dev mailing list.