On Tue, Apr 10, 2007 at 11:46:59AM -0700, Eric Gillespie wrote:
> Hrm, well, OK. Here's the updated patch to handle just the first
> part. Tests running over this now, seem to be doing fine.
>
> [[[
> Stop using apr_dbm to manage the DAV activities database which maps
> DAV activity ids to Subversion transaction names. Instead, use a
> simple directory structure, with activity ids as file names and
> transaction names as file contents.
>
> * subversion/mod_dav_svn/activity.c
> (ACTIVITY_DB): Change to 'dav/activities.d' so as not to conflict
> with the DBM file 'dav/activities'.
> (_read_txn): New helper to return the txn from an activity file.
Any reason this uses a leading underscore? That's not our usual naming
convention for static functions. (Have you been doing too much Python
recently? :-)
> (dav_svn__get_txn): Make this a simple wrapper around _read_txn .
> (dav_svn__delete_activity): Use _read_txn to get the transaction
> name; delete the file from the activities directory.
> (dav_svn__store_activity): Create the activities directory if it
> does not exist and create the activity file.
> ]]]
>
> Index: subversion/mod_dav_svn/activity.c
> ===================================================================
> --- subversion/mod_dav_svn/activity.c (revision 24508)
> +++ subversion/mod_dav_svn/activity.c (working copy)
> Index: activity.c
> ===================================================================
> --- activity.c (revision 24508)
> +++ activity.c (working copy)
> -const char *
> -dav_svn__get_txn(const dav_svn_repos *repos, const char *activity_id)
> +static const char *
> +_read_txn(const dav_svn_repos *repos, const char *pathname)
> {
> - apr_dbm_t *dbm;
> apr_status_t status;
> - const char *pathname;
> - apr_datum_t key;
> - apr_datum_t value;
> - const char *txn_name = NULL;
> + apr_finfo_t finfo;
> + char *txn_name = NULL;
> + apr_file_t *activity_file;
> + apr_size_t bytes_read;
>
> - pathname = svn_path_join(repos->fs_path, ACTIVITY_DB, repos->pool);
> - status = apr_dbm_open(&dbm, pathname, APR_DBM_READONLY,
> - APR_OS_DEFAULT, repos->pool);
> - if (status != APR_SUCCESS)
> + /* ### let's just assume that any error means the
> + ### activity/transaction doesn't exist */
Does the '###' imply that we might want to consider doing something
different in the future?
> +# define _CHECK_STATUS if (status != APR_SUCCESS) return NULL
Again with the underscores! :-)
> +
> + status = apr_stat(&finfo, pathname, APR_FINFO_SIZE, repos->pool);
> + _CHECK_STATUS;
> +
> + status = apr_file_open(&activity_file, pathname, APR_READ,
> + APR_OS_DEFAULT, repos->pool);
What's the lifetime of repos->pool? If it's too long, we could
potentially stack up file handles and...
> + _CHECK_STATUS;
> +
> + txn_name = apr_palloc(repos->pool, finfo.size + 1); /* null-term'd */
... other stuff. I'm sure it's fine, it's just not immediately
obvious.
> + status = apr_file_read_full(activity_file, txn_name, finfo.size, &bytes_read);
There's a race between the stat() and open() - if the activity was
reassigned to a new transaction with a longer name between the
two calls, we'd read a partial transaction name. Perhaps either
terminate the transaction name with something ('\n'?) and check for
that, or simply try to read an extra byte (and keep the length/read
verification), or maybe just use fstat() [which might even be more
efficient anyway, and is certainly easier to understand].
> + _CHECK_STATUS;
> + if (bytes_read != finfo.size)
> {
Worth checking for zero-sized files as well?
> - /* ### let's just assume that any error means the DB doesn't exist,
> - ### therefore, the activity/transaction doesn't exist */
> return NULL;
> }
> + status = apr_file_close(activity_file);
> + _CHECK_STATUS;
> +# undef _CHECK_STATUS
> @@ -169,37 +157,56 @@
> const char *activity_id,
> const char *txn_name)
> {
> - apr_dbm_t *dbm;
> + const char *pathname;
> + svn_error_t *err;
> apr_status_t status;
> - const char *pathname;
> - apr_datum_t key;
> - apr_datum_t value;
> + apr_file_t *activity_file;
> + apr_size_t nbytes, bytes_written;
>
> - pathname = svn_path_join(repos->fs_path, ACTIVITY_DB, repos->pool);
> - status = apr_dbm_open(&dbm, pathname, APR_DBM_RWCREATE,
> - APR_OS_DEFAULT, repos->pool);
> + /* Create activities directory if it does not yet exist. */
> + err = svn_io_make_dir_recursively(svn_path_join_many(repos->pool,
> + repos->fs_path,
> + ACTIVITY_DB,
> + NULL),
> + repos->pool);
(At the moment, this could just be svn_io_dir_make(), but I know you're
planning to change this to make the activities directory movable, so
don't worry.)
> + if (err != NULL)
> + return dav_svn__convert_err(err, HTTP_INTERNAL_SERVER_ERROR,
> + "could not open files.",
> + repos->pool);
> +
> + pathname = svn_path_join_many(repos->pool, repos->fs_path, ACTIVITY_DB,
> + activity_id, NULL);
> + status = apr_file_open(&activity_file, pathname, APR_WRITE | APR_CREATE,
> + APR_OS_DEFAULT, repos->pool);
This should write to a temporary file and rename() so that readers don't
see partially-written state. Of course, that also means that the code
should deal with ESTALE as well.
Regards,
Malcolm
- application/pgp-signature attachment: stored
Received on Wed Apr 11 00:20:54 2007