On 4/1/15 5:28 AM, Johan Corveleyn wrote:
> On Wed, Apr 1, 2015 at 1:21 PM, Philip Martin
> <philip.martin_at_wandisco.com> wrote:
>> Yes, I believe so. mod_dav should avoid the walk when the callback
>> result for every path in the walk is known in advance. I spoke to Ben
>> yesterday and he said he has an httpd patch that is just about ready.
So first of all this DAV code has two somewhat overlapping purposes. It
handles If headers and it handles locks. There is some logic to this since
lock tokens can be encoded in If headers. However, you can also encode ETags
as conditions in If headers. For example a DAV client could use an If header
to only copy something if it had not been modified since it was last aware.
Until r1476604 (released in 2.2.25 and 2.4.5) mod_dav didn't handle these
properly for the copy source.
It is necessary to do the walk to handle locks properly since we have to look
for all the locks and then make sure we received all the lock tokens for them.
It is not necessary to walk the tree to handle ETag conditions since we are
given the paths and can just handle them one by one.
So what I've been working on is trying to separate these two things out from
each other. So that locks and ETags can be handled separately and so that we
only walk the tree if necessary to check locks.
I'm not quite done with this work.
I'm considering committing the following to httpd today given the attention
this has gotten:
[[[
Index: modules/dav/main/util.c
===================================================================
--- modules/dav/main/util.c (revision 1671097)
+++ modules/dav/main/util.c (working copy)
@@ -1595,8 +1595,10 @@
}
}
- /* (1) Validate the specified resource, at the specified depth */
- if (resource->exists && depth > 0) {
+ /* (1) Validate the specified resource, at the specified depth.
+ * Avoid the walk there is no if_header and we aren't planning
+ * to modify this resource. */
+ if (resource->exists && depth > 0 && !(!if_header && flags &
DAV_VALIDATE_NO_MODIFY)) {
dav_walker_ctx ctx = { { 0 } };
dav_response *multi_status;
]]]
This is worse than the workarounds you have already come up with from an SVN
only perspective. The workarounds you have come up with would prevent the use
of ETag If headers on the copy source, but SVN clients don't use those anyway.
The above patch would not break ETag If headers but SVN would still have the
penalty of the walk if the client is sending lock tokens for the destination
(since SVN sends those as If headers). But I think it would at least hide the
issue from more users in the meantime.
For SVN only use you can remove the test for the if_header (which I'm not
comfortable committing to httpd since it just moves the breakage to mod_dav_fs):
if (resource->exists && depth > 0 && !(flags & DAV_VALIDATE_NO_MODIFY))
> That's great news.
>
> Just to be clear: the subject says "mod_dav 2.4.6", but be advised
> that 2.2.25 and higher are also affected, so I hope that an httpd fix
> will be backported to 2.2.x as well.
That's my intent. Whatever is done will be backported to 2.2.x and 2.4.x.
I'll keep an eye open and make sure that the above patch gets committed and
backported to 2.2.x and 2.4.x if I don't have the more complete refactoring
done to at least try and limit the situations this comes up in
Received on 2015-04-03 20:24:03 CEST