httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Dirk-Willem van Gulik <di...@webweaving.org>
Subject Re: cache - cleaning up mod_memcache and making other caches their live easier
Date Sun, 10 Feb 2008 16:39:26 GMT

On Feb 10, 2008, at 2:12 PM, Dirk-Willem van Gulik wrote:

> Right now (only) mod_disk_cache is doing the 'right(tm)' thing  
> w.r.t. to Vary - the other caches (mod_memcache as part of the  
> distribution and a handful of memcached, distcache and commercial  
> cache modules) are just acting on the URI (key).
>
> Bringing them in line involves a bit of cut-and-paste from disk-cache.
>
> So I am wondering if I should do the following -- but want to have  
> some feedback of the folks who have been spending the last years on  
> this -- as I may have missed something fundamental:
>
> -	Move sundry like array_alphasort, tokens_to_array up into  
> cache_utils or similar.
>
> -	Perhaps add a extra function vector called 'make_key' -- which can  
> be NULL to the
> 	cache_provider; which understands most of rfc2616; including case  
> (insensitivity). In
> 	short - we'll propably end up with a struct which details the  
> relevant headers, if
> 	they are int, date, case-insensitive or sensitive. Which allows us  
> then to always
> 	do the right thing.
>
> -	When we call store_* already pre-fillout cache_info (or add a  
> param) which does the
> 	right things around checking for Vary. And perhaps even a precooked  
> version of
> 	headers_out/in.
>
> Before I embark on an experiment (without much design/planning) --  
> any thoughts ? Or has someone already done most of this and/or  
> designed it properly ?
>
> Thanks,
>
> Dw

See below for some aloud thinking -- note that this is not yet well  
thought out (all I did was try to minimize the overlap between  
disk_cache, memcached, distcache and some commercial thing -- and  
tried to move as much RFC2616 knowledge into mod_cache.c*).

-	internal ap_cache_cacheable_hdr to hold all the RFC 2616
     	non cachable headers info as before.

-	ap_cache_cacheable_hdrs_out introduced as per earlier discussion
	which knows about error headers and mandatory content setting.

-	ap_cache_cacheable_hdrs_in introduced - which has an optional
	argument to further curtail the headers returned by just those
	in the 'Vary'.

-	ap_normalize_vary_to_array() introduced. Which can help
	caching storage modules to create a key across the Vary
	relevant fields, if any.

	=> given that they all have to create this -- wondering if
	we should make this part of the cache_info struct already.

-	Likewise a ap_generate_vary_key() which understands
	vary if/when needed.

Below more or less cleans mod_disk_cache completly of having to  
understand http (or any protocol aspect) - but for  
ap_generate_vary_key() -- but it is not yet HTTP neutral (i.e. a IMAP  
protocol module would have to overwrite things it has not yet XS to).

Thoughts,

Dw.


*: not yet nearly enough - i.e. lacking the whole case (in)sensitive,  
date, etc stuff.

Index: cache_util.c
===================================================================
--- cache_util.c	(revision 620288)
+++ cache_util.c	(working copy)
@@ -571,10 +573,10 @@
      return apr_pstrdup(p, hashfile);
  }

-/* Create a new table consisting of those elements from an input
+/* Create a new table consisting of those elements from an
   * headers table that are allowed to be stored in a cache.
   */
-CACHE_DECLARE(apr_table_t *)ap_cache_cacheable_hdrs_out(apr_pool_t  
*pool,
+CACHE_DECLARE(apr_table_t *)ap_cache_cacheable_hdrs(apr_pool_t *pool,
                                                          apr_table_t  
*t,
                                                          server_rec *s)
  {
@@ -582,12 +584,17 @@
      char **header;
      int i;

+    if (t == NULL) {
+	return NULL;
+    };
+
      /* Make a copy of the headers, and remove from
       * the copy any hop-by-hop headers, as defined in Section
       * 13.5.1 of RFC 2616
       */
      apr_table_t *headers_out;
      headers_out = apr_table_copy(pool, t);
+
      apr_table_unset(headers_out, "Connection");
      apr_table_unset(headers_out, "Keep-Alive");
      apr_table_unset(headers_out, "Proxy-Authenticate");
@@ -599,6 +606,7 @@

      conf = (cache_server_conf *)ap_get_module_config(s->module_config,
                                                       &cache_module);
+
      /* Remove the user defined headers set with CacheIgnoreHeaders.
       * This may break RFC 2616 compliance on behalf of the  
administrator.
       */
@@ -608,3 +616,170 @@
      }
      return headers_out;
  }
