You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Philip Martin <ph...@codematters.co.uk> on 2005/01/11 22:27:23 UTC
Re: svn commit: r12676 - branches/locking/subversion/libsvn_fs_fs
fitz@tigris.org writes:
> Author: fitz
> Date: Tue Jan 11 14:15:26 2005
> New Revision: 12676
> --- branches/locking/subversion/libsvn_fs_fs/lock.c (original)
> +++ branches/locking/subversion/libsvn_fs_fs/lock.c Tue Jan 11 14:15:26 2005
> @@ -23,10 +23,12 @@
> #include "svn_hash.h"
> #include "svn_time.h"
> #include "svn_utf.h"
> +#include "svn_md5.h"
>
> #include "apr_uuid.h"
> #include "apr_file_io.h"
> #include "apr_file_info.h"
> +#include "apr_md5.h"
>
> #include "lock.h"
> #include "tree.h"
> @@ -69,14 +71,47 @@
> }
>
No docstring.
> static svn_error_t *
> +abs_path_to_lock_digest_file (char **abs_path,
> + svn_fs_t *fs,
> + const char *digest,
> + apr_pool_t *pool)
> +{
> + SVN_ERR (merge_paths (abs_path, fs->path, LOCK_ROOT_DIR, pool));
> + SVN_ERR (merge_paths (abs_path, *abs_path, LOCK_LOCK_DIR, pool));
> + /* ###TODO create a 1 or 2 char subdir to spread the love across
> + many directories */
> + SVN_ERR (merge_paths (abs_path, *abs_path, digest, pool));
> +
> + return SVN_NO_ERROR;
> +}
> +
> +
ditto.
> +static const char *
> +make_digest (const char *str,
> + apr_pool_t *pool)
> +{
> + unsigned char digest[APR_MD5_DIGESTSIZE];
> +
> + apr_md5 (digest, str, strlen(str));
> + return svn_md5_digest_to_cstring (digest, pool);
> +}
> +
ditto (although not strictly caused by this commit).
> +static svn_error_t *
> abs_path_to_lock_file (char **abs_path,
> svn_fs_t *fs,
> const char *rel_path,
> apr_pool_t *pool)
> {
> + const char *digest_cstring;
> +
> SVN_ERR (merge_paths (abs_path, fs->path, LOCK_ROOT_DIR, pool));
> SVN_ERR (merge_paths (abs_path, *abs_path, LOCK_LOCK_DIR, pool));
> - SVN_ERR (merge_paths (abs_path, *abs_path, (char *)rel_path, pool));
> +
> + digest_cstring = make_digest (rel_path, pool);
> +
> + /* ###TODO create a 1 or 2 char subdir to spread the love across
> + many directories */
> + SVN_ERR (merge_paths (abs_path, *abs_path, digest_cstring, pool));
>
> return SVN_NO_ERROR;
> }
> @@ -88,6 +123,8 @@
> {
> SVN_ERR (merge_paths (base_path, fs->path, LOCK_ROOT_DIR, pool));
> SVN_ERR (merge_paths (base_path, *base_path, LOCK_LOCK_DIR, pool));
> + /* ###TODO create a 1 or 2 char subdir to spread the love across
> + many directories (and take the hash as an optional arg. */
>
> return SVN_NO_ERROR;
> }
> @@ -134,10 +171,177 @@
> return NULL;
> }
>
> +/* Write each hash in ENTRIES to path, one hash per line. */
> +static svn_error_t *
> +write_entries_file (apr_hash_t *entries,
> + char *path,
> + apr_pool_t *pool)
> +{
> + apr_file_t *fd;
> + apr_hash_index_t *hi;
> + char *digest, *content;
> + int status;
> +
> + status = apr_file_open (&fd, path, APR_WRITE | APR_TRUNCATE,
> + APR_OS_DEFAULT, pool);
>
> -/* 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. */
> + if (status)
> + return svn_error_wrap_apr (status,
> + _("Can't open '%s' to write lock entries."),
> + path);
> +
> + for (hi = apr_hash_first(pool, entries); hi; hi = apr_hash_next(hi))
> + {
> + apr_hash_this(hi, (void *)&digest, NULL, NULL);
Look at other uses of apr_hash_this, use temporary variables to avoid
that cast.
> + content = apr_pstrcat (pool, digest, "\n", NULL);
> + svn_io_file_write_full (fd, content, strlen(content), NULL, pool);
If you want to ignore svn_error_t errors then use svn_error_clear, but
I don't think you should be ignoring write errors.
> + }
> + apr_file_close (fd);
You should not ignore the error from apr_file_close, particularly when
writing.
> + return SVN_NO_ERROR;
Yeah, but only because you ignored all the errors!
> +}
> +
> +/* Read lock entries file at PATH, adding each child hash as a key in
> + ENTRIES. If FD is non-NULL, read entries from FD instead of
> + PATH. FD will be closed before returning. */
It also creates *ENTRIES. Perhaps mention the C type of the
keys/values.
> +static svn_error_t *
> +read_entries_file (apr_hash_t **entries,
> + const char *path,
> + apr_file_t *fd,
> + apr_pool_t *pool)
> +{
> + apr_status_t status;
> + apr_size_t buf_len = (APR_MD5_DIGESTSIZE * 2) + 1; /* Add room for "\n". */
> +
> + *entries = apr_hash_make (pool);
> +
> + if (!fd) /* Only open PATH if fd is NULL. */
> + {
> + status = apr_file_open (&fd, path, APR_READ, APR_OS_DEFAULT, pool);
Odd indentation.
> +
> + /* This happens when attempting to open the entries for "/" and it's
> + been deleted. */
> + if (APR_STATUS_IS_ENOENT (status))
> + return SVN_NO_ERROR;
> + if (status)
> + return svn_error_wrap_apr (status,
> + _("Can't open '%s' to read lock entries."),
> + path);
> + }
> +
> + while (apr_file_eof (fd) != APR_EOF)
> + {
> + apr_size_t nbytes = buf_len;
> + char *buf = apr_palloc (pool, buf_len);
> + status = apr_file_read (fd, buf, &nbytes);
> +
> + if (nbytes == 0)
> + continue;
It strikes me as odd that you check nbytes before status, is it valid
when an error occurs?
> +
> + if (status)
> + {
> + apr_file_close(fd);
> + return svn_error_wrap_apr (status,
> + _("Error reading lock entries file '%s.'"),
> + path);
> + }
> + if (buf_len != nbytes)
I don't understand why a short read is an error. Perhaps you should
be using apr_file_read_full?
> + {
> + apr_file_close(fd);
> + return svn_error_createf (SVN_ERR_FS_OUT_OF_DATE, NULL,
> + _("Error reading lock entries file '%s'."),
> + path);
> + }
> +
> + buf[buf_len - 1] = '\0'; /* Strip '\n' off the end. */
> + apr_hash_set (*entries, buf,
> + APR_HASH_KEY_STRING, &(buf[0])); /* Grab a byte for val.*/
> +
> + APR_DECLARE(apr_status_t) apr_file_read(apr_file_t *thefile, void *buf,
> + apr_size_t *nbytes);
What?
> + }
> +
> + apr_file_close(fd);
Ignoring apr_file_close errors on read is probably harmless, but why
take the risk?
> + return SVN_NO_ERROR;
> +}
> +
> +/* If PATH does not have a leading slash, prepend one and return the
> + new string. Else, just return a copy of PATH. */
> +static char *
> +repository_abs_path(const char *path,
> + apr_pool_t *pool)
> +{
> + char *new_path = apr_pstrdup (pool, path);
> + if (path[0] != '/')
> + new_path = apr_pstrcat (pool, "/", path, NULL);
Can you check path[0] before making the first copy and avoid copying
twice?
> + return new_path;
> +}
> +
> +
> +
> +/* If ABS_PATH exists, add DIGEST_STR to it iff it's not already in
> + it. Else, create ABS_PATH with DIGEST_STR as its only member. */
> +static svn_error_t *
> +add_hash_to_entries_file (const char *abs_path,
> + const char *digest_str,
> + apr_pool_t *pool)
> +{
> + apr_status_t status;
> + apr_finfo_t finfo;
> + char *content;
> +
> + status = apr_stat (&finfo, abs_path, APR_FINFO_TYPE, pool);
> + content = apr_pstrcat (pool, digest_str, "\n", NULL);
> +
Doing stat before open is not good from an efficiency point of view.
Perhaps you could try the open assuming either presence or absence (do
you know which is the more common?), and if that fails then try the
other.
> + if (APR_STATUS_IS_ENOENT (status)) /* Entries file doesn't exist. */
> + {
> + apr_status_t status;
> + apr_file_t *fd;
> +
> + status = apr_file_open (&fd, abs_path, APR_WRITE | APR_CREATE,
> + APR_OS_DEFAULT, pool);
> +
> + if (status)
> + return svn_error_wrap_apr (status,
> + _("Can't create '%s' for lock entry."),
> + abs_path);
> +
> + SVN_ERR (svn_io_file_write_full (fd, content, strlen (content),
> + NULL, pool));
> + apr_file_close(fd);
Again, ignoring errors when writing is bad.
> + }
> + else /* Entries file exists. */
> + {
> + apr_hash_t *entries;
> +
> + SVN_ERR (read_entries_file(&entries, abs_path, NULL, pool));
> +
> + /* - Append the MD5 hash of the next component to existing file. */
> + if (apr_hash_get (entries, digest_str, APR_HASH_KEY_STRING)
> + == NULL)
> + {
> + apr_status_t status;
> + apr_file_t *fd;
> +
> + status = apr_file_open (&fd, abs_path, APR_WRITE | APR_APPEND,
> + APR_OS_DEFAULT, pool);
> +
> + if (status)
> + return svn_error_wrap_apr (status,
> + _("Can't append lock entry to '%s'"),
> + abs_path);
> +
> + SVN_ERR (svn_io_file_write_full (fd, content, strlen (content),
> + NULL, pool));
The write_full functions simply guarantee that they return an error if
unable to write all the data, they don't guarantee to avoid partial
writes. What happens if this fails with a short write? Can the rest
of the code handle a file that is incomplete, or is the repository
hosed? Should this be done by writing to a temporary file and
renaming?
> + apr_file_close(fd);
Ignoring write errors again.
> + }
> + }
> + return SVN_NO_ERROR;
> +}
> +
> +
> +/* Store the lock in the OS level filesystem under
> + repos/db/locks/locks in a file named by composing the MD5 hash of
> + lock->path. */
> static svn_error_t *
> write_lock_to_file (svn_fs_t *fs,
> svn_lock_t *lock,
> @@ -146,21 +350,44 @@
> 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? */
> - SVN_ERR (abs_path_to_lock_file (&abs_path, fs, lock->path, pool));
> + apr_status_t status;
> + apr_array_header_t *nodes;
> + char *abs_path, *node_name, *path_so_far = "/";
> + const char *digest_str, *path;
> + int i;
>
> - /* 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);
> + SVN_ERR (base_path_to_lock_file (&abs_path, fs, pool));
> + status = apr_dir_make_recursive (abs_path, APR_OS_DEFAULT, pool);
>
> if (status)
> return svn_error_wrap_apr (status, _("Can't create lock directory '%s'"),
> - dir);
> + abs_path);
> +
> + path = repository_abs_path (lock->path, pool);
> + nodes = svn_path_decompose (path, pool);
> +
> + /* Make sure that each parent directory, starting with "/", has an
> + entries file on disk, and that the entries file contains an entry
> + for its child. */
> + for (i = 0; i < (nodes->nelts - 1); i++)
> + {
> + char *child_path = "/";
> +
> + node_name = APR_ARRAY_IDX (nodes, i, char *);
> +
> + SVN_ERR (merge_paths (&path_so_far, path_so_far, node_name, pool));
> +
> + SVN_ERR (merge_paths (&child_path,
> + path_so_far,
> + APR_ARRAY_IDX (nodes, i + 1, char *),
> + pool));
> +
> + digest_str = make_digest (child_path, pool);
> +
> + SVN_ERR (abs_path_to_lock_file (&abs_path, fs, path_so_far, pool));
> +
> + SVN_ERR (add_hash_to_entries_file (abs_path, digest_str, pool));
> + }
>
> /* Create our hash and load it up. */
> hash = apr_hash_make (pool);
> @@ -175,20 +402,22 @@
> hash_store (hash, EXPIRATION_DATE_KEY,
> svn_time_to_cstring(lock->expiration_date, pool), pool);
>
> + digest_str = make_digest (path, pool);
>
> + SVN_ERR (abs_path_to_lock_file (&abs_path, fs, path, pool));
> +
> 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);
> -
> +
> stream = svn_stream_from_aprfile (fd, pool);
> -
> +
> SVN_ERR_W (svn_hash_write2 (hash, stream, SVN_HASH_TERMINATOR, pool),
> apr_psprintf (pool,
> _("Cannot write lock hash to '%s'"),
> svn_path_local_style (abs_path, pool)));
> -
> return SVN_NO_ERROR;
> }
>
> @@ -242,26 +471,85 @@
> return SVN_NO_ERROR;
> }
>
> +
> +/* Join all items in COMPONENTS with directory separators to create
> + PATH. */
> +static svn_error_t *
> +merge_array_components (char **path,
> + apr_array_header_t *components,
> + apr_pool_t *pool)
> +{
> + int i;
> + *path = "/";
> +
> + for (i = 0; i < components->nelts; i++)
> + {
> + char *component;
> + component = APR_ARRAY_IDX (components, i, char *);
> + merge_paths (path, *path, component, pool);
Use SVN_ERR or svn_error_clear.
> + }
> + return SVN_NO_ERROR;
> +}
> +
> static svn_error_t *
> delete_lock (svn_fs_t *fs,
> svn_lock_t *lock,
> apr_pool_t *pool)
> {
> apr_status_t status = APR_SUCCESS;
> - char *abs_path;
> + apr_array_header_t *nodes;
> + char *abs_path, *path, *child_path, *parent_path, *node;
> + const char *digest_str;
>
> - /* Delete lock from locks area */
> - SVN_ERR (abs_path_to_lock_file (&abs_path, fs, lock->path, pool));
> - SVN_ERR (svn_io_remove_file (abs_path, pool));
> + path = repository_abs_path (lock->path, pool);
> + nodes = svn_path_decompose (path, pool);
>
> - /* Prune directories on the way back */
> - while (status == APR_SUCCESS)
> + do /* Iterate in reverse. */
> {
> - abs_path = svn_path_dirname (abs_path, pool);
> - /* If this fails, status will != APR_SUCCESS, so we drop out of
> - the loop. */
> - status = apr_dir_remove (abs_path, pool);
> + apr_hash_t *entries;
> +
> + SVN_ERR (merge_array_components (&child_path, nodes, pool));
> + digest_str = make_digest (child_path, pool);
> +
> + /* Compose the path to the parent entries file. */
> + (node = (char *) apr_array_pop (nodes));
Why the extra parentheses? Why the cast?
> + SVN_ERR (merge_array_components (&parent_path, nodes, pool));
> +
> + /* Stop when we get to the root. */
> + if (strcmp (child_path, "/") && strcmp (parent_path, "/"))
> + break;
Have you got that test correct?
> +
> + /* Remove child hash from entries, deleting the entries file if
> + we've removed the last hash. */
> + SVN_ERR (abs_path_to_lock_file (&abs_path, fs, parent_path, pool));
> + SVN_ERR (read_entries_file (&entries, abs_path, NULL, pool));
> + apr_hash_set (entries, digest_str,
> + APR_HASH_KEY_STRING, NULL); /* Delete from hash.*/
> +
> + if (apr_hash_count (entries) == 0) /* We just removed the last entry. */
> + {
> + status = apr_file_remove (abs_path, pool);
> + if (status)
> + return svn_error_wrap_apr (status,
> + _("Can't delete lock entries file '%s'"),
> + abs_path);
> + }
> + else
> + {
> + write_entries_file (entries, abs_path, pool);
Ignoring the write error again!
> + break; /* We're done deleting entries files. */
> + }
> }
> + while (node);
> +
> + digest_str = make_digest (path, pool);
> +
> + SVN_ERR (abs_path_to_lock_file (&abs_path, fs, path, pool));
> + status = apr_file_remove (abs_path, pool);
> + if (status)
> + return svn_error_wrap_apr (status,
> + _("Can't delete lock file '%s'."),
> + abs_path);
>
> /* Delete lock from tokens area */
> SVN_ERR (abs_path_to_lock_token_file (&abs_path, fs, lock->token, pool));
> @@ -288,7 +576,6 @@
> lock->path = apr_pstrdup (pool, path);
> lock->owner = apr_pstrdup (pool, owner);
> lock->comment = apr_pstrdup (pool, comment);
> - /* ### this function should take a 'comment' argument! */
> lock->creation_date = apr_time_now();
>
> if (timeout)
> @@ -298,6 +585,7 @@
> return SVN_NO_ERROR;
> }
>
> +
> static svn_error_t *
> read_path_from_lock_token_file(svn_fs_t *fs,
> char **path_p,
> @@ -330,37 +618,36 @@
> return SVN_NO_ERROR;
> }
>
> -
> +/* Read lock from file in FS at ABS_PATH into LOCK_P. If open FD is
> + non-NULL, use FD instead of opening a new file. FD will be closed
> + before returning. */
> static svn_error_t *
> -read_lock_from_file (svn_lock_t **lock_p,
> - svn_fs_t *fs,
> - const char *path,
> - apr_pool_t *pool)
> +read_lock_from_abs_path (svn_lock_t **lock_p,
> + svn_fs_t *fs,
> + const char *abs_path,
> + apr_file_t *fd,
> + 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;
>
> - SVN_ERR (abs_path_to_lock_file(&abs_path, 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);
> + return svn_fs_fs__err_no_such_lock (fs, abs_path);
> }
>
> - /* ###Is this necessary? */
> if (status && !APR_STATUS_IS_ENOENT (status))
> return svn_error_wrap_apr (status, _("Can't stat '%s'"), abs_path);
>
> - status = apr_file_open (&fd, abs_path, APR_READ, APR_OS_DEFAULT, pool);
> + if (!fd) /* Only open the file if we haven't been passed an apr_file_t. */
> + status = apr_file_open (&fd, abs_path, APR_READ, APR_OS_DEFAULT, pool);
> if (status)
> return svn_error_wrap_apr (status, _("Can't open '%s' to read lock"),
> abs_path);
> @@ -371,6 +658,8 @@
> SVN_ERR_W (svn_hash_read2 (hash, stream, SVN_HASH_TERMINATOR, pool),
> apr_psprintf (pool, _("Can't parse '%s'"), abs_path));
>
> + apr_file_close (fd);
No error checking.
> +
> /* Create our lock and load it up. */
> lock = apr_palloc (pool, sizeof (*lock));
>
> @@ -405,6 +694,42 @@
>
>
> static svn_error_t *
> +read_lock_from_file (svn_lock_t **lock_p,
> + svn_fs_t *fs,
> + const char *path,
> + apr_file_t *fd,
> + apr_pool_t *pool)
> +{
> + char *abs_path, *rep_path;
> + const char *digest_str;
> + /* - Gen MD5 hash of full path to file. */
> + rep_path = repository_abs_path (path, pool);
> + digest_str = make_digest (rep_path, pool);
What's digest_str for?
> +
> + SVN_ERR (abs_path_to_lock_file(&abs_path, fs, path, pool));
> + SVN_ERR (read_lock_from_abs_path (lock_p, fs, abs_path, fd, pool));
> +
> + return SVN_NO_ERROR;
> +}
> +
> +
> +static svn_error_t *
> +read_lock_from_hash_name (svn_lock_t **lock_p,
> + svn_fs_t *fs,
> + const char *hash,
> + apr_file_t *fd,
> + apr_pool_t *pool)
> +{
> + char *abs_path;
> +
> + SVN_ERR (abs_path_to_lock_digest_file (&abs_path, fs, hash, pool));
> + SVN_ERR (read_lock_from_abs_path (lock_p, fs, abs_path, fd, pool));
> +
> + return SVN_NO_ERROR;
> +}
> +
> +
> +static svn_error_t *
> get_lock_from_path (svn_lock_t **lock_p,
> svn_fs_t *fs,
> const char *path,
> @@ -412,7 +737,7 @@
> {
> svn_lock_t *lock;
>
> - SVN_ERR (read_lock_from_file (&lock, fs, path, pool));
> + SVN_ERR (read_lock_from_file (&lock, fs, path, NULL, pool));
>
> /* Possibly auto-expire the lock. */
> if (lock->expiration_date
> @@ -456,6 +781,56 @@
> }
>
>
> +/* A recursive function that puts all locks under PATH in FS into
> + LOCKS. FD, if non-NULL, will be read from by read_entries_file
> + instead of read_entries_file opening PATH. */
> +static svn_error_t *
> +get_locks_under_path (apr_hash_t **locks,
> + svn_fs_t *fs,
> + const char *path,
> + apr_file_t *fd,
> + apr_pool_t *pool)
> +{
> + apr_hash_t *entries;
> + char *abs_path;
> + apr_hash_index_t *hi;
> + char *child;
> + apr_off_t offset = 0;
> + apr_status_t status;
> +
> + SVN_ERR (read_entries_file (&entries, path, fd, pool));
> +
> + for (hi = apr_hash_first(pool, entries); hi; hi = apr_hash_next(hi))
> + {
> + int nbytes = 2;
> + char *buf = apr_palloc (pool, nbytes);
> + apr_hash_this(hi, (void *)&child, NULL, NULL);
Use temporaries to avoid that cast.
> +
> + abs_path_to_lock_digest_file (&abs_path, fs, child, pool);
SVN_ERR or svn_error_clear.
> +
> + status = apr_file_open (&fd, abs_path, APR_READ, APR_OS_DEFAULT, pool);
> + if (status)
> + return svn_error_wrap_apr (status,
> + _("Can't open '%s' to read lock info."),
> + abs_path);
> +
> + status = apr_file_read (fd, buf, &nbytes);
You don't check status, or nbytes. Perhaps use apr_file_read_full?
> + apr_file_seek (fd, APR_SET, &offset); /* Rewind to the beginning. */
Can apr_file_seek fail?
> +
> + if (strncmp (buf, "K ", 2) == 0) /* We have a lock file. */
> + {
> + svn_lock_t *lock;
> + SVN_ERR (read_lock_from_hash_name (&lock, fs, child, fd, pool));
> +
> + apr_hash_set (*locks, lock->path, APR_HASH_KEY_STRING, lock);
> + }
> + else /* It's another entries file. Recurse. */
> + get_locks_under_path (locks, fs, abs_path, fd, pool);
SVN_ERR or svn_error_clear.
You haven't closed fd.
> + }
> + return SVN_NO_ERROR;
> +}
> +
> +
> svn_error_t *
> svn_fs_fs__lock (svn_lock_t **lock_p,
> svn_fs_t *fs,
> @@ -728,42 +1103,6 @@
>
>
>
> -struct dir_walker_baton
> -{
> - svn_fs_t *fs;
> - apr_hash_t *locks;
> -};
> -
> -
> -static svn_error_t *
> -locks_dir_walker (void *baton,
> - const char *path,
> - const apr_finfo_t *finfo,
> - apr_pool_t *pool)
> -{
> - char *base_path;
> - const char *rel_path;
> - svn_lock_t *lock;
> - struct dir_walker_baton *dir_baton;
> - dir_baton = baton;
> -
> - /* Skip directories. */
> - if (finfo->filetype == APR_DIR)
> - return SVN_NO_ERROR;
> -
> - /* Get the repository-relative path for the lock. */
> - SVN_ERR (base_path_to_lock_file (&base_path, dir_baton->fs, pool));
> - rel_path = path + strlen (base_path);
> -
> - /* Get lock */
> - SVN_ERR (svn_fs_fs__get_lock_from_path (&lock, dir_baton->fs,
> - rel_path, pool));
> -
> - /* Stuff lock in hash, keyed on lock->path */
> - apr_hash_set (dir_baton->locks, lock->path, APR_HASH_KEY_STRING, lock);
> -
> - return SVN_NO_ERROR;
> -}
>
>
> svn_error_t *
> @@ -773,9 +1112,9 @@
> apr_pool_t *pool)
> {
> char *abs_path;
> - struct dir_walker_baton baton;
> apr_finfo_t finfo;
> apr_status_t status;
> + const char *digest_str;
>
> /* Make the hash that we'll return. */
> *locks = apr_hash_make(pool);
> @@ -783,18 +1122,23 @@
> /* Compose the absolute/rel path to PATH */
> SVN_ERR (abs_path_to_lock_file (&abs_path, fs, path, pool));
>
> + /* Strip any trailing slashes. */
> + if (abs_path[strlen (abs_path) - 1] == '/')
> + abs_path[strlen (abs_path) - 1] = '\0';
The comment says "slashes" but it only removes a single slash.
If strlen(abs_path) is not > 0 then accessing abs_path[-1] is dubious.
> +
> status = apr_stat (&finfo, abs_path, APR_FINFO_TYPE, pool);
>
> /* If base dir doesn't exist, then we don't have any locks. */
> if (APR_STATUS_IS_ENOENT (status))
> return SVN_NO_ERROR;
>
> - baton.fs = fs;
> - baton.locks = *locks;
> -
> - SVN_ERR (svn_io_dir_walk (abs_path, APR_FINFO_TYPE, locks_dir_walker,
> - &baton, pool));
> -
> + digest_str = make_digest (path, pool);
> + abs_path = repository_abs_path (path, pool);
> + SVN_ERR (abs_path_to_lock_file (&abs_path, fs, path, pool));
> +
> + /* Recursively walk lock "tree" */
> + SVN_ERR (get_locks_under_path (locks, fs, abs_path, NULL, pool));
> +
> return SVN_NO_ERROR;
> }
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r12676 - branches/locking/subversion/libsvn_fs_fs
Posted by Branko Čibej <br...@xbc.nu>.
Philip Martin wrote:
>"Brian W. Fitzpatrick" <fi...@red-bean.com> writes:
>
>
>
>>On Wed, 2005-01-12 at 12:01, Philip Martin wrote:
>>
>>
>>>"Brian W. Fitzpatrick" <fi...@red-bean.com> writes:
>>>
>>>
>>>
>>>>I'll be coming through in a pass later today to turn most of the
>>>>apr_file_FOO calls into svn_io_file_FOO calls with a nice neat SVN_ERR
>>>>wrapper around it (which should clean up the code a good bit).
>>>>
>>>>
>>>One difference between apr_ and svn_io_ is that the apr functions
>>>accept paths in the system's native encoding while the svn functions
>>>accept paths in UTF-8 and converts to native before calling apr. Now,
>>>the locking paths are ASCII representations of MD5 sums are they not?
>>>Does that mean that the encoding is irrelevant?
>>>
>>>
>>Yes.
>>
>>
>
>I'm not convinced. There are at least two problems:
>
>1. The restriction to ASCII only applies to the part of the path
> within the repository, the path to the repository may contain
> non-ASCII characters that require conversion.
>
>2. Suppose I have LANG set to a UTF-16 encoding, then even the ASCII
> bits of the path will need to be converted.
>
>Thus, I think the current use of apr_ functions in lock.c is wrong.
>The paths need to undergo UTF-8 to native conversion before being
>passed to the apr_ functions, just as in the rest of Subversion. This
>applies to the apr_filepath_merge, apr_dir_make_recursive and
>apr_status calls.
>
>
I agree with Philip. It's dangerous to make any kind of assumption about
path encoding, or to take shortcuts.
-- Brane
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r12676 - branches/locking/subversion/libsvn_fs_fs
Posted by "Brian W. Fitzpatrick" <fi...@red-bean.com>.
On Jan 13, 2005, at 12:54 PM, Philip Martin wrote:
> "Brian W. Fitzpatrick" <fi...@red-bean.com> writes:
>
>> On Wed, 2005-01-12 at 12:01, Philip Martin wrote:
>>> "Brian W. Fitzpatrick" <fi...@red-bean.com> writes:
>>>
>>>> I'll be coming through in a pass later today to turn most of the
>>>> apr_file_FOO calls into svn_io_file_FOO calls with a nice neat
>>>> SVN_ERR
>>>> wrapper around it (which should clean up the code a good bit).
>>>
>>> One difference between apr_ and svn_io_ is that the apr functions
>>> accept paths in the system's native encoding while the svn functions
>>> accept paths in UTF-8 and converts to native before calling apr.
>>> Now,
>>> the locking paths are ASCII representations of MD5 sums are they not?
>>> Does that mean that the encoding is irrelevant?
>>
>> Yes.
>
> I'm not convinced. There are at least two problems:
>
> 1. The restriction to ASCII only applies to the part of the path
> within the repository, the path to the repository may contain
> non-ASCII characters that require conversion.
>
> 2. Suppose I have LANG set to a UTF-16 encoding, then even the ASCII
> bits of the path will need to be converted.
>
> Thus, I think the current use of apr_ functions in lock.c is wrong.
> The paths need to undergo UTF-8 to native conversion before being
> passed to the apr_ functions, just as in the rest of Subversion. This
> applies to the apr_filepath_merge, apr_dir_make_recursive and
> apr_status calls.
You are, of course, completely correct.
This has been fixed in r12770.
-Fitz
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r12676 - branches/locking/subversion/libsvn_fs_fs
Posted by Philip Martin <ph...@codematters.co.uk>.
"Brian W. Fitzpatrick" <fi...@red-bean.com> writes:
> On Wed, 2005-01-12 at 12:01, Philip Martin wrote:
>> "Brian W. Fitzpatrick" <fi...@red-bean.com> writes:
>>
>> > I'll be coming through in a pass later today to turn most of the
>> > apr_file_FOO calls into svn_io_file_FOO calls with a nice neat SVN_ERR
>> > wrapper around it (which should clean up the code a good bit).
>>
>> One difference between apr_ and svn_io_ is that the apr functions
>> accept paths in the system's native encoding while the svn functions
>> accept paths in UTF-8 and converts to native before calling apr. Now,
>> the locking paths are ASCII representations of MD5 sums are they not?
>> Does that mean that the encoding is irrelevant?
>
> Yes.
I'm not convinced. There are at least two problems:
1. The restriction to ASCII only applies to the part of the path
within the repository, the path to the repository may contain
non-ASCII characters that require conversion.
2. Suppose I have LANG set to a UTF-16 encoding, then even the ASCII
bits of the path will need to be converted.
Thus, I think the current use of apr_ functions in lock.c is wrong.
The paths need to undergo UTF-8 to native conversion before being
passed to the apr_ functions, just as in the rest of Subversion. This
applies to the apr_filepath_merge, apr_dir_make_recursive and
apr_status calls.
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r12676 - branches/locking/subversion/libsvn_fs_fs
Posted by "Brian W. Fitzpatrick" <fi...@red-bean.com>.
On Wed, 2005-01-12 at 12:01, Philip Martin wrote:
> "Brian W. Fitzpatrick" <fi...@red-bean.com> writes:
>
> > I'll be coming through in a pass later today to turn most of the
> > apr_file_FOO calls into svn_io_file_FOO calls with a nice neat SVN_ERR
> > wrapper around it (which should clean up the code a good bit).
>
> One difference between apr_ and svn_io_ is that the apr functions
> accept paths in the system's native encoding while the svn functions
> accept paths in UTF-8 and converts to native before calling apr. Now,
> the locking paths are ASCII representations of MD5 sums are they not?
> Does that mean that the encoding is irrelevant?
Yes.
> > I modified the code to use write_entries_file, which I
> > modified to do an atomic write by using a temporary file. The only
> > problem with this is that since I had been just appending to the file,
> > I'm now rewriting it entirely--a performance hit, but I guess
> > something that we'll have to live with to deal with to prevent writing
> > half a hash to an entries file.
>
> Do you modify an file more than once during a single operation? If so
> then the WC code used to have a similar problem with the entries file.
> It's much less of a problem now, see notes/entries-caching for some of
> the gory details.
>
> >>> + if (strncmp (buf, "K ", 2) == 0) /* We have a lock file. */
> >>> + {
> >>> + svn_lock_t *lock;
> >>> + SVN_ERR (read_lock_from_hash_name (&lock, fs, child, fd,
> >>> pool));
> >>> +
> >>> + apr_hash_set (*locks, lock->path, APR_HASH_KEY_STRING,
> >>> lock);
> >>> + }
> >>> + else /* It's another entries file. Recurse. */
> >>> + get_locks_under_path (locks, fs, abs_path, fd, pool);
> >>
> >> SVN_ERR or svn_error_clear.
> >
> > Done.
> >
> >> You haven't closed fd.
> >
> > read_lock_from_abs_path closes it.
>
> And that is called by read_lock_from_hash_name? I think that the call
> to read_lock_from_hash_name warrents a comment about the closing of
> the file.
*nod* See below.
> > I could change the code so that I
> > (the caller) always close the fd, but I was worried that if an
> > svn_error_t was thrown before it returned to me, the file would never
> > get closed. Thoughts?
>
> The fallback is that the file will get closed automatically when the
> pool passed to the open call gets cleared/destroyed. That is usually
> sufficient for most errors.
In that case, I'll just have the caller close the fd--I prefer that.
Thanks,
-Fitz
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r12676 - branches/locking/subversion/libsvn_fs_fs
Posted by Philip Martin <ph...@codematters.co.uk>.
"Brian W. Fitzpatrick" <fi...@red-bean.com> writes:
> I'll be coming through in a pass later today to turn most of the
> apr_file_FOO calls into svn_io_file_FOO calls with a nice neat SVN_ERR
> wrapper around it (which should clean up the code a good bit).
One difference between apr_ and svn_io_ is that the apr functions
accept paths in the system's native encoding while the svn functions
accept paths in UTF-8 and converts to native before calling apr. Now,
the locking paths are ASCII representations of MD5 sums are they not?
Does that mean that the encoding is irrelevant?
> I modified the code to use write_entries_file, which I
> modified to do an atomic write by using a temporary file. The only
> problem with this is that since I had been just appending to the file,
> I'm now rewriting it entirely--a performance hit, but I guess
> something that we'll have to live with to deal with to prevent writing
> half a hash to an entries file.
Do you modify an file more than once during a single operation? If so
then the WC code used to have a similar problem with the entries file.
It's much less of a problem now, see notes/entries-caching for some of
the gory details.
>>> + if (strncmp (buf, "K ", 2) == 0) /* We have a lock file. */
>>> + {
>>> + svn_lock_t *lock;
>>> + SVN_ERR (read_lock_from_hash_name (&lock, fs, child, fd,
>>> pool));
>>> +
>>> + apr_hash_set (*locks, lock->path, APR_HASH_KEY_STRING,
>>> lock);
>>> + }
>>> + else /* It's another entries file. Recurse. */
>>> + get_locks_under_path (locks, fs, abs_path, fd, pool);
>>
>> SVN_ERR or svn_error_clear.
>
> Done.
>
>> You haven't closed fd.
>
> read_lock_from_abs_path closes it.
And that is called by read_lock_from_hash_name? I think that the call
to read_lock_from_hash_name warrents a comment about the closing of
the file.
> I could change the code so that I
> (the caller) always close the fd, but I was worried that if an
> svn_error_t was thrown before it returned to me, the file would never
> get closed. Thoughts?
The fallback is that the file will get closed automatically when the
pool passed to the open call gets cleared/destroyed. That is usually
sufficient for most errors.
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r12676 - branches/locking/subversion/libsvn_fs_fs
Posted by "Brian W. Fitzpatrick" <fi...@red-bean.com>.
Philip,
Thank you *so* much for the detailed review. I've applied most of the
changes that you recommended, with a few comments and a question or two
inline below.
I'll be coming through in a pass later today to turn most of the
apr_file_FOO calls into svn_io_file_FOO calls with a nice neat SVN_ERR
wrapper around it (which should clean up the code a good bit).
Applied in r12682.
Thanks again,
-Fitz
On Jan 11, 2005, at 4:27 PM, Philip Martin wrote:
> fitz@tigris.org writes:
>
>> Author: fitz
>> Date: Tue Jan 11 14:15:26 2005
>> New Revision: 12676
>
>> --- branches/locking/subversion/libsvn_fs_fs/lock.c (original)
>> +++ branches/locking/subversion/libsvn_fs_fs/lock.c Tue Jan 11
>> 14:15:26 2005
>> @@ -23,10 +23,12 @@
>> #include "svn_hash.h"
>> #include "svn_time.h"
>> #include "svn_utf.h"
>> +#include "svn_md5.h"
>>
>> #include "apr_uuid.h"
>> #include "apr_file_io.h"
>> #include "apr_file_info.h"
>> +#include "apr_md5.h"
>>
>> #include "lock.h"
>> #include "tree.h"
>> @@ -69,14 +71,47 @@
>> }
>>
>
> No docstring.
Added.
>> static svn_error_t *
>> +abs_path_to_lock_digest_file (char **abs_path,
>> + svn_fs_t *fs,
>> + const char *digest,
>> + apr_pool_t *pool)
>> +{
>> + SVN_ERR (merge_paths (abs_path, fs->path, LOCK_ROOT_DIR, pool));
>> + SVN_ERR (merge_paths (abs_path, *abs_path, LOCK_LOCK_DIR, pool));
>> + /* ###TODO create a 1 or 2 char subdir to spread the love across
>> + many directories */
>> + SVN_ERR (merge_paths (abs_path, *abs_path, digest, pool));
>> +
>> + return SVN_NO_ERROR;
>> +}
>> +
>> +
>
> ditto.
Added.
>> +static const char *
>> +make_digest (const char *str,
>> + apr_pool_t *pool)
>> +{
>> + unsigned char digest[APR_MD5_DIGESTSIZE];
>> +
>> + apr_md5 (digest, str, strlen(str));
>> + return svn_md5_digest_to_cstring (digest, pool);
>> +}
>> +
>
> ditto (although not strictly caused by this commit).
Added.
>> +static svn_error_t *
>> abs_path_to_lock_file (char **abs_path,
>> svn_fs_t *fs,
>> const char *rel_path,
>> apr_pool_t *pool)
>> {
>> + const char *digest_cstring;
>> +
>> SVN_ERR (merge_paths (abs_path, fs->path, LOCK_ROOT_DIR, pool));
>> SVN_ERR (merge_paths (abs_path, *abs_path, LOCK_LOCK_DIR, pool));
>> - SVN_ERR (merge_paths (abs_path, *abs_path, (char *)rel_path,
>> pool));
>> +
>> + digest_cstring = make_digest (rel_path, pool);
>> +
>> + /* ###TODO create a 1 or 2 char subdir to spread the love across
>> + many directories */
>> + SVN_ERR (merge_paths (abs_path, *abs_path, digest_cstring, pool));
>>
>> return SVN_NO_ERROR;
>> }
>> @@ -88,6 +123,8 @@
>> {
>> SVN_ERR (merge_paths (base_path, fs->path, LOCK_ROOT_DIR, pool));
>> SVN_ERR (merge_paths (base_path, *base_path, LOCK_LOCK_DIR, pool));
>> + /* ###TODO create a 1 or 2 char subdir to spread the love across
>> + many directories (and take the hash as an optional arg. */
>>
>> return SVN_NO_ERROR;
>> }
>> @@ -134,10 +171,177 @@
>> return NULL;
>> }
>>
>> +/* Write each hash in ENTRIES to path, one hash per line. */
>> +static svn_error_t *
>> +write_entries_file (apr_hash_t *entries,
>> + char *path,
>> + apr_pool_t *pool)
>> +{
>> + apr_file_t *fd;
>> + apr_hash_index_t *hi;
>> + char *digest, *content;
>> + int status;
>> +
>> + status = apr_file_open (&fd, path, APR_WRITE | APR_TRUNCATE,
>> + APR_OS_DEFAULT, pool);
>>
>> -/* 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. */
>> + if (status)
>> + return svn_error_wrap_apr (status,
>> + _("Can't open '%s' to write lock
>> entries."),
>> + path);
>> +
>> + for (hi = apr_hash_first(pool, entries); hi; hi =
>> apr_hash_next(hi))
>> + {
>> + apr_hash_this(hi, (void *)&digest, NULL, NULL);
>
> Look at other uses of apr_hash_this, use temporary variables to avoid
> that cast.
Done.
>> + content = apr_pstrcat (pool, digest, "\n", NULL);
>> + svn_io_file_write_full (fd, content, strlen(content), NULL,
>> pool);
>
> If you want to ignore svn_error_t errors then use svn_error_clear, but
> I don't think you should be ignoring write errors.
You're right. I don't want to ignore those errors. I've added the
check.
>> + }
>> + apr_file_close (fd);
>
> You should not ignore the error from apr_file_close, particularly when
> writing.
Checking for it now.
>> + return SVN_NO_ERROR;
>
> Yeah, but only because you ignored all the errors!
Ignorance really is bliss, isn't it? :)
>> +}
>> +
>> +/* Read lock entries file at PATH, adding each child hash as a key in
>> + ENTRIES. If FD is non-NULL, read entries from FD instead of
>> + PATH. FD will be closed before returning. */
>
> It also creates *ENTRIES. Perhaps mention the C type of the
> keys/values.
Done.
>> +static svn_error_t *
>> +read_entries_file (apr_hash_t **entries,
>> + const char *path,
>> + apr_file_t *fd,
>> + apr_pool_t *pool)
>> +{
>> + apr_status_t status;
>> + apr_size_t buf_len = (APR_MD5_DIGESTSIZE * 2) + 1; /* Add room for
>> "\n". */
>> +
>> + *entries = apr_hash_make (pool);
>> +
>> + if (!fd) /* Only open PATH if fd is NULL. */
>> + {
>> + status = apr_file_open (&fd, path, APR_READ, APR_OS_DEFAULT,
>> pool);
>
> Odd indentation.
Fixed.
>> + /* This happens when attempting to open the entries for "/" and
>> it's
>> + been deleted. */
>> + if (APR_STATUS_IS_ENOENT (status))
>> + return SVN_NO_ERROR;
>> + if (status)
>> + return svn_error_wrap_apr (status,
>> + _("Can't open '%s' to read lock
>> entries."),
>> + path);
>> + }
>> +
>> + while (apr_file_eof (fd) != APR_EOF)
>> + {
>> + apr_size_t nbytes = buf_len;
>> + char *buf = apr_palloc (pool, buf_len);
>> + status = apr_file_read (fd, buf, &nbytes);
>> +
>> + if (nbytes == 0)
>> + continue;
>
> It strikes me as odd that you check nbytes before status, is it valid
> when an error occurs?
Yes. I should have been checking for APR_EOF and exiting, which will
eliminate the need to test for EOF in the while clause. Fixed.
>> + if (status)
>> + {
>> + apr_file_close(fd);
>> + return svn_error_wrap_apr (status,
>> + _("Error reading lock entries
>> file '%s.'"),
>> + path);
>> + }
>> + if (buf_len != nbytes)
>
> I don't understand why a short read is an error. Perhaps you should
> be using apr_file_read_full?
Yep. Changed it to use apr_file_read_full.
>> + {
>> + apr_file_close(fd);
>> + return svn_error_createf (SVN_ERR_FS_OUT_OF_DATE, NULL,
>> + _("Error reading lock entries
>> file '%s'."),
>> + path);
>> + }
>> +
>> + buf[buf_len - 1] = '\0'; /* Strip '\n' off the end. */
>> + apr_hash_set (*entries, buf,
>> + APR_HASH_KEY_STRING, &(buf[0])); /* Grab a byte
>> for val.*/
>> +
>> + APR_DECLARE(apr_status_t) apr_file_read(apr_file_t *thefile,
>> void *buf,
>> + apr_size_t *nbytes);
>
> What?
Doh! Cut and paste error. Kids, don't try this at home.
Excised.
>> + }
>> +
>> + apr_file_close(fd);
>
> Ignoring apr_file_close errors on read is probably harmless, but why
> take the risk?
Fixed.
>> + return SVN_NO_ERROR;
>> +}
>> +
>> +/* If PATH does not have a leading slash, prepend one and return the
>> + new string. Else, just return a copy of PATH. */
>> +static char *
>> +repository_abs_path(const char *path,
>> + apr_pool_t *pool)
>> +{
>> + char *new_path = apr_pstrdup (pool, path);
>> + if (path[0] != '/')
>> + new_path = apr_pstrcat (pool, "/", path, NULL);
>
> Can you check path[0] before making the first copy and avoid copying
> twice?
Duh, of course! In fact, the copy isn't even necessary since I'm doing
the pstrcat. Fixed.
>> + return new_path;
>> +}
>> +
>> +
>> +
>> +/* If ABS_PATH exists, add DIGEST_STR to it iff it's not already in
>> + it. Else, create ABS_PATH with DIGEST_STR as its only member. */
>> +static svn_error_t *
>> +add_hash_to_entries_file (const char *abs_path,
>> + const char *digest_str,
>> + apr_pool_t *pool)
>> +{
>> + apr_status_t status;
>> + apr_finfo_t finfo;
>> + char *content;
>> +
>> + status = apr_stat (&finfo, abs_path, APR_FINFO_TYPE, pool);
>> + content = apr_pstrcat (pool, digest_str, "\n", NULL);
>> +
>
> Doing stat before open is not good from an efficiency point of view.
> Perhaps you could try the open assuming either presence or absence (do
> you know which is the more common?), and if that fails then try the
> other.
There's no way to tell which is more common--it depends on the
repository. A repository that has many files locked in the same
directory will have the entries file more often than not. A repository
with few files locked in many directories will not have the entries
file very often.
In any case, I've changed it to just try and read the entries file
right off the bat. If it returns a hash with > 0 entries, then I
atomically write out the entries file (including the existing entries).
Otherwise, I open and write the new file. This has the added benefit
of drastically simplifying that function.
>> + if (APR_STATUS_IS_ENOENT (status)) /* Entries file doesn't exist.
>> */
>> + {
>> + apr_status_t status;
>> + apr_file_t *fd;
>> +
>> + status = apr_file_open (&fd, abs_path, APR_WRITE | APR_CREATE,
>> + APR_OS_DEFAULT, pool);
>> +
>> + if (status)
>> + return svn_error_wrap_apr (status,
>> + _("Can't create '%s' for lock
>> entry."),
>> + abs_path);
>> +
>> + SVN_ERR (svn_io_file_write_full (fd, content, strlen (content),
>> + NULL, pool));
>> + apr_file_close(fd);
>
> Again, ignoring errors when writing is bad.
Agreed. Fixed.
>> + }
>> + else /* Entries file exists. */
>> + {
>> + apr_hash_t *entries;
>> +
>> + SVN_ERR (read_entries_file(&entries, abs_path, NULL, pool));
>> +
>> + /* - Append the MD5 hash of the next component to existing
>> file. */
>> + if (apr_hash_get (entries, digest_str, APR_HASH_KEY_STRING)
>> + == NULL)
>> + {
>> + apr_status_t status;
>> + apr_file_t *fd;
>> +
>> + status = apr_file_open (&fd, abs_path, APR_WRITE |
>> APR_APPEND,
>> + APR_OS_DEFAULT, pool);
>> +
>> + if (status)
>> + return svn_error_wrap_apr (status,
>> + _("Can't append lock entry to
>> '%s'"),
>> + abs_path);
>> +
>> + SVN_ERR (svn_io_file_write_full (fd, content, strlen
>> (content),
>> + NULL, pool));
>
> The write_full functions simply guarantee that they return an error if
> unable to write all the data, they don't guarantee to avoid partial
> writes. What happens if this fails with a short write? Can the rest
> of the code handle a file that is incomplete, or is the repository
> hosed? Should this be done by writing to a temporary file and
> renaming?
Yes it should. I modified the code to use write_entries_file, which I
modified to do an atomic write by using a temporary file. The only
problem with this is that since I had been just appending to the file,
I'm now rewriting it entirely--a performance hit, but I guess something
that we'll have to live with to deal with to prevent writing half a
hash to an entries file.
>> + apr_file_close(fd);
>
> Ignoring write errors again.
Fixed.
>> + }
>> + }
>> + return SVN_NO_ERROR;
>> +}
>> +
>> +
>> +/* Store the lock in the OS level filesystem under
>> + repos/db/locks/locks in a file named by composing the MD5 hash of
>> + lock->path. */
>> static svn_error_t *
>> write_lock_to_file (svn_fs_t *fs,
>> svn_lock_t *lock,
>> @@ -146,21 +350,44 @@
>> 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? */
>> - SVN_ERR (abs_path_to_lock_file (&abs_path, fs, lock->path, pool));
>> + apr_status_t status;
>> + apr_array_header_t *nodes;
>> + char *abs_path, *node_name, *path_so_far = "/";
>> + const char *digest_str, *path;
>> + int i;
>>
>> - /* 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);
>> + SVN_ERR (base_path_to_lock_file (&abs_path, fs, pool));
>> + status = apr_dir_make_recursive (abs_path, APR_OS_DEFAULT, pool);
>>
>> if (status)
>> return svn_error_wrap_apr (status, _("Can't create lock
>> directory '%s'"),
>> - dir);
>> + abs_path);
>> +
>> + path = repository_abs_path (lock->path, pool);
>> + nodes = svn_path_decompose (path, pool);
>> +
>> + /* Make sure that each parent directory, starting with "/", has an
>> + entries file on disk, and that the entries file contains an
>> entry
>> + for its child. */
>> + for (i = 0; i < (nodes->nelts - 1); i++)
>> + {
>> + char *child_path = "/";
>> +
>> + node_name = APR_ARRAY_IDX (nodes, i, char *);
>> +
>> + SVN_ERR (merge_paths (&path_so_far, path_so_far, node_name,
>> pool));
>> +
>> + SVN_ERR (merge_paths (&child_path,
>> + path_so_far,
>> + APR_ARRAY_IDX (nodes, i + 1, char *),
>> + pool));
>> +
>> + digest_str = make_digest (child_path, pool);
>> +
>> + SVN_ERR (abs_path_to_lock_file (&abs_path, fs, path_so_far,
>> pool));
>> +
>> + SVN_ERR (add_hash_to_entries_file (abs_path, digest_str,
>> pool));
>> + }
>>
>> /* Create our hash and load it up. */
>> hash = apr_hash_make (pool);
>> @@ -175,20 +402,22 @@
>> hash_store (hash, EXPIRATION_DATE_KEY,
>> svn_time_to_cstring(lock->expiration_date, pool),
>> pool);
>>
>> + digest_str = make_digest (path, pool);
>>
>> + SVN_ERR (abs_path_to_lock_file (&abs_path, fs, path, pool));
>> +
>> 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);
>> -
>> +
>> stream = svn_stream_from_aprfile (fd, pool);
>> -
>> +
>> SVN_ERR_W (svn_hash_write2 (hash, stream, SVN_HASH_TERMINATOR,
>> pool),
>> apr_psprintf (pool,
>> _("Cannot write lock hash to '%s'"),
>> svn_path_local_style (abs_path, pool)));
>> -
>> return SVN_NO_ERROR;
>> }
>>
>> @@ -242,26 +471,85 @@
>> return SVN_NO_ERROR;
>> }
>>
>> +
>> +/* Join all items in COMPONENTS with directory separators to create
>> + PATH. */
>> +static svn_error_t *
>> +merge_array_components (char **path,
>> + apr_array_header_t *components,
>> + apr_pool_t *pool)
>> +{
>> + int i;
>> + *path = "/";
>> +
>> + for (i = 0; i < components->nelts; i++)
>> + {
>> + char *component;
>> + component = APR_ARRAY_IDX (components, i, char *);
>> + merge_paths (path, *path, component, pool);
>
> Use SVN_ERR or svn_error_clear.
Done
>> + }
>> + return SVN_NO_ERROR;
>> +}
>> +
>> static svn_error_t *
>> delete_lock (svn_fs_t *fs,
>> svn_lock_t *lock,
>> apr_pool_t *pool)
>> {
>> apr_status_t status = APR_SUCCESS;
>> - char *abs_path;
>> + apr_array_header_t *nodes;
>> + char *abs_path, *path, *child_path, *parent_path, *node;
>> + const char *digest_str;
>>
>> - /* Delete lock from locks area */
>> - SVN_ERR (abs_path_to_lock_file (&abs_path, fs, lock->path, pool));
>> - SVN_ERR (svn_io_remove_file (abs_path, pool));
>> + path = repository_abs_path (lock->path, pool);
>> + nodes = svn_path_decompose (path, pool);
>>
>> - /* Prune directories on the way back */
>> - while (status == APR_SUCCESS)
>> + do /* Iterate in reverse. */
>> {
>> - abs_path = svn_path_dirname (abs_path, pool);
>> - /* If this fails, status will != APR_SUCCESS, so we drop out of
>> - the loop. */
>> - status = apr_dir_remove (abs_path, pool);
>> + apr_hash_t *entries;
>> +
>> + SVN_ERR (merge_array_components (&child_path, nodes, pool));
>> + digest_str = make_digest (child_path, pool);
>> +
>> + /* Compose the path to the parent entries file. */
>> + (node = (char *) apr_array_pop (nodes));
>
> Why the extra parentheses? Why the cast?
Unnecessary. Removed.
>> + SVN_ERR (merge_array_components (&parent_path, nodes, pool));
>> +
>> + /* Stop when we get to the root. */
>> + if (strcmp (child_path, "/") && strcmp (parent_path, "/"))
>> + break;
>
> Have you got that test correct?
No. Fixed.
>> + /* Remove child hash from entries, deleting the entries file if
>> + we've removed the last hash. */
>> + SVN_ERR (abs_path_to_lock_file (&abs_path, fs, parent_path,
>> pool));
>> + SVN_ERR (read_entries_file (&entries, abs_path, NULL, pool));
>> + apr_hash_set (entries, digest_str,
>> + APR_HASH_KEY_STRING, NULL); /* Delete from
>> hash.*/
>> +
>> + if (apr_hash_count (entries) == 0) /* We just removed the last
>> entry. */
>> + {
>> + status = apr_file_remove (abs_path, pool);
>> + if (status)
>> + return svn_error_wrap_apr (status,
>> + _("Can't delete lock entries
>> file '%s'"),
>> + abs_path);
>> + }
>> + else
>> + {
>> + write_entries_file (entries, abs_path, pool);
>
> Ignoring the write error again!
Fixed.
>> + break; /* We're done deleting entries files. */
>> + }
>> }
>> + while (node);
>> +
>> + digest_str = make_digest (path, pool);
>> +
>> + SVN_ERR (abs_path_to_lock_file (&abs_path, fs, path, pool));
>> + status = apr_file_remove (abs_path, pool);
>> + if (status)
>> + return svn_error_wrap_apr (status,
>> + _("Can't delete lock file '%s'."),
>> + abs_path);
>>
>> /* Delete lock from tokens area */
>> SVN_ERR (abs_path_to_lock_token_file (&abs_path, fs, lock->token,
>> pool));
>> @@ -288,7 +576,6 @@
>> lock->path = apr_pstrdup (pool, path);
>> lock->owner = apr_pstrdup (pool, owner);
>> lock->comment = apr_pstrdup (pool, comment);
>> - /* ### this function should take a 'comment' argument! */
>> lock->creation_date = apr_time_now();
>>
>> if (timeout)
>> @@ -298,6 +585,7 @@
>> return SVN_NO_ERROR;
>> }
>>
>> +
>> static svn_error_t *
>> read_path_from_lock_token_file(svn_fs_t *fs,
>> char **path_p,
>> @@ -330,37 +618,36 @@
>> return SVN_NO_ERROR;
>> }
>>
>> -
>> +/* Read lock from file in FS at ABS_PATH into LOCK_P. If open FD is
>> + non-NULL, use FD instead of opening a new file. FD will be closed
>> + before returning. */
>> static svn_error_t *
>> -read_lock_from_file (svn_lock_t **lock_p,
>> - svn_fs_t *fs,
>> - const char *path,
>> - apr_pool_t *pool)
>> +read_lock_from_abs_path (svn_lock_t **lock_p,
>> + svn_fs_t *fs,
>> + const char *abs_path,
>> + apr_file_t *fd,
>> + 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;
>>
>> - SVN_ERR (abs_path_to_lock_file(&abs_path, 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);
>> + return svn_fs_fs__err_no_such_lock (fs, abs_path);
>> }
>>
>> - /* ###Is this necessary? */
>> if (status && !APR_STATUS_IS_ENOENT (status))
>> return svn_error_wrap_apr (status, _("Can't stat '%s'"),
>> abs_path);
>>
>> - status = apr_file_open (&fd, abs_path, APR_READ, APR_OS_DEFAULT,
>> pool);
>> + if (!fd) /* Only open the file if we haven't been passed an
>> apr_file_t. */
>> + status = apr_file_open (&fd, abs_path, APR_READ, APR_OS_DEFAULT,
>> pool);
>> if (status)
>> return svn_error_wrap_apr (status, _("Can't open '%s' to read
>> lock"),
>> abs_path);
>> @@ -371,6 +658,8 @@
>> SVN_ERR_W (svn_hash_read2 (hash, stream, SVN_HASH_TERMINATOR,
>> pool),
>> apr_psprintf (pool, _("Can't parse '%s'"), abs_path));
>>
>> + apr_file_close (fd);
>
> No error checking.
Fixed.
>> +
>> /* Create our lock and load it up. */
>> lock = apr_palloc (pool, sizeof (*lock));
>>
>> @@ -405,6 +694,42 @@
>>
>>
>> static svn_error_t *
>> +read_lock_from_file (svn_lock_t **lock_p,
>> + svn_fs_t *fs,
>> + const char *path,
>> + apr_file_t *fd,
>> + apr_pool_t *pool)
>> +{
>> + char *abs_path, *rep_path;
>> + const char *digest_str;
>> + /* - Gen MD5 hash of full path to file. */
>> + rep_path = repository_abs_path (path, pool);
>> + digest_str = make_digest (rep_path, pool);
>
> What's digest_str for?
Nothing. It's gone (It was a leftover from a previous experiment).
>> +
>> + SVN_ERR (abs_path_to_lock_file(&abs_path, fs, path, pool));
>> + SVN_ERR (read_lock_from_abs_path (lock_p, fs, abs_path, fd, pool));
>> +
>> + return SVN_NO_ERROR;
>> +}
>> +
>> +
>> +static svn_error_t *
>> +read_lock_from_hash_name (svn_lock_t **lock_p,
>> + svn_fs_t *fs,
>> + const char *hash,
>> + apr_file_t *fd,
>> + apr_pool_t *pool)
>> +{
>> + char *abs_path;
>> +
>> + SVN_ERR (abs_path_to_lock_digest_file (&abs_path, fs, hash, pool));
>> + SVN_ERR (read_lock_from_abs_path (lock_p, fs, abs_path, fd, pool));
>> +
>> + return SVN_NO_ERROR;
>> +}
>> +
>> +
>> +static svn_error_t *
>> get_lock_from_path (svn_lock_t **lock_p,
>> svn_fs_t *fs,
>> const char *path,
>> @@ -412,7 +737,7 @@
>> {
>> svn_lock_t *lock;
>>
>> - SVN_ERR (read_lock_from_file (&lock, fs, path, pool));
>> + SVN_ERR (read_lock_from_file (&lock, fs, path, NULL, pool));
>>
>> /* Possibly auto-expire the lock. */
>> if (lock->expiration_date
>> @@ -456,6 +781,56 @@
>> }
>>
>>
>> +/* A recursive function that puts all locks under PATH in FS into
>> + LOCKS. FD, if non-NULL, will be read from by read_entries_file
>> + instead of read_entries_file opening PATH. */
>> +static svn_error_t *
>> +get_locks_under_path (apr_hash_t **locks,
>> + svn_fs_t *fs,
>> + const char *path,
>> + apr_file_t *fd,
>> + apr_pool_t *pool)
>> +{
>> + apr_hash_t *entries;
>> + char *abs_path;
>> + apr_hash_index_t *hi;
>> + char *child;
>> + apr_off_t offset = 0;
>> + apr_status_t status;
>> +
>> + SVN_ERR (read_entries_file (&entries, path, fd, pool));
>> +
>> + for (hi = apr_hash_first(pool, entries); hi; hi =
>> apr_hash_next(hi))
>> + {
>> + int nbytes = 2;
>> + char *buf = apr_palloc (pool, nbytes);
>> + apr_hash_this(hi, (void *)&child, NULL, NULL);
>
> Use temporaries to avoid that cast.
Fixed.
>> +
>> + abs_path_to_lock_digest_file (&abs_path, fs, child, pool);
>
> SVN_ERR or svn_error_clear.
Fixed.
>> + status = apr_file_open (&fd, abs_path, APR_READ,
>> APR_OS_DEFAULT, pool);
>> + if (status)
>> + return svn_error_wrap_apr (status,
>> + _("Can't open '%s' to read lock
>> info."),
>> + abs_path);
>> +
>> + status = apr_file_read (fd, buf, &nbytes);
>
> You don't check status, or nbytes. Perhaps use apr_file_read_full?
Done.
>> + apr_file_seek (fd, APR_SET, &offset); /* Rewind to the
>> beginning. */
>
> Can apr_file_seek fail?
Certainly. Added check.
>> +
>> + if (strncmp (buf, "K ", 2) == 0) /* We have a lock file. */
>> + {
>> + svn_lock_t *lock;
>> + SVN_ERR (read_lock_from_hash_name (&lock, fs, child, fd,
>> pool));
>> +
>> + apr_hash_set (*locks, lock->path, APR_HASH_KEY_STRING,
>> lock);
>> + }
>> + else /* It's another entries file. Recurse. */
>> + get_locks_under_path (locks, fs, abs_path, fd, pool);
>
> SVN_ERR or svn_error_clear.
Done.
> You haven't closed fd.
read_lock_from_abs_path closes it. I could change the code so that I
(the caller) always close the fd, but I was worried that if an
svn_error_t was thrown before it returned to me, the file would never
get closed. Thoughts?
>> + }
>> + return SVN_NO_ERROR;
>> +}
>> +
>> +
>> svn_error_t *
>> svn_fs_fs__lock (svn_lock_t **lock_p,
>> svn_fs_t *fs,
>> @@ -728,42 +1103,6 @@
>>
>>
>>
>> -struct dir_walker_baton
>> -{
>> - svn_fs_t *fs;
>> - apr_hash_t *locks;
>> -};
>> -
>> -
>> -static svn_error_t *
>> -locks_dir_walker (void *baton,
>> - const char *path,
>> - const apr_finfo_t *finfo,
>> - apr_pool_t *pool)
>> -{
>> - char *base_path;
>> - const char *rel_path;
>> - svn_lock_t *lock;
>> - struct dir_walker_baton *dir_baton;
>> - dir_baton = baton;
>> -
>> - /* Skip directories. */
>> - if (finfo->filetype == APR_DIR)
>> - return SVN_NO_ERROR;
>> -
>> - /* Get the repository-relative path for the lock. */
>> - SVN_ERR (base_path_to_lock_file (&base_path, dir_baton->fs, pool));
>> - rel_path = path + strlen (base_path);
>> -
>> - /* Get lock */
>> - SVN_ERR (svn_fs_fs__get_lock_from_path (&lock, dir_baton->fs,
>> - rel_path, pool));
>> -
>> - /* Stuff lock in hash, keyed on lock->path */
>> - apr_hash_set (dir_baton->locks, lock->path, APR_HASH_KEY_STRING,
>> lock);
>> -
>> - return SVN_NO_ERROR;
>> -}
>>
>>
>> svn_error_t *
>> @@ -773,9 +1112,9 @@
>> apr_pool_t *pool)
>> {
>> char *abs_path;
>> - struct dir_walker_baton baton;
>> apr_finfo_t finfo;
>> apr_status_t status;
>> + const char *digest_str;
>>
>> /* Make the hash that we'll return. */
>> *locks = apr_hash_make(pool);
>> @@ -783,18 +1122,23 @@
>> /* Compose the absolute/rel path to PATH */
>> SVN_ERR (abs_path_to_lock_file (&abs_path, fs, path, pool));
>>
>> + /* Strip any trailing slashes. */
>> + if (abs_path[strlen (abs_path) - 1] == '/')
>> + abs_path[strlen (abs_path) - 1] = '\0';
>
> The comment says "slashes" but it only removes a single slash.
Fixed.
> If strlen(abs_path) is not > 0 then accessing abs_path[-1] is dubious.
Added strlen.
>> +
>> status = apr_stat (&finfo, abs_path, APR_FINFO_TYPE, pool);
>>
>> /* If base dir doesn't exist, then we don't have any locks. */
>> if (APR_STATUS_IS_ENOENT (status))
>> return SVN_NO_ERROR;
>>
>> - baton.fs = fs;
>> - baton.locks = *locks;
>> -
>> - SVN_ERR (svn_io_dir_walk (abs_path, APR_FINFO_TYPE,
>> locks_dir_walker,
>> - &baton, pool));
>> -
>> + digest_str = make_digest (path, pool);
>> + abs_path = repository_abs_path (path, pool);
>> + SVN_ERR (abs_path_to_lock_file (&abs_path, fs, path, pool));
>> +
>> + /* Recursively walk lock "tree" */
>> + SVN_ERR (get_locks_under_path (locks, fs, abs_path, NULL, pool));
>> +
>> return SVN_NO_ERROR;
>> }
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org