On Sun, 5 Dec 2004 fitz@tigris.org wrote:
> Author: fitz
> Date: Sun Dec 5 23:33:18 2004
> New Revision: 12175
>
> 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=12175&p1=branches/locking/subversion/libsvn_fs_fs/lock.c&r1=12174&p2=branches/locking/subversion/libsvn_fs_fs/lock.c&r2=12175
> ==============================================================================
> --- branches/locking/subversion/libsvn_fs_fs/lock.c (original)
> +++ branches/locking/subversion/libsvn_fs_fs/lock.c Sun Dec 5 23:33:18 2004
> @@ -18,15 +18,357 @@
...
> +/* Store the lock in the OS level filesystem in a tree under
> + repos/db/locks/locks that reflects the location of lock->path in
> + the repository. */
> +static svn_error_t *
> +write_lock_to_file (svn_fs_t *fs,
> + svn_lock_t *lock,
> + apr_pool_t *pool)
> +{
> + apr_hash_t *hash;
> + apr_file_t *fd;
> + svn_stream_t *stream;
> + apr_status_t status = APR_SUCCESS;
> + char *abs_path;
> +
> + char *dir;
> + /* ###file and pathnames will be limited by the native filesystem's
> + encoding--could that pose a problem? */
> + abs_path = abs_path_to_lock_file (fs, lock->path, pool);
As we discussed on IRC, this is a problem. I don't think we ompose
pathname limits on the system on which the repository is stored into the
repository. We do here. I proposed using the MD5 of the filename as the
filename.
> +
> + /* Make sure that the directory exists before we create the lock file. */
> + dir = svn_path_dirname (abs_path, pool);
> + status = apr_dir_make_recursive (dir, APR_OS_DEFAULT, pool);
> +
Here, you send UTF8-encoded string to APR, which expects native encoding.
> + if (status)
> + return svn_error_wrap_apr (status, _("Can't create lock directory '%s'"),
> + dir);
> +
> + /* Create our hash and load it up. */
> + hash = apr_hash_make (pool);
> +
> + hash_store (hash, PATH_KEY, lock->path, pool);
> + hash_store (hash, TOKEN_KEY, lock->token, pool);
> + hash_store (hash, OWNER_KEY, lock->owner, pool);
> + hash_store (hash, COMMENT_KEY, lock->comment, pool);
> +
> + hash_store (hash, CREATION_DATE_KEY,
> + svn_time_to_cstring(lock->creation_date, pool), pool);
> + hash_store (hash, EXPIRATION_DATE_KEY,
> + svn_time_to_cstring(lock->expiration_date, pool), pool);
What happens if creation_date is 0? Is that consistent on different
platforms? I suggest just don't setting the key if it is zero.
> + status = apr_file_open (&fd, abs_path, APR_WRITE | APR_CREATE,
> + APR_OS_DEFAULT, pool);
> + if (status && !APR_STATUS_IS_ENOENT (status))
> + return svn_error_wrap_apr (status, _("Can't open '%s' to write lock"),
> + abs_path);
> +
What about ENOENT here? Why should it succeed then?
> +
> +static svn_error_t *
> +read_lock_from_file (svn_lock_t **lock_p,
> + svn_fs_t *fs,
> + const char *path,
> + apr_pool_t *pool)
> +{
> + svn_lock_t *lock;
> + apr_hash_t *hash;
> + apr_file_t *fd;
> + svn_stream_t *stream;
> + apr_status_t status;
> + char *abs_path;
> + const char *val;
> + apr_finfo_t finfo;
> +
> + abs_path = abs_path_to_lock_file(fs, path, pool);
> +
> + status = apr_stat (&finfo, abs_path, APR_FINFO_TYPE, pool);
> + /* If file doesn't exist, then there's no lock, so return immediately. */
> + if (APR_STATUS_IS_ENOENT (status))
> + {
> + *lock_p = NULL;
> + return svn_fs_fs__err_no_such_lock (fs, path);
> + }
> +
> + /* ###Is this necessary? */
> + if (status && !APR_STATUS_IS_ENOENT (status))
> + return svn_error_wrap_apr (status, _("Can't stat '%s'"), abs_path);
> +
What's the point in stating the file just before opening it?
> + status = apr_file_open (&fd, abs_path, APR_READ, APR_OS_DEFAULT, pool);
Encoding mix again.
> + if (status)
> + return svn_error_wrap_apr (status, _("Can't open '%s' to read lock"),
> + abs_path);
> +
> + hash = apr_hash_make (pool);
> +
> + stream = svn_stream_from_aprfile(fd, pool);
> + SVN_ERR_W (svn_hash_read2 (hash, stream, SVN_HASH_TERMINATOR, pool),
> + apr_psprintf (pool, _("Can't parse '%s'"), abs_path));
> +
> + /* Create our lock and load it up. */
> + lock = apr_palloc (pool, sizeof (*lock));
Hmmm, apr_palloc means uninitialized memory, right?
> +
> + val = hash_fetch (hash, PATH_KEY, pool);
> + if (val)
> + lock->path = val;
> +
Else, corrupt lock file. Need to check, else API violations higher up.
> + val = hash_fetch (hash, TOKEN_KEY, pool);
> + if (val)
> + lock->token = val;
> +
Same.
> + val = hash_fetch (hash, OWNER_KEY, pool);
> + if (val)
> + lock->owner = val;
Same.
> +
> + val = hash_fetch (hash, COMMENT_KEY, pool);
> + if (val)
> + lock->comment = val;
And here.
> +
> + val = hash_fetch (hash, CREATION_DATE_KEY, pool);
> + if (val)
> + svn_time_from_cstring (&(lock->creation_date), val, pool);
Error leak/missing check.
> +
> + val = hash_fetch (hash, EXPIRATION_DATE_KEY, pool);
> + if (val)
> + svn_time_from_cstring (&(lock->expiration_date), val, pool);
> +
Same.
Note that if a value is missing above, the field is uninitialized. This is
important, especially in the last case, where a mising value is OK.
> + *lock_p = lock;
> +
> + return SVN_NO_ERROR;
> +}
> +
> +
A general comment. I think you need the commit write-lock when creating a
lock to guarantee atomicity. Won't you need that anyway in the final
commit to check for broken/stolen locks? For example, after you ensured
that the lock directories exist, but before you create the lock file,
someone else could unlock the last file in that directory and prune. If
you move lock files in place atomically, I don't think you need the write
lock to get a lock. The same applies for the lock token file. ghudson
might have more informed comments to add here, ghough.
Regards,
//Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Mon Dec 6 21:06:38 2004