+
+/* Create a new table consisting of those elements from an input
+ * headers table that are allowed to be stored in a cache. Optionally
+ * filtered to just the fields passed in the vary_filter array.
+ *
+ * @return a table with the valid input headers or NULL if none are  
relevant.
+ */
+CACHE_DECLARE(apr_table_t *)ap_cache_cacheable_hdrs_in(request_rec *  
r, apr_array_header_t * vary_filter)
+{
+	apr_table_t *headers_in, * vary_filtered;
+        int i;
+
+	if (r->headers_in == NULL)
+		return NULL;
+
+        headers_in = ap_cache_cacheable_hdrs(r->pool, r->headers_in,  
r->server);
+	
+	if (!vary_filter)
+		return apr_is_empty_table(headers_in) ? NULL: headers_in;
+
+        /* The vary array is most propably the smallest of the two - su
+	 * only copy just those fields across.
+	 */
+     	vary_filtered = apr_table_make(r->pool, vary_filter->nelts);
+        for (i = 0; i < vary_filter->nelts; i++) {
+                const char *key = ((const char**)vary_filter->elts)[i];
+                const char * val = apr_table_get(headers_in, key); /*  
XXX: Not case insensitive */
+                if (val)
+                        apr_table_add(vary_filtered, key, val);
+        };
+
+	return apr_is_empty_table(vary_filtered) ? NULL : vary_filtered;
+}
+
+/* Create a new table consisting of those elements from an output
+ * headers table that are allowed to be stored in a cache;
+ * ensure there is a content type and capture any errors.
+ */
+CACHE_DECLARE(apr_table_t *)ap_cache_cacheable_hdrs_out(request_rec *  
r)
+{
+	apr_table_t *headers_out;
+
+	headers_out = ap_cache_cacheable_hdrs(r->pool, r->headers_out,
+                                                  r->server);
+
+        if (!apr_table_get(headers_out, "Content-Type")
+            && r->content_type) {
+            apr_table_setn(headers_out, "Content-Type",
+                           ap_make_content_type(r, r->content_type));
+        }
+
+        headers_out = apr_table_overlay(r->pool, headers_out,
+                                        r->err_headers_out);
+
+	return headers_out;
+}
+
+static int array_alphasort(const void *fn1, const void *fn2)
+{
+    return strcmp(*(char**)fn1, *(char**)fn2);
+}
+
+static void vary_tokens_to_array(apr_pool_t *p, const char *data,
+                            apr_array_header_t *arr)
+{
+    char *token;
+
+    while ((token = ap_get_list_item(p, &data)) != NULL) {
+        *((const char **) apr_array_push(arr)) = token;
+    }
+
+    qsort((void *) arr->elts, arr->nelts,
+         sizeof(char *), array_alphasort);
+}
+
+/* Given a header table; extract any Vary present and return
+ * the values in a predictable way (i.e. Sort it so that
+ * "Vary: A, B" and "Vary: B, A" are stored the same; and take
+ * care of any case sensitive issues. Onb exit the array will
+ * either be NULL (no VARYance) or a sorted array.
+ *
+ * @return vary-array or NULL, if no variance
+ */
+CACHE_DECLARE(apr_array_header_t*)  
ap_normalize_vary_to_array(apr_pool_t *p, apr_table_t * headers)
+{
+	apr_array_header_t* varray = NULL;
+	const char *vary;
+	
+	if ((headers) && ((vary= apr_table_get(headers, "Vary")) != NULL))
+	{
+	    varray = apr_array_make(p, 6, sizeof(char*));
+            vary_tokens_to_array(p, vary, varray);
+	}
+	
+	return varray;
+}
+
+/* Generate a new key based on the normal key (URI) and normalized  
values from
+ * the Vary array. The vary array is assumed to be in a stable order  
and format.
+ */
+CACHE_DECLARE(const char*) ap_generate_vary_key(apr_pool_t *p,  
apr_table_t * input_headers,
+                             apr_array_header_t *varray, const char  
*oldkey)
+{
+    struct iovec *iov;
+    int i, k;
+    int nvec;
+    const char *header;
+    const char **elts;
+
+    if (varray == NULL);
+	return oldkey;
+
+    nvec = (varray->nelts * 2) + 1;
+    iov = apr_palloc(p, sizeof(struct iovec) * nvec);
+    elts = (const char **) varray->elts;
+
+    /* TODO:
+     *    - Handle multiple-value headers better. (sort them?)
+     *    - Handle Case in-sensitive Values better.
+     *        This isn't the end of the world, since it just lowers  
the cache
+     *        hit rate, but it would be nice to fix.
+     *
+     * The majority are case insenstive if they are values (encoding  
etc).
+     * Most of rfc2616 is case insensitive on header contents.
+     *
+     * So the better solution may be to identify headers which should  
be
+     * treated case-sensitive?
+     *  HTTP URI's (3.2.3) [host and scheme are insensitive]
+     *  HTTP method (5.1.1)
+     *  HTTP-date values (3.3.1)
+     *  3.7 Media Types [exerpt]
+     *     The type, subtype, and parameter attribute names are case-
+     *     insensitive. Parameter values might or might not be case- 
sensitive,
+     *     depending on the semantics of the parameter name.
+     *  4.20 Except [exerpt]
+     *     Comparison of expectation values is case-insensitive for  
unquoted
+     *     tokens (including the 100-continue token), and is case- 
sensitive for
+     *     quoted-string expectation-extensions.
+	 *
+	 *  XX we are currently concatenating the values. With some puzzling  
one could
+	 *     set a header value which looks like the 'next/previous' value  
- and hence
+	 *     cause a value which is not the 'real' one. This may be a  
security risk.
+	 *     Ideally we should use a special value which cannot occur in a  
header
+	 *     legally (or is escaped/wacked). Given that (at least) a  
Header cannot
+	 *     contain a space or ':' - may be an idea to insert those.
+     */
+    for(i=0, k=0; i < varray->nelts; i++) {
+		/* Note that we're assuming that the case if the Vary fields matches
+		 * the case of the actual headers.
+		 */
+        header = apr_table_get(input_headers, elts[i]);
+        if (!header) {
+            header = "";
+        }
+        iov[k].iov_base = (char*) elts[i];
+        iov[k].iov_len = strlen(elts[i]);
+        k++;
+        iov[k].iov_base = (char*) header;
+        iov[k].iov_len = strlen(header);
+        k++;
+    }
+    iov[k].iov_base = (char*) oldkey;
+    iov[k].iov_len = strlen(oldkey);
+    k++;
+
+    return apr_pstrcatv(p, iov, k, NULL);
+}
Index: mod_mem_cache.c
===================================================================
--- mod_mem_cache.c	(revision 620288)
+++ mod_mem_cache.c	(working copy)
@@ -605,17 +605,8 @@
      mobj->req_hdrs = deep_table_copy(mobj->pool, r->headers_in);

      /* Precompute how much storage we need to hold the headers */
