There seems to be some consensus that we should add a key to r->notes
as a temporary fix ( at least till 2.4 ). Hence, I'm attaching a patch
- ap_meets_conditions_fixes.patch, with the appropriate fixes to
ap_meets_conditions() using "resource-exists" key for this purpose.
To be sure that we do not break anything, we would want
ap_meets_conditions() to behave as it does now when the key is not
set. This implies that the value corresponding to the key needs to
pass two states - one to signify resource exists and another for
resource does not exist ( using "Y" and "N" as the corresponding
values here ). I used "resource-exists" as the key instead of
"NON_EXTANT_RESOURCE" or "NO_RESOURCE" as suggested by Chris Darroch,
to avoid double negatives.
Also attaching a corresponding patch to mod_dav ( mod_dav_fixes.patch
) which sets "resource-exists" appropriately and also the Etag header
by calling resource->hooks->set_headers() before calling
ap_meets_conditions()
Again, the patches are against 2.2.6 and I've tested them against
mod_dav_fs w/ DEBUG_GET_HANDLER set to 1.
Suggestions ? Comments ?
- Paritosh Shah.
On 10/18/07, Chris Darroch <[EMAIL PROTECTED]> wrote:
> Nick Kew wrote:
>
> > My vote goes to r->notes.
> >
> > Anything else relies on something with semantic significance that'll
> > risk breaking random things.
> >
> > For the future (e.g. 2.4), we could review other options. For example,
> > abstract out r->finfo to a void* with an API for inspecting resources
> > including but not limited to those visible in the filesystem.
>
> Sounds good; maybe an r->notes key named "NON_EXTANT_RESOURCE" or
> "NO_RESOURCE" or some such?
>
> > Feel free to ignore my totally half-baked ideas, but mod_dav is not
> > the only area where we have issues, with ETags. There's also the
> > whole question of negotiated content to consider, and I'm still confused
> > by the problems that still surround PR#39727. If we're reworking
> > ETag logic, we should take the opportunity to deal with this, too.
> > Can we incorporate some kind of tokenisation into ETags that will
> > express dependence on negotiation?
> >
> > For example, Henrik Nordstrom suggests ";gzip". If we expand that to,
> > say, ";CE=gzip", then we can start to apply semantics to decoding the
> > Etag, and determine equivalence. So in that case, we could determine
> > that the (uncompressed) file does indeed match the ETag. Not sure if
> > there's anything we might do with other negotiated headers, but maybe
> > we could leave that open, providing a hook that something like
> > mod_negotiation might someday use.
>
> I confess I've been following this discussion at an abstract
> level only (at best). I don't think, though, that it directly affects
> any of the several bugs I'm hoping to tackle, if I understand the
> issue correctly. But I don't claim to fully understand the details of
> mod_negiotiation.
>
> Chris.
>
> --
> GPG Key ID: 366A375B
> GPG Key Fingerprint: 485E 5041 17E1 E2BB C263 E4DE C8E3 FA36 366A 375B
>
>
Index: modules/http/http_protocol.c
===================================================================
--- modules/http/http_protocol.c (revision 5021)
+++ modules/http/http_protocol.c (working copy)
@@ -261,6 +261,7 @@
apr_time_t tmp_time;
apr_int64_t mtime;
int not_modified = 0;
+ const char *resource_exists;
/* Check for conditional requests --- note that we only want to do
* this if we are successful so far and we are not processing a
@@ -278,6 +279,10 @@
}
etag = apr_table_get(r->headers_out, "ETag");
+ if((resource_exists = apr_table_get(r->notes, "resource-exists")) == NULL) {
+ /* "resource-exists" tag not set */
+ resource_exists = "U";
+ }
/* All of our comparisons must be in seconds, because that's the
* highest time resolution the HTTP specification allows.
@@ -286,17 +291,26 @@
tmp_time = ((r->mtime != 0) ? r->mtime : apr_time_now());
mtime = apr_time_sec(tmp_time);
- /* If an If-Match request-header field was given
- * AND the field value is not "*" (meaning match anything)
- * AND if our strong ETag does not match any entity tag in that field,
- * respond with a status of 412 (Precondition Failed).
- */
+ /* If an If-Match request-header field was given */
if ((if_match = apr_table_get(r->headers_in, "If-Match")) != NULL) {
- if (if_match[0] != '*'
- && (etag == NULL || etag[0] == 'W'
- || !ap_find_list_item(r->pool, if_match, etag))) {
- return HTTP_PRECONDITION_FAILED;
+ /* AND the field value is not "*" (meaning match anything)
+ * AND if our strong ETag does not match any entity tag in that field,
+ * respond with a status of 412 (Precondition Failed).
+ */
+ if (if_match[0] != '*') {
+ if (etag == NULL || etag[0] == 'W'
+ || !ap_find_list_item(r->pool, if_match, etag)) {
+ return HTTP_PRECONDITION_FAILED;
+ }
}
+ else {
+ /* Else if the field value is '*' (meaning match anything)
+ * AND resource does not exist,
+ * respond with a status of 412 (Precondition Failed).
+ */
+ if(resource_exists[0] == 'N')
+ return HTTP_PRECONDITION_FAILED;
+ }
}
else {
/* Else if a valid If-Unmodified-Since request-header field was given
@@ -316,6 +330,7 @@
/* If an If-None-Match request-header field was given
* AND the field value is "*" (meaning match anything)
+ * AND the resource exists
* OR our ETag matches any of the entity tags in that field, fail.
*
* If the request method was GET or HEAD, failure means the server
@@ -329,7 +344,7 @@
if_nonematch = apr_table_get(r->headers_in, "If-None-Match");
if (if_nonematch != NULL) {
if (r->method_number == M_GET) {
- if (if_nonematch[0] == '*') {
+ if (if_nonematch[0] == '*' && resource_exists[0] == 'Y') {
not_modified = 1;
}
else if (etag != NULL) {
@@ -344,7 +359,7 @@
}
}
}
- else if (if_nonematch[0] == '*'
+ else if ((if_nonematch[0] == '*' && resource_exists[0] == 'Y')
|| (etag != NULL
&& ap_find_list_item(r->pool, if_nonematch, etag))) {
return HTTP_PRECONDITION_FAILED;
Index: modules/dav/main/util.c
===================================================================
--- modules/dav/main/util.c (revision 5021)
+++ modules/dav/main/util.c (working copy)
@@ -1433,6 +1433,7 @@
const dav_hooks_repository *repos_hooks = resource->hooks;
dav_buffer work_buf = { 0 };
dav_response *new_response;
+ const char *resource_exists;
#if DAV_DEBUG
if (depth && response == NULL) {
@@ -1449,6 +1450,19 @@
if (response != NULL)
*response = NULL;
+ resource_exists =
+ (DAV_RESOURCE_EXISTS == dav_get_resource_state(r, resource)) ? "Y" : "N";
+
+ /* Set "resource-exists" key in r->notes, required by ap_meets_conditions()
+ * for proper If-* header checking */
+ apr_table_set(r->notes, "resource-exists", resource_exists);
+
+ /* Set the Etag header required by ap_meets_conditions() */
+ if ((err = (*resource->hooks->set_headers)(r, resource)) != NULL) {
+ return dav_push_error(r->pool, err->status, 0,
+ "Unable to setup HTTP headers.", err);
+ }
+
/* Do the standard checks for conditional requests using
* If-..-Since, If-Match etc */
if ((result = ap_meets_conditions(r)) != OK) {
