Malcolm Rowe <malcolm-svn-dev@farside.org.uk> writes:
> On Mon, Apr 16, 2007 at 02:22:18PM -0700, Eric Gillespie wrote:
> > I've updated the patch to account for Malcolm's and Ben's
> > comments. The activity file is created as a temporary file and
> > then renamed into place with the MD5-encoded file name, and
> > read_txn (renamed :) handles ESTALE. The transaction name is now
> > terminated with a newline.
> >=20
>
> Thanks - nearly there, see comments below.
Whew! I've been busy with so many things, but have finally
gotten back to this. Thanks for your patience!
> > +#ifdef ESTALE
> > + if (APR_TO_OS_ERROR(err->apr_err) =3D=3D ESTALE)
> > + continue;
> > +#endif
> > + /* ### let's just assume that any error means the
> > + ### activity/transaction doesn't exist */
> > return NULL;
>
> You just leaked 'err'.
Oops, that stuff was way busted. I was only half-finished with
the return stuff at the end, and these were meant to be
breaks... Should all be fixed now.
> > + 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
untraditional rename(2) semantics. If it weren't for that, we'd
be returning out-of-date data and not even know it. That still
may happen, if the data is replaced just after we close it;
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.
For other errors, we do return NULL. See the ### comment for that.
> I'm curious why (here and elsewhere) we're calling direct into APR
> rather than using the normal Subversion calls - I'd have expected
> something like this:
Yeah, that was a strange move to make, wasn't it? Crack
overdose, perhaps. Fixed.
> > + repos->fs_path,
> > + ACTIVITY_DB,
> > + NULL),
> > + repos->pool);
> > + if (err !=3D NULL)
> > + return dav_svn__convert_err(err, HTTP_INTERNAL_SERVER_ERROR,
> > + "could not open files.",
>
> Seems like an odd error message to choose?
Too much copy and paste.
[[[
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 MD5 checksums of activity
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'.
(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 .
(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.
+*/
+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. */
+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);
+ 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,
+ 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);
+ if (err)
+ {
+#ifdef ESTALE
+ if (APR_TO_OS_ERROR(err->apr_err) == ESTALE)
+ continue;
+#endif
+ break;
+ }
+
+ err = svn_io_file_close(activity_file, iterpool);
+#ifdef ESTALE
+ if (err)
+ {
+ if (APR_TO_OS_ERROR(err->apr_err) == ESTALE)
+ {
+ /* No retry, just completely ignore this ESTALE. */
+ svn_error_clear(err);
+ err = SVN_NO_ERROR;
+ }
+ }
+#endif
+
+ /* We have a txn_name or had a non-ESTALE close failure; either
+ way, we're finished. */
+ break;
}
+ svn_pool_destroy(iterpool);
- apr_dbm_close(dbm);
+ /* ### let's just assume that any error means the
+ ### activity/transaction doesn't exist */
+ if (err)
+ {
+ svn_error_clear(err);
+ return NULL;
+ }
return txn_name;
}
+const char *
+dav_svn__get_txn(const dav_svn_repos *repos, const char *activity_id)
+{
+ const char *pathname;
+ pathname = svn_path_join_many(repos->pool, repos->fs_path, ACTIVITY_DB,
+ escape_activity(activity_id, repos->pool),
+ NULL);
+ return read_txn(repos, pathname);
+}
+
+
dav_error *
dav_svn__delete_activity(const dav_svn_repos *repos, const char *activity_id)
{
dav_error *err = NULL;
- apr_dbm_t *dbm;
- apr_status_t status;
const char *pathname;
- apr_datum_t key;
- apr_datum_t value;
svn_fs_txn_t *txn;
const char *txn_name;
svn_error_t *serr;
@@ -89,23 +158,12 @@
404. If the transaction is not present or is immutable, return a
204. For all other failures, return a 500. */
- /* Open the activities database. */
- pathname = svn_path_join(repos->fs_path, ACTIVITY_DB, repos->pool);
- status = apr_dbm_open(&dbm, pathname, APR_DBM_READWRITE,
- APR_OS_DEFAULT, repos->pool);
- if (status != APR_SUCCESS)
- return dav_new_error(repos->pool, HTTP_NOT_FOUND, 0,
- "could not open activities database.");
-
- /* Get the activity from the activity database. */
- key.dptr = (char *)activity_id;
- key.dsize = strlen(activity_id) + 1; /* null-term'd */
- status = apr_dbm_fetch(dbm, key, &value);
- if (status == APR_SUCCESS)
- txn_name = value.dptr;
- else
+ pathname = svn_path_join_many(repos->pool, repos->fs_path, ACTIVITY_DB,
+ escape_activity(activity_id, repos->pool),
+ NULL);
+ txn_name = read_txn(repos, pathname);
+ if (txn_name == NULL)
{
- apr_dbm_close(dbm);
return dav_new_error(repos->pool, HTTP_NOT_FOUND, 0,
"could not find activity.");
}
@@ -131,7 +189,7 @@
err = dav_svn__convert_err(serr, HTTP_INTERNAL_SERVER_ERROR,
"could not open transaction.",
repos->pool);
- goto cleanup;
+ return err;
}
}
else
@@ -142,24 +200,18 @@
err = dav_svn__convert_err(serr, HTTP_INTERNAL_SERVER_ERROR,
"could not abort transaction.",
repos->pool);
- goto cleanup;
+ return err;
}
}
}
-
+
/* Finally, we remove the activity from the activities database. */
- status = apr_dbm_delete(dbm, key);
- if (status)
- {
- err = dav_new_error(repos->pool, HTTP_INTERNAL_SERVER_ERROR, 0,
- "unable to remove activity.");
- goto cleanup;
- }
+ serr = svn_io_remove_file(pathname, repos->pool);
+ if (serr)
+ err = dav_svn__convert_err(serr, HTTP_INTERNAL_SERVER_ERROR,
+ "unable to remove activity.",
+ repos->pool);
- cleanup:
- apr_dbm_freedatum(dbm, value);
- apr_dbm_close(dbm);
-
return err;
}
@@ -169,40 +221,69 @@
const char *activity_id,
const char *txn_name)
{
- apr_dbm_t *dbm;
- apr_status_t status;
- const char *pathname;
- apr_datum_t key;
- apr_datum_t value;
+ const char *final_path, *tmp_path, *activity_contents;
+ svn_error_t *err;
+ apr_file_t *activity_file;
- 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);
- if (status != APR_SUCCESS)
+ /* 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);
+ if (err != NULL)
+ return dav_svn__convert_err(err, HTTP_INTERNAL_SERVER_ERROR,
+ "could not initialize activity db.",
+ repos->pool);
+
+ final_path = svn_path_join_many(repos->pool, repos->fs_path, ACTIVITY_DB,
+ escape_activity(activity_id, repos->pool),
+ NULL);
+ err = svn_io_open_unique_file2(&activity_file, &tmp_path, final_path,
+ ".tmp", svn_io_file_del_none, repos->pool);
+ if (err)
{
- svn_error_t *serr = svn_error_wrap_apr(status, "Can't open activity db");
-
+ svn_error_t *serr = svn_error_quick_wrap(err, "Can't open activity db");
return dav_svn__convert_err(serr, HTTP_INTERNAL_SERVER_ERROR,
- "could not open dbm files.",
+ "could not open files.",
repos->pool);
}
- key.dptr = (char *)activity_id;
- key.dsize = strlen(activity_id) + 1; /* null-term'd */
- value.dptr = (char *)txn_name;
- value.dsize = strlen(txn_name) + 1; /* null-term'd */
- status = apr_dbm_store(dbm, key, value);
- apr_dbm_close(dbm);
- if (status != APR_SUCCESS)
+ activity_contents = apr_psprintf(repos->pool, "%s\n%s\n",
+ txn_name, activity_id);
+ err = svn_io_file_write_full(activity_file, activity_contents,
+ strlen(activity_contents), NULL, repos->pool);
+ if (err)
{
- svn_error_t *serr =
- svn_error_wrap_apr(status, "Can't close activity db");
+ svn_error_t *serr = svn_error_quick_wrap(err,
+ "Can't write to activity db");
+ /* Try to remove the tmp file, but we already have an error... */
+ svn_error_clear(svn_io_file_close(activity_file, repos->pool));
+ svn_error_clear(svn_io_remove_file(tmp_path, repos->pool));
return dav_svn__convert_err(serr, HTTP_INTERNAL_SERVER_ERROR,
- "could not close dbm files.",
+ "could not write files.",
repos->pool);
}
+ err = svn_io_file_close(activity_file, repos->pool);
+ if (err)
+ {
+ svn_error_clear(svn_io_remove_file(tmp_path, repos->pool));
+ return dav_svn__convert_err(err, HTTP_INTERNAL_SERVER_ERROR,
+ "could not close files.",
+ repos->pool);
+ }
+
+ err = svn_io_file_rename(tmp_path, final_path, repos->pool);
+ if (err)
+ {
+ svn_error_clear(svn_io_remove_file(tmp_path, repos->pool));
+ return dav_svn__convert_err(err, HTTP_INTERNAL_SERVER_ERROR,
+ "could not replace files.",
+ repos->pool);
+ }
+
return NULL;
}
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Fri Apr 27 02:20:39 2007