-    headers_out = ap_cache_cacheable_hdrs_out(r->pool, r->headers_out,
-                                              r->server);
+    headers_out = ap_cache_cacheable_hdrs_out(r);

-    /* If not set in headers_out, set Content-Type */
-    if (!apr_table_get(headers_out, "Content-Type")
-        && r->content_type) {
-        apr_table_setn(headers_out, "Content-Type",
-                       ap_make_content_type(r, r->content_type));
-    }
-
-    headers_out = apr_table_overlay(r->pool, headers_out, r- 
 >err_headers_out);
      mobj->header_out = deep_table_copy(mobj->pool, headers_out);

      /* Init the info struct */
Index: mod_cache.c
===================================================================
--- mod_cache.c	(revision 620288)
+++ mod_cache.c	(working copy)
@@ -752,10 +752,12 @@
           * However, before doing that, we need to first merge in
           * err_headers_out and we also need to strip any hop-by-hop
           * headers that might have snuck in.
           */
          r->headers_out = apr_table_overlay(r->pool, r->headers_out,
                                             r->err_headers_out);
-        r->headers_out = ap_cache_cacheable_hdrs_out(r->pool, r- 
 >headers_out,
+        r->headers_out = ap_cache_cacheable_hdrs(r->pool, r- 
 >headers_out,
                                                       r->server);
          apr_table_clear(r->err_headers_out);

Index: mod_cache.h
===================================================================
--- mod_cache.h	(revision 620288)
+++ mod_cache.h	(working copy)
@@ -288,13 +288,25 @@
                                      const char *key, char **val);
  CACHE_DECLARE(const char *)ap_cache_tokstr(apr_pool_t *p, const char  
*list, const char **str);

