You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by kf...@collab.net on 2004/10/04 15:33:12 UTC
Re: svn commit: r11211 - trunk/subversion/mod_dav_svn
bliss@tigris.org writes:
> Modified:
> trunk/subversion/mod_dav_svn/liveprops.c
> Log:
> Fix the horrible ra_dav->get_dir performance introduced in 1.1.0-rc4.
>
> ...also to be known as...
>
> Fix the horrible ls performance over ra_dav introduced in 1.1.0-rc4.
>
> If svn_repos_fs_revision_prop is supplied a path read authz function,
> it will run that function on all changed paths for the revision to
> verify that the revision is readable. That is a very expensive
> operation, but the best we can do when we simply want to read a
> revision property.
>
> The liveprops code in ra_dav already has a path and wants to read a
> couple of revision properties for the last changed revision of those
> paths. By letting mod_dav_svn checking the path first, and not
> letting svn_repos_fs_revision_prop do any authz we save an enormous
> amount of work.
>
> One test case (which I'm too lazy to describe how to set up here, but
> the client operation was a non-recursive ls of a directory with 412
> files) went down from six minutes to three seconds with this change.
>
> * subversion/mod_dav_svn/liveprops.c
> (dav_svn_insert_prop, dav_svn_get_last_modified_time): Call
> dav_svn_authz_read on our path, and pass NULL as path read authz
> callback to svn_repos_fs_revision_prop.
Quick sanity check: the reason this is okay is because of the
paragraph in section 5 of notes/authz_policy.txt, that says:
* If a revision has a mixture of readable/unreadable
changed-paths, then all revprops are unreadable, except for
svn:author and svn:date. All revprops are unwritable.
right? The only properties we're grabbing here are svn:author and
svn:date, exactly the two that are allowed even if there are some
other unreadable paths in the revision. Therefore all we need to know
is that *at least* one path is readable, and we do know that.
Am I on the right track?
Code comments below:
> --- trunk/subversion/mod_dav_svn/liveprops.c (original)
> +++ trunk/subversion/mod_dav_svn/liveprops.c Sun Oct 3 13:54:53 2004
> @@ -117,7 +117,6 @@
> apr_pool_t *p = resource->info->pool;
> const dav_liveprop_spec *info;
> int global_ns;
> - dav_svn_authz_read_baton arb;
> svn_error_t *serr;
>
> /*
> @@ -131,10 +130,6 @@
> if (!resource->exists)
> return DAV_PROP_INSERT_NOTSUPP;
>
> - /* Build a baton for path-based authz function (dav_svn_authz_read) */
> - arb.r = resource->info->r;
> - arb.repos = resource->info->repos;
> -
> /* ### we may want to respond to DAV_PROPID_resourcetype for PRIVATE
> ### resources. need to think on "proper" interaction with mod_dav */
>
> @@ -188,6 +183,8 @@
> {
> svn_revnum_t committed_rev = SVN_INVALID_REVNUM;
> svn_string_t *last_author = NULL;
> + dav_svn_authz_read_baton arb;
> + svn_boolean_t allowed;
>
> /* ### for now, our global VCC has no such property. */
> if (resource->type == DAV_RESOURCE_TYPE_PRIVATE
> @@ -222,13 +219,32 @@
> {
> return DAV_PROP_INSERT_NOTSUPP;
> }
> -
> - /* Get the date property of the created revision. */
Heh, it appears the old comment said "date" where it meant "author",
but...
> +
> + /* Check if we have access to this path and return NOTDEF if
> + we don't. */
> + arb.r = resource->info->r;
> + arb.repos = resource->info->repos;
> + serr = dav_svn_authz_read(&allowed,
> + resource->info->root.root,
> + resource->info->repos_path,
> + &arb, p);
> + if (serr)
> + {
> + /* ### what to do? */
> + svn_error_clear(serr);
> + value = "###error###";
> + break;
> + }
> + if (! allowed)
> + return DAV_PROP_INSERT_NOTDEF;
> +
> + /* Get the date property of the created revision. The authz is
> + already performed, so we don't need to do it here too. */
> serr = svn_repos_fs_revision_prop(&last_author,
> resource->info->repos->repos,
> committed_rev,
> SVN_PROP_REVISION_AUTHOR,
> - dav_svn_authz_read, &arb, p);
> + NULL, NULL, p);
...your new comment also says "date" when in fact we're fetching the
author.
The comment should maybe also state the at-least-one-readable-path
policy for author, to remind the programmer of why this is okay.
> if (serr != NULL)
> {
> /* ### what to do? */
> @@ -667,13 +683,27 @@
> svn_error_t *serr;
> apr_time_t timeval_tmp;
> dav_svn_authz_read_baton arb;
> -
> - arb.r = resource->info->r;
> - arb.repos = resource->info->repos;
> + svn_boolean_t allowed;
>
> if ((datestring == NULL) && (timeval == NULL))
> return 0;
>
> + /* Check if we have access to this path and return NOTDEF if we
> + don't. */
> + arb.r = resource->info->r;
> + arb.repos = resource->info->repos;
> + serr = dav_svn_authz_read(&allowed,
> + resource->info->root.root,
> + resource->info->repos_path,
> + &arb, pool);
> + if (serr)
> + {
> + svn_error_clear(serr);
> + return 1;
> + }
> + if (! allowed)
> + return 1;
> +
This is exactly the same code as before, right? Maybe worth
abstracting?
> if (resource->baselined && resource->type == DAV_RESOURCE_TYPE_VERSION)
> {
> /* A baseline URI. */
> @@ -698,12 +728,13 @@
> return 1;
> }
>
> - /* Get the svn:date property of the CR */
> + /* Get the svn:date property of the CR. The authz is already
> + performed, so we don't need to do it here too. */
> serr = svn_repos_fs_revision_prop(&committed_date,
> resource->info->repos->repos,
> committed_rev,
> SVN_PROP_REVISION_DATE,
> - dav_svn_authz_read, &arb,
> + NULL, NULL,
> pool);
> if (serr != NULL)
> {
Here "date" is correct :-).
But this comment also could maybe state the at-least-one-readable-path
policy for dates, as a reminder. Most people won't know the authz
policy by heart.
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r11211 - trunk/subversion/mod_dav_svn
Posted by kf...@collab.net.
Tobias Ringström <to...@ringstrom.mine.nu> writes:
> I've expanded and corrected the comments you pointed out in your
> review, but I did not abstract the duplicated code because of the
> large changes to the code which would only have made reviewing
> harder. I'll do that in trunk well after 1.0.x and 1.1.x are fixed.
Sounds great! Thanks, Tobias.
-K
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r11211 - trunk/subversion/mod_dav_svn
Posted by Tobias Ringström <to...@ringstrom.mine.nu>.
I've expanded and corrected the comments you pointed out in your review,
but I did not abstract the duplicated code because of the large changes
to the code which would only have made reviewing harder. I'll do that in
trunk well after 1.0.x and 1.1.x are fixed.
Again, thanks for the review!
/Tobias
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r11211 - trunk/subversion/mod_dav_svn
Posted by Tobias Ringström <to...@ringstrom.mine.nu>.
Thanks for reviewing the patch, Karl!
kfogel@collab.net wrote:
>Quick sanity check: the reason this is okay is because of the
>paragraph in section 5 of notes/authz_policy.txt, that says:
>
> * If a revision has a mixture of readable/unreadable
> changed-paths, then all revprops are unreadable, except for
> svn:author and svn:date. All revprops are unwritable.
>
>right? The only properties we're grabbing here are svn:author and
>svn:date, exactly the two that are allowed even if there are some
>other unreadable paths in the revision. Therefore all we need to know
>is that *at least* one path is readable, and we do know that.
>
>Am I on the right track?
>
>
Absolutely.
>...your new comment also says "date" when in fact we're fetching the
>author.
>
>The comment should maybe also state the at-least-one-readable-path
>policy for author, to remind the programmer of why this is okay.
>
>
Right, I'll add that.
>This is exactly the same code as before, right? Maybe worth
>abstracting?
>
>
I though it was too few lines, but I'll take a look at it again.
>Here "date" is correct :-).
>
>But this comment also could maybe state the at-least-one-readable-path
>policy for dates, as a reminder. Most people won't know the authz
>policy by heart.
>
>
Roger that!
Food now, patch to fix comments soon... :-)
/Tobias
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org