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
Received on Wed Jan 12 17:12:41 2005