-/* Create a new table consisting of those elements from a request_rec's
- * headers_out that are allowed to be stored in a cache
+/* Create a new table consisting of those elements from an
+ * headers table that are allowed to be stored in a cache.
   */
-CACHE_DECLARE(apr_table_t *)ap_cache_cacheable_hdrs_out(apr_pool_t  
*pool,
+CACHE_DECLARE(apr_table_t *)ap_cache_cacheable_hdrs(apr_pool_t *pool,
                                                          apr_table_t  
*t,
                                                          server_rec  
*s);

+/* Create a new table consisting of those elements from an input
+ * headers table that are allowed to be stored in a cache. Optionally
+ * filtered to just the fields in the vary_filter.
+ */
+CACHE_DECLARE(apr_table_t *)ap_cache_cacheable_hdrs_in(request_rec *  
r, apr_array_header_t * vary_filter);
+
+/* Create a new table consisting of those elements from an output
+ * headers table that are allowed to be stored in a cache;
+ * ensure there is a content type and capture any errors.
+ */
+CACHE_DECLARE(apr_table_t *)ap_cache_cacheable_hdrs_out(request_rec *  
r);
+
  /**
   * cache_storage.c
   */
@@ -316,6 +328,21 @@
  apr_status_t cache_recall_entity_body(cache_handle_t *h, apr_pool_t  
*p, apr_bucket_brigade *bb);
  */

+/* Given a header table; extract any Vary present and return
+ * the values in a predictable way (i.e. Sort it so that
+ * "Vary: A, B" and "Vary: B, A" are stored the same; and take
+ * care of any case sensitive issues. Onb exit the array will
+ * either be NULL (no VARYance) or a sorted array.
+ *
+ * @return vary-array or NULL, if no variance
+ */
+CACHE_DECLARE(apr_array_header_t*)  
ap_normalize_vary_to_array(apr_pool_t *p, apr_table_t * headers);
+
+/* Generate a new key based on the normal key (URI) and normalized  
values from
+ * the Vary array. The vary array is assumed to be in a stable order  
and format.
+ */
+CACHE_DECLARE(const char*) ap_generate_vary_key(apr_pool_t *p,  
apr_table_t * input_headers,
+                             apr_array_header_t *varray, const char  
*oldkey);
  /* hooks */

  APR_DECLARE_OPTIONAL_FN(apr_status_t,
Index: mod_disk_cache.c
===================================================================
--- mod_disk_cache.c	(revision 620288)
+++ mod_disk_cache.c	(working copy)
@@ -102,7 +102,7 @@
       }
  }

