On Fri, Apr 06, 2007 at 04:20:24PM -0700, Eric Gillespie wrote:
> OK, i've implemented my proposal, except i have not removed the
> 'dav' directory. It's part of the public svn_repos_t structure.
> Can i just leave that field there (and continue to fill it in),
> but just stop creating the directory?
>
svn_repos_t is an opaque structure - we can change it however we want.
We could stop creating the 'dav' directory, but then you'd have a
situation where 'svnadmin create' couldn't create a repository that
worked with an older mod_dav_svn, I think. You could create the
directory only for --pre-1.x-compatible repostiories, though.
Is there no reason you can't just continue to use the existing 'dav'
directory?
> Stop using apr_dbm to manage the DAV activities database which maps
> Make the location of this directory configurable with the new
I think these should probably be two separate patches.
I've not reviewed this thoroughly, but I've got some obvious questions:
- Does mod_dav restrict activity names to a filename-safe subset?
What's to stop someone performing a MKACTIVITY/MERGE against an activity
called '../../../../etc/passwd', for example (or 'NUL' or 'AUX'...)?
- Are we happy to decide that the activities are tied to a specific
mod_dav_svn version? i.e. we don't need to provide for compatibility
between mod_dav_svn 1.5 and activities created with mod_dav_svn 1.4.
- I find it rather confusing that you're referring to an 'activities db'
when it's not actually a database :-)
> Index: subversion/mod_dav_svn/activity.c
> ===================================================================
> +static const char *
> +_read_txn(const dav_svn_repos *repos, const char *pathname)
> {
> +
> + status = apr_stat(&finfo, pathname, APR_FINFO_SIZE, repos->pool);
> + _CHECK_STATUS;
> +
> + status = apr_file_open(&activity_file, pathname, APR_READ | APR_BUFFERED,
> + APR_OS_DEFAULT, repos->pool);
Opening the file in buffered mode when you're going to read everything
at once just wastes a 4k APR file buffer.
> +const char *
> +dav_svn__get_txn(const dav_svn_repos *repos, const char *activity_id)
> +{
> + const char *pathname;
> + pathname = svn_path_join(repos->activities_db, activity_id, repos->pool);
As I mentioned above, this seems unsafe to me given that the client
controls the activity id :-)
Regards,
Malcolm
- application/pgp-signature attachment: stored
Received on Tue Apr 10 16:14:10 2007