On 12/31/2007 02:26 AM, Nick Kew wrote:
> On Sun, 30 Dec 2007 09:22:28 +0800
> Michael Clark <[EMAIL PROTECTED]> wrote:
>
>> I'm getting a segfault here in mod_dav from trunk
>
> Oops ... more haste less speed:-(
>
> I attach a revised patch (against current trunk).
> This is completely untested; just posting before going to bed,
> in case anyone feels like picking it up. If not, I'll try
> and find time to revisit it tomorrow.
>
I think the following patch against trunk which is based on yours
does it better, because:
- It does not set the ETag header permanently as setting it may not
be desired for all responses. If we want to do this for more methods
as we currently do we should do this explicitly in dav_method_* in
mod_dav.c.
- Handles the case that hooks->getetag returns an empty ETag ("") like
dav_fs_getetag does for non existing resources.
- Does remove the conditional between calling dav_meets_conditions and
ap_meets_conditions. To be honest I do not understand why we should not
call dav_meets_conditions in any case. Especially in the '*' case this
make sense IMHO as we do not need to do a comparison to an existing
ETag of the resource in this case. It is only of interest in this case
whether the resource exists and not what its ETag is.
Passed perl test framework and litmus.
Regards
Rüdiger
Index: modules/dav/main/util.c
===================================================================
--- modules/dav/main/util.c (Revision 607709)
+++ modules/dav/main/util.c (Arbeitskopie)
@@ -1467,6 +1467,8 @@
dav_buffer work_buf = { 0 };
dav_response *new_response;
int resource_state;
+ const char *etag;
+ int set_etag = 0;
#if DAV_DEBUG
if (depth && response == NULL) {
@@ -1484,17 +1486,28 @@
*response = NULL;
/* Set the ETag header required by dav_meets_conditions() */
- if ((err = (*resource->hooks->set_headers)(r, resource)) != NULL) {
- return dav_push_error(r->pool, err->status, 0,
- "Unable to set up HTTP headers.",
- err);
+ etag = apr_table_get(r->headers_out, "ETag");
+ if (!etag) {
+ etag = (*resource->hooks->getetag)(resource);
+ if (etag && *etag) {
+ apr_table_set(r->headers_out, "ETag", etag);
+ set_etag = 1;
+ }
}
-
- resource_state = dav_get_resource_state(r, resource);
/* Do the standard checks for conditional requests using
* If-..-Since, If-Match etc */
- if ((result = dav_meets_conditions(r, resource_state)) != OK) {
- /* ### fix this up... how? */
+ resource_state = dav_get_resource_state(r, resource);
+ result = dav_meets_conditions(r, resource_state);
+ if (set_etag) {
+ /*
+ * If we have set an ETag to headers out above for
+ * dav_meets_conditions() revert this here as we do not want to set
+ * the ETag in responses to requests with methods where this might not
+ * be desired.
+ */
+ apr_table_unset(r->headers_out, "ETag");
+ }
+ if (result != OK) {
return dav_new_error(r->pool, result, 0, NULL);
}