-static void mkdir_structure(disk_cache_conf *conf, const char *file,  
apr_pool_t *pool)
+static apr_status_t mkdir_structure(disk_cache_conf *conf, const char  
*file, apr_pool_t *pool)
  {
      apr_status_t rv;
      char *p;
@@ -116,11 +116,12 @@
          rv = apr_dir_make(file,
                            APR_UREAD|APR_UWRITE|APR_UEXECUTE, pool);
          if (rv != APR_SUCCESS && !APR_STATUS_IS_EEXIST(rv)) {
-            /* XXX */
+            return rv;
          }
          *p = '/';
          ++p;
      }
+    return APR_SUCCESS;
  }

  /* htcacheclean may remove directories underneath us.
@@ -141,7 +142,9 @@
              /* 1000 micro-seconds aka 0.001 seconds. */
              apr_sleep(1000);

-            mkdir_structure(conf, dest, pool);
+            rv = mkdir_structure(conf, dest, pool);
+	    if (rv != APR_SUCCESS)
+		continue;

              rv = apr_file_rename(src, dest, pool);
          }
@@ -241,81 +244,6 @@
      return APR_SUCCESS;
  }

-static const char* regen_key(apr_pool_t *p, apr_table_t *headers,
-                             apr_array_header_t *varray, const char  
*oldkey)
-{
-    struct iovec *iov;
-    int i, k;
-    int nvec;
-    const char *header;
-    const char **elts;
-
-    nvec = (varray->nelts * 2) + 1;
-    iov = apr_palloc(p, sizeof(struct iovec) * nvec);
-    elts = (const char **) varray->elts;
-
-    /* TODO:
-     *    - Handle multiple-value headers better. (sort them?)
-     *    - Handle Case in-sensitive Values better.
-     *        This isn't the end of the world, since it just lowers  
the cache
-     *        hit rate, but it would be nice to fix.
-     *
-     * The majority are case insenstive if they are values (encoding  
etc).
-     * Most of rfc2616 is case insensitive on header contents.
-     *
-     * So the better solution may be to identify headers which should  
be
-     * treated case-sensitive?
-     *  HTTP URI's (3.2.3) [host and scheme are insensitive]
-     *  HTTP method (5.1.1)
-     *  HTTP-date values (3.3.1)
-     *  3.7 Media Types [exerpt]
-     *     The type, subtype, and parameter attribute names are case-
-     *     insensitive. Parameter values might or might not be case- 
sensitive,
-     *     depending on the semantics of the parameter name.
-     *  4.20 Except [exerpt]
-     *     Comparison of expectation values is case-insensitive for  
unquoted
-     *     tokens (including the 100-continue token), and is case- 
sensitive for
-     *     quoted-string expectation-extensions.
-     */
-
-    for(i=0, k=0; i < varray->nelts; i++) {
-        header = apr_table_get(headers, elts[i]);
-        if (!header) {
-            header = "";
-        }
-        iov[k].iov_base = (char*) elts[i];
-        iov[k].iov_len = strlen(elts[i]);
-        k++;
-        iov[k].iov_base = (char*) header;
-        iov[k].iov_len = strlen(header);
-        k++;
-    }
-    iov[k].iov_base = (char*) oldkey;
-    iov[k].iov_len = strlen(oldkey);
-    k++;
-
-    return apr_pstrcatv(p, iov, k, NULL);
-}
-
-static int array_alphasort(const void *fn1, const void *fn2)
-{
-    return strcmp(*(char**)fn1, *(char**)fn2);
-}
-
-static void tokens_to_array(apr_pool_t *p, const char *data,
-                            apr_array_header_t *arr)
-{
-    char *token;
-
-    while ((token = ap_get_list_item(p, &data)) != NULL) {
-        *((const char **) apr_array_push(arr)) = token;
-    }
-
-    /* Sort it so that "Vary: A, B" and "Vary: B, A" are stored the  
same. */
-    qsort((void *) arr->elts, arr->nelts,
-         sizeof(char *), array_alphasort);
-}
-
  /*
   * Hook and mod_cache callback functions
   */
