Re: [PATCH] Is mod_dav_svn safe for use in a threaded MPM?

Hi Eric,

Some minor (mostly nitpicking) comments below, one of which (the last) I
think you should definitely fix, but +1 to commit whether or not you
change anything else.

On Thu, Apr 26, 2007 at 05:19:29PM -0700, Eric Gillespie wrote:
> > > + err =3D svn_io_file_close(activity_file, iterpool);
> > > + if (err && err->apr_err !=3D APR_SUCCESS)
> > > + {
> > > +#ifdef ESTALE
> > > + if (APR_TO_OS_ERROR(err->apr_err) !=3D ESTALE)
> > > +#endif
> > > + return NULL;
> >
> > Why have different behaviour for ESTALE/non-ESTALE? While I can't think
> > of a valid non-ESTALE return, it seems like we should treat them the
> > same - whether that means returning what we have or returning NULL, I'm
> > not sure. (especially as ESTALE means that someone just overwrote the
> > transaction name, so we know we have stale data).
>
> We know we have stale data at this time only on NFS < 3, with its

(We don't support NFSv2 - it doesn't have sane locking)

> there's no way around it, but i don't see that it matters. I
> think it's best to ignore ESTALE and pretend we didn't notice,
> just as on local storage.

Okay, that sounds like a good enough excuse. Perhaps add a comment?
(Oh, you did - excellent :-))

> [[[
> Stop using apr_dbm to manage the DAV activities database which
s/which/that.
> maps DAV activity ids to Subversion transaction names. Instead,
> use a simple directory structure, with MD5 checksums of activity
s/,//.
> ids as file names and transaction names as the first line.
> Include the activity id on a second line, as a diagnostic.
>
> * subversion/mod_dav_svn/activity.c
> (ACTIVITY_DB): Change to 'dav/activities.d' so as not to conflict
> with the DBM file 'dav/activities'.
(Incidentally, I think convention is to indent follow-on lines like this
in log messages.)
> (escape_activity): New helper to escape activity for safe use as a
> file name.
> (read_txn): New helper to return the txn from an activity file.
> (dav_svn__get_txn): Make this a simple wrapper around read_txn .
Weird space before '.'.
> (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 24594)
> +++ subversion/mod_dav_svn/activity.c (working copy)
> @@ -16,11 +16,16 @@
> * ====================================================================
> */
>
> -#include <apr_dbm.h>
> +#include <errno.h>
>
> +#include <apr_md5.h>
> +
> #include <httpd.h>
> #include <mod_dav.h>
>
> +#include "svn_error.h"
> +#include "svn_io.h"
> +#include "svn_md5.h"
> #include "svn_path.h"
> #include "svn_fs.h"
> #include "svn_repos.h"
> @@ -28,59 +33,123 @@
> #include "dav_svn.h"
>
>
> -#define ACTIVITY_DB "dav/activities"
> +#define ACTIVITY_DB "dav/activities.d"
>
>
> -const char *
> -dav_svn__get_txn(const dav_svn_repos *repos, const char *activity_id)
> +/* Escape the activity id to be safely usable as a filename. Simply
> + returns the MD5 checksum of the id.

Should use our internal doc-comment style (e.g. ACTIVITY_ID).

> +*/
> +static const char *
> +escape_activity(const char *activity_id, apr_pool_t *pool)
> {
> - apr_dbm_t *dbm;
> - apr_status_t status;
> - const char *pathname;
> - apr_datum_t key;
> - apr_datum_t value;
> - const char *txn_name = NULL;
> + unsigned char digest[APR_MD5_DIGESTSIZE];
> + apr_md5(digest, activity_id, strlen(activity_id));
> + return svn_md5_digest_to_cstring_display(digest, pool);
> +}
>
> - 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)
> +
> +/* FSFS transaction ids:
> + 19 bytes for svn_revnum_t (room for 32 or 64 bit values)
> + + 1 byte for '-'
> + + 5 bytes (up to 99999)
> + + 1 terminating null / newline
> + = 26 bytes
> + BDB transaction ids are base-36, bounded by bdb MAX_KEY_SIZE, but
> + realistically, 26 bytes ought to be enough.
> + */
> +#define TXN_LEN 26
> +
> +/* Return the transaction name from the file pathname. */
"Return the transaction name of the activity stored in file PATHNAME in
repos REPOS, or NULL if PATHNAME cannot be read for any reason.", or similar?
> +static const char *
> +read_txn(const dav_svn_repos *repos, const char *pathname)
> +{
> + apr_file_t *activity_file;
> + apr_pool_t *iterpool = svn_pool_create(repos->pool);

Still unsure about this lifetime of repos->pool, but as you pointed out
before, that's not a new problem.

> + apr_size_t len;
> + svn_error_t *err = SVN_NO_ERROR;
> + char *txn_name = apr_palloc(repos->pool, TXN_LEN);
> + int i;
> +
> + /* Try up to 10 times to read the txn name, retrying on ESTALE
> + (stale NFS file handle because of dav_svn__store_activity
> + renaming the activity file into place).
> + */
> + for (i = 0; i < 10; i++)
> {
> - /* ### let's just assume that any error means the DB doesn't exist,
> - ### therefore, the activity/transaction doesn't exist */
> - return NULL;
> - }
> + svn_error_clear(err);
> + svn_pool_clear(iterpool);
>
> - key.dptr = (char *)activity_id;
> - key.dsize = strlen(activity_id) + 1; /* null-term'd */
> - if (apr_dbm_exists(dbm, key))
> - {
> - status = apr_dbm_fetch(dbm, key, &value);
> - if (status != APR_SUCCESS)
> + err = svn_io_file_open(&activity_file, pathname, APR_READ | APR_BUFFERED,

line length?

> + APR_OS_DEFAULT, iterpool);
> + if (err)
> {
> - /* ### again: assume failure means it doesn't exist */
> - apr_dbm_close(dbm);
> - return NULL;
> +#ifdef ESTALE
> + if (APR_TO_OS_ERROR(err->apr_err) == ESTALE)
> + /* Retry on ESTALE... */
> + continue;
> +#endif
> + /* ...else bail. */
> + break;
> }
> - txn_name = apr_pstrdup(repos->pool, value.dptr); /* null-term'd */
> - apr_dbm_freedatum(dbm, value);
> +
> + len = TXN_LEN;
> + err = svn_io_read_length_line(activity_file, txn_name, &len, NULL);

Using the global pool for allocations? I don't think so :-)
(even if, as here, the pool is only used for error reporting)

Rest looks good to me.

Regards,
Malcolm