@@ -432,7 +360,7 @@
          }
          apr_file_close(dobj->hfd);

-        nkey = regen_key(r->pool, r->headers_in, varray, key);
+        nkey = ap_generate_vary_key(r->pool, r->headers_in, varray,  
key);

          dobj->hashfile = NULL;
          dobj->prefix = dobj->hdrsfile;
@@ -472,7 +400,8 @@
  #endif
      rc = apr_file_open(&dobj->fd, dobj->datafile, flags, 0, r->pool);
      if (rc != APR_SUCCESS) {
-        /* XXX: Log message */
+    	ap_log_error(APLOG_MARK, APLOG_ERR, rc, r->server,
+                 "disk_cache: Cannot open %f",  dobj->datafile);
          return DECLINED;
      }

@@ -484,7 +413,8 @@
      /* Read the bytes to setup the cache_info fields */
      rc = file_cache_recall_mydata(dobj->hfd, info, dobj, r);
      if (rc != APR_SUCCESS) {
-        /* XXX log message */
+    	ap_log_error(APLOG_MARK, APLOG_ERR, rc, r->server,
+                 "disk_cache: Cannot read cache_info from %f",  dobj- 
 >hdrsfile);
          return DECLINED;
      }

@@ -821,6 +751,7 @@
      apr_status_t rv;
      apr_size_t amt;
      disk_cache_object_t *dobj = (disk_cache_object_t*) h->cache_obj- 
 >vobj;
+    apr_array_header_t* varray;

      disk_cache_info_t disk_info;
      struct iovec iov[2];
@@ -828,15 +759,18 @@
      /* This is flaky... we need to manage the cache_info differently  
*/
      h->cache_obj->info = *info;

-    if (r->headers_out) {
-        const char *tmp;
-
-        tmp = apr_table_get(r->headers_out, "Vary");
-
-        if (tmp) {
-            apr_array_header_t* varray;
+    /* XXX it may make sense to make this part of cache_info -- as  
every
+     *     sane module will need to calculate this. Potentially
+     *     allong with a ap_generate_vary_key() value as well.
+     *
+     *     Or alternatively insist on the create_/open_ to already
+     *     prepare for this eventuality.
+     */
+    varray  = ap_normalize_vary_to_array(r->pool, r->headers_out);
+    if (varray) {
              apr_uint32_t format = VARY_FORMAT_VERSION;
-
+	    const char * nkey;
+			
              /* If we were initially opened as a vary format, rollback
               * that internal state for the moment so we can recreate  
the
               * vary format hints in the appropriate directory.
@@ -846,7 +780,12 @@
                  dobj->prefix = NULL;
              }

-            mkdir_structure(conf, dobj->hdrsfile, r->pool);
+            rv = mkdir_structure(conf, dobj->hdrsfile, r->pool);
+            if (rv != APR_SUCCESS) {
+        	ap_log_error(APLOG_MARK, APLOG_WARNING, rv, r->server,
+                     "disk_cache: cannot create path for %s", dobj- 
 >hdrsfile);
+                return rv;
+            }

              rv = apr_file_mktemp(&dobj->tfd, dobj->tempfile,
                                   APR_CREATE | APR_WRITE | APR_BINARY  
| APR_EXCL,
@@ -862,9 +801,6 @@
              amt = sizeof(info->expire);
              apr_file_write(dobj->tfd, &info->expire, &amt);

-            varray = apr_array_make(r->pool, 6, sizeof(char*));
-            tokens_to_array(r->pool, tmp, varray);
-
              store_array(dobj->tfd, varray);

              apr_file_close(dobj->tfd);
@@ -882,15 +818,13 @@
              }

              dobj->tempfile = apr_pstrcat(r->pool, conf->cache_root,  
AP_TEMPFILE, NULL);
-            tmp = regen_key(r->pool, r->headers_in, varray, dobj- 
 >name);
+            nkey = ap_generate_vary_key(r->pool, r->headers_in,  
varray, dobj->name);
              dobj->prefix = dobj->hdrsfile;
              dobj->hashfile = NULL;
-            dobj->datafile = data_file(r->pool, conf, dobj, tmp);
-            dobj->hdrsfile = header_file(r->pool, conf, dobj, tmp);
+            dobj->datafile = data_file(r->pool, conf, dobj, nkey);
+            dobj->hdrsfile = header_file(r->pool, conf, dobj, nkey);
          }
-    }

-
      rv = apr_file_mktemp(&dobj->hfd, dobj->tempfile,
                           APR_CREATE | APR_WRITE | APR_BINARY |
                           APR_BUFFERED | APR_EXCL, r->pool);
@@ -916,41 +850,40 @@

      rv = apr_file_writev(dobj->hfd, (const struct iovec *) &iov, 2,  
&amt);
      if (rv != APR_SUCCESS) {
+        apr_file_close(dobj->hfd); /* flush and close */
+	apr_file_remove(dobj->tempfile, r->pool);
          return rv;
      }

      if (r->headers_out) {
          apr_table_t *headers_out;

-        headers_out = ap_cache_cacheable_hdrs_out(r->pool, r- 
 >headers_out,
-                                                  r->server);
+        headers_out = ap_cache_cacheable_hdrs_out(r);

-        if (!apr_table_get(headers_out, "Content-Type")
-            && r->content_type) {
-            apr_table_setn(headers_out, "Content-Type",
-                           ap_make_content_type(r, r->content_type));
-        }
-
-        headers_out = apr_table_overlay(r->pool, headers_out,
-                                        r->err_headers_out);
          rv = store_table(dobj->hfd, headers_out);
          if (rv != APR_SUCCESS) {
+    	    apr_file_close(dobj->hfd); /* flush and close */
+	    apr_file_remove(dobj->tempfile, r->pool);
              return rv;
          }
      }

-    /* Parse the vary header and dump those fields from the  
headers_in. */
-    /* FIXME: Make call to the same thing cache_select calls to crack  
Vary. */
-    if (r->headers_in) {
+    /* Parse the vary header and dump those fields from the headers_in.
+     * But only do so if needed - i.e. there is a Vary - and then limit
+     * it to the headers needed to validate the Vary.
+     */
+    if (r->headers_in && varray) {
          apr_table_t *headers_in;
-
-        headers_in = ap_cache_cacheable_hdrs_out(r->pool, r- 
 >headers_in,
-                                                 r->server);
+        headers_in = ap_cache_cacheable_hdrs_in(r, varray);
+        if (headers_in) {
          rv = store_table(dobj->hfd, headers_in);
          if (rv != APR_SUCCESS) {
+    		     apr_file_close(dobj->hfd); /* flush and close */
+	    	     apr_file_remove(dobj->tempfile, r->pool);
              return rv;
          }
      }
+    }

      apr_file_close(dobj->hfd); /* flush and close */

@@ -960,8 +893,14 @@
       */
      rv = apr_file_remove(dobj->hdrsfile, r->pool);
      if (rv != APR_SUCCESS) {
-        mkdir_structure(conf, dobj->hdrsfile, r->pool);
+        rv = mkdir_structure(conf, dobj->hdrsfile, r->pool);
+            if (rv != APR_SUCCESS) {
+        	ap_log_error(APLOG_MARK, APLOG_WARNING, rv, r->server,
+                     "disk_cache: cannot create path for %s", dobj- 
 >hdrsfile);
+		apr_file_remove(dobj->tempfile, r->pool);
+                return rv;
      }
+    }

      rv = safe_file_rename(conf, dobj->tempfile, dobj->hdrsfile, r- 
 >pool);
      if (rv != APR_SUCCESS) {


Mime
View raw message