You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Davi Arnaut <da...@haxent.com.br> on 2006/09/20 04:33:53 UTC

[patch 00/16] SoC 2006, cache refactoring

Hi,

My first round of patches to the cache-refactor branch. They are mostly cleanups,
bug fixes and small but useful enchantments.

The patches are also available at http://verdesmares.com/Apache/patches/

--
Davi Arnaut

[patch 13/16] remove unneeded prototypes

Posted by Davi Arnaut <da...@haxent.com.br>.
Remove unneeded prototypes for static functions.

Index: modules/cache/mod_disk_cache.c
===================================================================
--- modules/cache/mod_disk_cache.c.orig
+++ modules/cache/mod_disk_cache.c
@@ -53,14 +53,6 @@
 
 module AP_MODULE_DECLARE_DATA disk_cache_module;
 
-/* Forward declarations */
-static int remove_entity(cache_handle_t *h);
-static apr_status_t store_headers(cache_handle_t *h, request_rec *r, cache_info *i);
-static apr_status_t store_body(cache_handle_t *h, request_rec *r, apr_bucket_brigade *b);
-static apr_status_t recall_headers(cache_handle_t *h, request_rec *r);
-static apr_status_t recall_body(cache_handle_t *h, apr_pool_t *p, apr_bucket_brigade *bb);
-static apr_status_t read_array(apr_array_header_t* arr, apr_file_t *file);
-
 /*
  * Local static functions
  */
@@ -153,6 +145,58 @@
     return rv;
 }
 
+/* XXX this is a temporary function, it will be removed later */
+static apr_status_t read_array(apr_array_header_t* array, apr_file_t *file)
+{
+    apr_status_t rv;
+    apr_size_t nbytes;
+    apr_finfo_t finfo;
+    char *buffer, *end;
+
+    rv = apr_file_info_get(&finfo, APR_FINFO_SIZE, file);
+
+    if (rv != APR_SUCCESS) {
+        return rv;
+    }
+
+    nbytes = finfo.size;
+
+    buffer = apr_palloc(array->pool, nbytes);
+
+    rv = apr_file_read(file, buffer, &nbytes);
+
+    if (rv != APR_SUCCESS) {
+        return rv;
+    }
+
+    end = memchr(buffer, CR, nbytes);
+
+    if (end == NULL) {
+        return APR_EGENERAL;
+    }
+
+    *end = '\0';
+
+    array_unserialize(array, buffer);
+
+    return APR_SUCCESS;
+}
+
+/* XXX this is a temporary function, it will be removed later */
+static apr_status_t store_array(apr_pool_t *p, apr_file_t *fd,
+                                apr_array_header_t* array)
+{
+    char *str;
+    apr_size_t len;
+
+    str = array_serialize(p, array, &len);
+
+    if (apr_file_printf(fd, "%s" CRLF, str) < 0)
+        return APR_EGENERAL;
+    else
+        return APR_SUCCESS;
+}
+
 /* htcacheclean may remove directories underneath us.
  * So, we'll try renaming three times at a cost of 0.002 seconds.
  */
@@ -582,58 +626,6 @@
 }
 
 /* XXX this is a temporary function, it will be removed later */
-static apr_status_t read_array(apr_array_header_t* array, apr_file_t *file)
-{
-    apr_status_t rv;
-    apr_size_t nbytes;
-    apr_finfo_t finfo;
-    char *buffer, *end;
-
-    rv = apr_file_info_get(&finfo, APR_FINFO_SIZE, file);
-
-    if (rv != APR_SUCCESS) {
-        return rv;
-    }
-
-    nbytes = finfo.size;
-
-    buffer = apr_palloc(array->pool, nbytes);
-
-    rv = apr_file_read(file, buffer, &nbytes);
-
-    if (rv != APR_SUCCESS) {
-        return rv;
-    }
-
-    end = memchr(buffer, CR, nbytes);
-
-    if (end == NULL) {
-        return APR_EGENERAL;
-    }
-
-    *end = '\0';
-
-    array_unserialize(array, buffer);
-
-    return APR_SUCCESS;
-}
-
-/* XXX this is a temporary function, it will be removed later */
-static apr_status_t store_array(apr_pool_t *p, apr_file_t *fd,
-                                apr_array_header_t* array)
-{
-    char *str;
-    apr_size_t len;
-
-    str = array_serialize(p, array, &len);
-
-    if (apr_file_printf(fd, "%s" CRLF, str) < 0)
-        return APR_EGENERAL;
-    else
-        return APR_SUCCESS;
-}
-
-/* XXX this is a temporary function, it will be removed later */
 static apr_status_t read_table(apr_pool_t *p, apr_table_t *table, apr_file_t *fd)
 {
     char *end, *buf;

--

Re: [patch 10/16] fix up coding style issues

Posted by Davi Arnaut <da...@haxent.com.br>.
On 20/09/2006, at 06:20, Joe Orton wrote:

> Hi Davi,
>
> On Tue, Sep 19, 2006 at 11:34:03PM -0300, Davi Arnaut wrote:
>> Clean up code style in the cache code and shrink the mod_mem_cache
>> store_body function.
>
> The casts to/from void * are unnecessary and could just be removed
> rather than being whitespace-adjusted.

Yes, certainly.

>
>> --- modules/cache/mod_mem_cache.c.orig
>> +++ modules/cache/mod_mem_cache.c
>> @@ -52,27 +52,30 @@
>>
>>  module AP_MODULE_DECLARE_DATA mem_cache_module;
>>
>> -typedef enum {
>> +typedef enum
>> +{
>
> The traditional style is to keep the brace on the same line for data
> type definitions (but not with function definitions).  see e.g. "grep
> struct httpd.h" :)

I used the GNU indent command line as cited at http:// 
httpd.apache.org/dev/styleguide.html
Anyway, I should have double checked the result.

--
Davi Arnaut

Re: [patch 10/16] fix up coding style issues

Posted by Joe Orton <jo...@redhat.com>.
Hi Davi,

On Tue, Sep 19, 2006 at 11:34:03PM -0300, Davi Arnaut wrote:
> Clean up code style in the cache code and shrink the mod_mem_cache
> store_body function.

The casts to/from void * are unnecessary and could just be removed 
rather than being whitespace-adjusted.

> --- modules/cache/mod_mem_cache.c.orig
> +++ modules/cache/mod_mem_cache.c
> @@ -52,27 +52,30 @@
>  
>  module AP_MODULE_DECLARE_DATA mem_cache_module;
>  
> -typedef enum {
> +typedef enum
> +{

The traditional style is to keep the brace on the same line for data 
type definitions (but not with function definitions).  see e.g. "grep 
struct httpd.h" :)

Regards,

joe

[patch 10/16] fix up coding style issues

Posted by Davi Arnaut <da...@haxent.com.br>.
Clean up code style in the cache code and shrink the mod_mem_cache
store_body function.


Index: modules/cache/mod_mem_cache.c
===================================================================
--- modules/cache/mod_mem_cache.c.orig
+++ modules/cache/mod_mem_cache.c
@@ -52,27 +52,30 @@
 
 module AP_MODULE_DECLARE_DATA mem_cache_module;
 
-typedef enum {
+typedef enum
+{
     CACHE_TYPE_FILE = 1,
     CACHE_TYPE_HEAP,
     CACHE_TYPE_MMAP
 } cache_type_e;
 
-typedef struct {
-    char* hdr;
-    char* val;
+typedef struct
+{
+    char *hdr;
+    char *val;
 } cache_header_tbl_t;
 
-typedef struct mem_cache_object {
+typedef struct mem_cache_object
+{
     cache_type_e type;
     apr_ssize_t num_header_out;
     apr_ssize_t num_req_hdrs;
     cache_header_tbl_t *header_out;
-    cache_header_tbl_t *req_hdrs; /* for Vary negotiation */
+    cache_header_tbl_t *req_hdrs;       /* for Vary negotiation */
     apr_size_t m_len;
     void *m;
     apr_os_file_t fd;
-    apr_int32_t flags;  /* File open flags */
+    apr_int32_t flags;          /* File open flags */
     long priority;      /**< the priority of this entry */
     long total_refs;          /**< total number of references this entry has had */
 
@@ -80,14 +83,15 @@
 
 } mem_cache_object_t;
 
-typedef struct {
+typedef struct
+{
     apr_thread_mutex_t *lock;
     cache_cache_t *cache_cache;
 
     /* Fields set by config directives */
     apr_size_t min_cache_object_size;   /* in bytes */
     apr_size_t max_cache_object_size;   /* in bytes */
-    apr_size_t max_cache_size;          /* in bytes */
+    apr_size_t max_cache_size;  /* in bytes */
     apr_size_t max_object_cnt;
     cache_pqueue_set_priority cache_remove_algorithm;
 
@@ -95,8 +99,11 @@
      * we haven't yet seen EOS */
     apr_off_t max_streaming_buffer_size;
 } mem_cache_conf;
+
 static mem_cache_conf *sconf;
 
+static void cleanup_cache_object(cache_object_t * obj);
+
 #define DEFAULT_MAX_CACHE_SIZE 100*1024
 #define DEFAULT_MIN_CACHE_OBJECT_SIZE 0
 #define DEFAULT_MAX_CACHE_OBJECT_SIZE 10000
@@ -104,15 +111,6 @@
 #define DEFAULT_MAX_STREAMING_BUFFER_SIZE 100000
 #define CACHEFILE_LEN 20
 
-/* Forward declarations */
-static int remove_entity(cache_handle_t *h);
-static apr_status_t store_headers(cache_handle_t *h, request_rec *r, cache_info *i);
-static apr_status_t store_body(cache_handle_t *h, request_rec *r, apr_bucket_brigade *b);
-static apr_status_t recall_headers(cache_handle_t *h, request_rec *r);
-static apr_status_t recall_body(cache_handle_t *h, apr_pool_t *p, apr_bucket_brigade *bb);
-
-static void cleanup_cache_object(cache_object_t *obj);
-
 static long memcache_get_priority(void*a)
 {
     cache_object_t *obj = (cache_object_t *)a;
@@ -132,31 +130,34 @@
 
 static void memcache_set_pos(void *a, apr_ssize_t pos)
 {
-    cache_object_t *obj = (cache_object_t *)a;
+    cache_object_t *obj = (cache_object_t *) a;
     mem_cache_object_t *mobj = obj->vobj;
 
     apr_atomic_set32(&mobj->pos, pos);
 }
+
 static apr_ssize_t memcache_get_pos(void *a)
 {
-    cache_object_t *obj = (cache_object_t *)a;
+    cache_object_t *obj = (cache_object_t *) a;
     mem_cache_object_t *mobj = obj->vobj;
 
     return apr_atomic_read32(&mobj->pos);
 }
 
-static apr_size_t memcache_cache_get_size(void*a)
+static apr_size_t memcache_cache_get_size(void *a)
 {
-    cache_object_t *obj = (cache_object_t *)a;
+    cache_object_t *obj = (cache_object_t *) a;
     mem_cache_object_t *mobj = obj->vobj;
     return mobj->m_len;
 }
+
 /** callback to get the key of a item */
-static const char* memcache_cache_get_key(void*a)
+static const char *memcache_cache_get_key(void *a)
 {
-    cache_object_t *obj = (cache_object_t *)a;
+    cache_object_t *obj = (cache_object_t *) a;
     return obj->key;
 }
+
 /**
  * memcache_cache_free()
  * memcache_cache_free is a callback that is only invoked by a thread
@@ -165,9 +166,9 @@
  * has been ejected from the cache. decrement the refcount and if the refcount drops
  * to 0, cleanup the cache object.
  */
-static void memcache_cache_free(void*a)
+static void memcache_cache_free(void *a)
 {
-    cache_object_t *obj = (cache_object_t *)a;
+    cache_object_t *obj = (cache_object_t *) a;
 
     /* Decrement the refcount to account for the object being ejected
      * from the cache. If the refcount is 0, free the object.
@@ -176,13 +177,14 @@
         cleanup_cache_object(obj);
     }
 }
+
 /*
  * functions return a 'negative' score since priority queues
  * dequeue the object with the highest value first
  */
 static long memcache_lru_algorithm(long queue_clock, void *a)
 {
-    cache_object_t *obj = (cache_object_t *)a;
+    cache_object_t *obj = (cache_object_t *) a;
     mem_cache_object_t *mobj = obj->vobj;
     if (mobj->priority == 0)
         mobj->priority = queue_clock - mobj->total_refs;
@@ -196,17 +198,17 @@
 
 static long memcache_gdsf_algorithm(long queue_clock, void *a)
 {
-    cache_object_t *obj = (cache_object_t *)a;
+    cache_object_t *obj = (cache_object_t *) a;
     mem_cache_object_t *mobj = obj->vobj;
 
     if (mobj->priority == 0)
         mobj->priority = queue_clock -
-                           (long)(mobj->total_refs*1000 / mobj->m_len);
+            (long) (mobj->total_refs * 1000 / mobj->m_len);
 
     return mobj->priority;
 }
 
-static void cleanup_cache_object(cache_object_t *obj)
+static void cleanup_cache_object(cache_object_t * obj)
 {
     mem_cache_object_t *mobj = obj->vobj;
 
@@ -218,7 +220,7 @@
 
     /* Cleanup the cache_object_t */
     if (obj->key) {
-        free((void*)obj->key);
+        free((void *) obj->key);
     }
 
     free(obj);
@@ -248,6 +250,7 @@
         free(mobj);
     }
 }
+
 static apr_status_t decrement_refcount(void *arg)
 {
     cache_object_t *obj = (cache_object_t *) arg;
@@ -279,10 +282,11 @@
     }
     return APR_SUCCESS;
 }
+
 static apr_status_t cleanup_cache_mem(void *sconfv)
 {
     cache_object_t *obj;
-    mem_cache_conf *co = (mem_cache_conf*) sconfv;
+    mem_cache_conf *co = (mem_cache_conf *) sconfv;
 
     if (!co) {
         return APR_SUCCESS;
@@ -311,10 +315,11 @@
     }
     return APR_SUCCESS;
 }
+
 /*
  * TODO: enable directives to be overridden in various containers
  */
-static void *create_cache_config(apr_pool_t *p, server_rec *s)
+static void *create_cache_config(apr_pool_t * p, server_rec *s)
 {
     sconf = apr_pcalloc(p, sizeof(mem_cache_conf));
 
@@ -331,7 +336,7 @@
     return sconf;
 }
 
-static int create_entity(cache_handle_t *h, cache_type_e type_e,
+static int create_entity(cache_handle_t * h, cache_type_e type_e,
                          request_rec *r, const char *key, apr_off_t len)
 {
     cache_object_t *obj, *tmp_obj;
@@ -381,7 +386,7 @@
         cleanup_cache_object(obj);
         return DECLINED;
     }
-    memcpy((void*)obj->key, key, key_len);
+    memcpy((void *) obj->key, key, key_len);
 
     /* Allocate and init mem_cache_object_t */
     mobj = calloc(1, sizeof(*mobj));
@@ -396,7 +401,7 @@
     obj->complete = 0;
     obj->vobj = mobj;
     /* Safe cast: We tested < sconf->max_cache_object_size above */
-    mobj->m_len = (apr_size_t)len;
+    mobj->m_len = (apr_size_t) len;
     mobj->type = type_e;
 
     /* Place the cache_object_t into the hash table.
@@ -446,19 +451,19 @@
     return OK;
 }
 
-static int create_mem_entity(cache_handle_t *h, request_rec *r,
+static int create_mem_entity(cache_handle_t * h, request_rec *r,
                              const char *key, apr_off_t len)
 {
     return create_entity(h, CACHE_TYPE_HEAP, r, key, len);
 }
 
-static int create_fd_entity(cache_handle_t *h, request_rec *r,
+static int create_fd_entity(cache_handle_t * h, request_rec *r,
                             const char *key, apr_off_t len)
 {
     return create_entity(h, CACHE_TYPE_FILE, r, key, len);
 }
 
-static int open_entity(cache_handle_t *h, request_rec *r, const char *key)
+static int open_entity(cache_handle_t * h, request_rec *r, const char *key)
 {
     cache_object_t *obj;
 
@@ -469,7 +474,7 @@
     obj = (cache_object_t *) cache_find(sconf->cache_cache, key);
     if (obj) {
         if (obj->complete) {
-            request_rec *rmain=r, *rtmp;
+            request_rec *rmain = r, *rtmp;
             apr_atomic_inc32(&obj->refcount);
             /* cache is worried about overall counts, not 'open' ones */
             cache_update(sconf->cache_cache, obj);
@@ -502,7 +507,7 @@
 
     /* Initialize the cache_handle */
     h->cache_obj = obj;
-    h->req_hdrs = NULL;  /* Pick these up in recall_headers() */
+    h->req_hdrs = NULL;         /* Pick these up in recall_headers() */
     return OK;
 }
 
@@ -513,7 +518,7 @@
  *   object has been removed from the cache by another thread and this thread
  *   is the last thread accessing the object.
  */
-static int remove_entity(cache_handle_t *h)
+static int remove_entity(cache_handle_t * h)
 {
     cache_object_t *obj = h->cache_obj;
     cache_object_t *tobj = NULL;
@@ -539,9 +544,9 @@
 
     return OK;
 }
-static apr_status_t serialize_table(cache_header_tbl_t **obj,
-                                    apr_ssize_t *nelts,
-                                    apr_table_t *table)
+
+static apr_status_t serialize_table(cache_header_tbl_t ** obj,
+                                    apr_ssize_t * nelts, apr_table_t * table)
 {
     const apr_array_header_t *elts_arr = apr_table_elts(table);
     apr_table_entry_t *elts = (apr_table_entry_t *) elts_arr->elts;
@@ -551,8 +556,8 @@
     char *buf;
 
     *nelts = elts_arr->nelts;
-    if (*nelts == 0 ) {
-        *obj=NULL;
+    if (*nelts == 0) {
+        *obj = NULL;
         return APR_SUCCESS;
     }
     *obj = malloc(sizeof(cache_header_tbl_t) * elts_arr->nelts);
@@ -562,7 +567,7 @@
     for (i = 0; i < elts_arr->nelts; ++i) {
         len += strlen(elts[i].key);
         len += strlen(elts[i].val);
-        len += 2;  /* Extra space for NULL string terminator for key and val */
+        len += 2;               /* Extra space for NULL string terminator for key and val */
     }
 
     /* Transfer the headers into a contiguous memory block */
@@ -575,20 +580,20 @@
 
     for (i = 0; i < *nelts; ++i) {
         (*obj)[i].hdr = &buf[idx];
-        len = strlen(elts[i].key) + 1;              /* Include NULL terminator */
+        len = strlen(elts[i].key) + 1;  /* Include NULL terminator */
         memcpy(&buf[idx], elts[i].key, len);
-        idx+=len;
+        idx += len;
 
         (*obj)[i].val = &buf[idx];
         len = strlen(elts[i].val) + 1;
         memcpy(&buf[idx], elts[i].val, len);
-        idx+=len;
+        idx += len;
     }
     return APR_SUCCESS;
 }
-static int unserialize_table( cache_header_tbl_t *ctbl,
-                              int num_headers,
-                              apr_table_t *t )
+
+static int unserialize_table(cache_header_tbl_t * ctbl, int num_headers,
+                             apr_table_t * t)
 {
     int i;
 
@@ -598,11 +603,9 @@
 
     return APR_SUCCESS;
 }
+
 /* Define request processing hook handlers */
-/* remove_url()
- * Notes:
- */
-static int remove_url(cache_handle_t *h, apr_pool_t *p)
+static int remove_url(cache_handle_t * h, apr_pool_t * p)
 {
     cache_object_t *obj;
     int cleanup = 0;
@@ -643,10 +646,11 @@
     return rc;
 }
 
-static apr_status_t recall_body(cache_handle_t *h, apr_pool_t *p, apr_bucket_brigade *bb)
+static apr_status_t recall_body(cache_handle_t * h, apr_pool_t * p,
+                                apr_bucket_brigade * bb)
 {
     apr_bucket *b;
-    mem_cache_object_t *mobj = (mem_cache_object_t*) h->cache_obj->vobj;
+    mem_cache_object_t *mobj = (mem_cache_object_t *) h->cache_obj->vobj;
 
     if (mobj->type == CACHE_TYPE_FILE) {
         /* CACHE_TYPE_FILE */
@@ -657,7 +661,8 @@
     }
     else {
         /* CACHE_TYPE_HEAP */
-        b = apr_bucket_immortal_create(mobj->m, mobj->m_len, bb->bucket_alloc);
+        b = apr_bucket_immortal_create(mobj->m, mobj->m_len,
+                                       bb->bucket_alloc);
         APR_BRIGADE_INSERT_TAIL(bb, b);
     }
     b = apr_bucket_eos_create(bb->bucket_alloc);
@@ -667,10 +672,11 @@
 }
 
 
-static apr_status_t store_headers(cache_handle_t *h, request_rec *r, cache_info *info)
+static apr_status_t store_headers(cache_handle_t * h, request_rec *r,
+                                  cache_info * info)
 {
     cache_object_t *obj = h->cache_obj;
-    mem_cache_object_t *mobj = (mem_cache_object_t*) obj->vobj;
+    mem_cache_object_t *mobj = (mem_cache_object_t *) obj->vobj;
     int rc;
     apr_table_t *headers_out;
 
@@ -681,9 +687,7 @@
      * - The original response headers (for returning with a cached response)
      * - The body of the message
      */
-    rc = serialize_table(&mobj->req_hdrs,
-                         &mobj->num_req_hdrs,
-                         r->headers_in);
+    rc = serialize_table(&mobj->req_hdrs, &mobj->num_req_hdrs, r->headers_in);
     if (rc != APR_SUCCESS) {
         return rc;
     }
@@ -725,12 +729,66 @@
     return APR_SUCCESS;
 }
 
-static apr_status_t store_body(cache_handle_t *h, request_rec *r, apr_bucket_brigade *b)
+static void update_object_on_cache(cache_object_t * obj,
+                                   mem_cache_object_t * mobj)
+{
+    cache_object_t *tobj = NULL;
+
+    /* Now comes the crufty part... there is no way to tell the
+     * cache that the size of the object has changed. We need
+     * to remove the object, update the size and re-add the
+     * object, all under protection of the lock.
+     */
+    if (sconf->lock) {
+        apr_thread_mutex_lock(sconf->lock);
+    }
+
+    /* Has the object been ejected from the cache? */
+    tobj = (cache_object_t *) cache_find(sconf->cache_cache, obj->key);
+
+    if (tobj == obj) {
+        /* Object is still in the cache, remove it, update the len field then
+         * replace it under protection of sconf->lock.
+         */
+        cache_remove(sconf->cache_cache, obj);
+
+        /* For illustration, cache no longer has reference to the object
+         * so decrement the refcount
+         * apr_atomic_dec32(&obj->refcount);
+         */
+        mobj->m_len = obj->count;
+
+        cache_insert(sconf->cache_cache, obj);
+        /* For illustration, cache now has reference to the object, so
+         * increment the refcount
+         * apr_atomic_inc32(&obj->refcount);
+         */
+    }
+    else if (tobj) {
+        /* Different object with the same key found in the cache. Doing nothing
+         * here will cause the object refcount to drop to 0 in decrement_refcount
+         * and the object will be cleaned up.
+         */
+
+    }
+    else {
+        /* Object has been ejected from the cache, add it back to the cache */
+        mobj->m_len = obj->count;
+        cache_insert(sconf->cache_cache, obj);
+        apr_atomic_inc32(&obj->refcount);
+    }
+
+    if (sconf->lock) {
+        apr_thread_mutex_unlock(sconf->lock);
+    }
+}
+
+static apr_status_t store_body(cache_handle_t * h, request_rec *r,
+                               apr_bucket_brigade * b)
 {
     apr_status_t rv;
     cache_object_t *obj = h->cache_obj;
-    cache_object_t *tobj = NULL;
-    mem_cache_object_t *mobj = (mem_cache_object_t*) obj->vobj;
+    mem_cache_object_t *mobj = (mem_cache_object_t *) obj->vobj;
     apr_read_type_e eblock = APR_BLOCK_READ;
     apr_bucket *e;
     char *cur;
@@ -747,9 +805,7 @@
          * - the file_bucket is the last data bucket in the brigade
          */
         for (e = APR_BRIGADE_FIRST(b);
-             e != APR_BRIGADE_SENTINEL(b);
-             e = APR_BUCKET_NEXT(e))
-        {
+             e != APR_BRIGADE_SENTINEL(b); e = APR_BUCKET_NEXT(e)) {
             if (APR_BUCKET_IS_EOS(e)) {
                 eos = 1;
             }
@@ -768,9 +824,10 @@
             /* Open a new XTHREAD handle to the file */
             apr_file_name_get(&name, file);
             mobj->flags = ((APR_SENDFILE_ENABLED & apr_file_flags_get(file))
-                           | APR_READ | APR_BINARY | APR_XTHREAD | APR_FILE_NOCLEANUP);
-            rv = apr_file_open(&tmpfile, name, mobj->flags,
-                               APR_OS_DEFAULT, r->pool);
+                           | APR_READ | APR_BINARY | APR_XTHREAD |
+                           APR_FILE_NOCLEANUP);
+            rv = apr_file_open(&tmpfile, name, mobj->flags, APR_OS_DEFAULT,
+                               r->pool);
             if (rv != APR_SUCCESS) {
                 return rv;
             }
@@ -779,7 +836,8 @@
 
             /* Open for business */
             ap_log_error(APLOG_MARK, APLOG_INFO, 0, r->server,
-                         "mem_cache: Cached file: %s with key: %s", name, obj->key);
+                         "mem_cache: Cached file: %s with key: %s", name,
+                         obj->key);
             obj->complete = 1;
             return APR_SUCCESS;
         }
@@ -799,13 +857,11 @@
         }
         obj->count = 0;
     }
-    cur = (char*) mobj->m + obj->count;
+    cur = (char *) mobj->m + obj->count;
 
     /* Iterate accross the brigade and populate the cache storage */
     for (e = APR_BRIGADE_FIRST(b);
-         e != APR_BRIGADE_SENTINEL(b);
-         e = APR_BUCKET_NEXT(e))
-    {
+         e != APR_BRIGADE_SENTINEL(b); e = APR_BUCKET_NEXT(e)) {
         const char *s;
         apr_size_t len;
 
@@ -815,57 +871,16 @@
                  * correct size and copy the streamed response into that
                  * buffer */
                 char *buf = malloc(obj->count);
+
                 if (!buf) {
                     return APR_ENOMEM;
                 }
+
                 memcpy(buf, mobj->m, obj->count);
                 free(mobj->m);
                 mobj->m = buf;
 
-                /* Now comes the crufty part... there is no way to tell the
-                 * cache that the size of the object has changed. We need
-                 * to remove the object, update the size and re-add the
-                 * object, all under protection of the lock.
-                 */
-                if (sconf->lock) {
-                    apr_thread_mutex_lock(sconf->lock);
-                }
-                /* Has the object been ejected from the cache?
-                 */
-                tobj = (cache_object_t *) cache_find(sconf->cache_cache, obj->key);
-                if (tobj == obj) {
-                    /* Object is still in the cache, remove it, update the len field then
-                     * replace it under protection of sconf->lock.
-                     */
-                    cache_remove(sconf->cache_cache, obj);
-                    /* For illustration, cache no longer has reference to the object
-                     * so decrement the refcount
-                     * apr_atomic_dec32(&obj->refcount);
-                     */
-                    mobj->m_len = obj->count;
-
-                    cache_insert(sconf->cache_cache, obj);
-                    /* For illustration, cache now has reference to the object, so
-                     * increment the refcount
-                     * apr_atomic_inc32(&obj->refcount);
-                     */
-                }
-                else if (tobj) {
-                    /* Different object with the same key found in the cache. Doing nothing
-                     * here will cause the object refcount to drop to 0 in decrement_refcount
-                     * and the object will be cleaned up.
-                     */
-
-                } else {
-                    /* Object has been ejected from the cache, add it back to the cache */
-                    mobj->m_len = obj->count;
-                    cache_insert(sconf->cache_cache, obj);
-                    apr_atomic_inc32(&obj->refcount);
-                }
-
-                if (sconf->lock) {
-                    apr_thread_mutex_unlock(sconf->lock);
-                }
+                update_object_on_cache(obj, mobj);
             }
             /* Open for business */
             ap_log_error(APLOG_MARK, APLOG_INFO, 0, r->server,
@@ -879,14 +894,14 @@
         }
         if (len) {
             /* Check for buffer overflow */
-           if ((obj->count + len) > mobj->m_len) {
-               return APR_ENOMEM;
-           }
-           else {
-               memcpy(cur, s, len);
-               cur+=len;
-               obj->count+=len;
-           }
+            if ((obj->count + len) > mobj->m_len) {
+                return APR_ENOMEM;
+            }
+            else {
+                memcpy(cur, s, len);
+                cur += len;
+                obj->count += len;
+            }
         }
         /* This should not fail, but if it does, we are in BIG trouble
          * cause we just stomped all over the heap.
@@ -895,11 +910,12 @@
     }
     return APR_SUCCESS;
 }
+
 /**
  * Configuration and start-up
  */
-static int mem_cache_post_config(apr_pool_t *p, apr_pool_t *plog,
-                                 apr_pool_t *ptemp, server_rec *s)
+static int mem_cache_post_config(apr_pool_t * p, apr_pool_t * plog,
+                                 apr_pool_t * ptemp, server_rec *s)
 {
     int threaded_mpm;
 
@@ -917,8 +933,10 @@
     if (sconf->max_streaming_buffer_size > sconf->max_cache_object_size) {
         /* Issue a notice only if something other than the default config
          * is being used */
-        if (sconf->max_streaming_buffer_size != DEFAULT_MAX_STREAMING_BUFFER_SIZE &&
-            sconf->max_cache_object_size != DEFAULT_MAX_CACHE_OBJECT_SIZE) {
+        if (sconf->max_streaming_buffer_size !=
+            DEFAULT_MAX_STREAMING_BUFFER_SIZE
+            && sconf->max_cache_object_size !=
+            DEFAULT_MAX_CACHE_OBJECT_SIZE) {
             ap_log_error(APLOG_MARK, APLOG_NOTICE, 0, s,
                          "MCacheMaxStreamingBuffer must be less than or equal to MCacheMaxObjectSize. "
                          "Resetting MCacheMaxStreamingBuffer to MCacheMaxObjectSize.");
@@ -946,7 +964,8 @@
                                     memcache_cache_get_size,
                                     memcache_cache_get_key,
                                     memcache_cache_free);
-    apr_pool_cleanup_register(p, sconf, cleanup_cache_mem, apr_pool_cleanup_null);
+    apr_pool_cleanup_register(p, sconf, cleanup_cache_mem,
+                              apr_pool_cleanup_null);
 
     if (sconf->cache_cache)
         return OK;
@@ -955,19 +974,22 @@
 
 }
 
-static const char
-*set_max_cache_size(cmd_parms *parms, void *in_struct_ptr, const char *arg)
+static const char *set_max_cache_size(cmd_parms *parms, void *in_struct_ptr,
+                                      const char *arg)
 {
     apr_size_t val;
 
     if (sscanf(arg, "%" APR_SIZE_T_FMT, &val) != 1) {
-        return "MCacheSize argument must be an integer representing the max cache size in KBytes.";
+        return
+            "MCacheSize argument must be an integer representing the max cache size in KBytes.";
     }
-    sconf->max_cache_size = val*1024;
+    sconf->max_cache_size = val * 1024;
     return NULL;
 }
-static const char
-*set_min_cache_object_size(cmd_parms *parms, void *in_struct_ptr, const char *arg)
+
+static const char *set_min_cache_object_size(cmd_parms *parms,
+                                             void *in_struct_ptr,
+                                             const char *arg)
 {
     apr_size_t val;
 
@@ -977,8 +999,10 @@
     sconf->min_cache_object_size = val;
     return NULL;
 }
-static const char
-*set_max_cache_object_size(cmd_parms *parms, void *in_struct_ptr, const char *arg)
+
+static const char *set_max_cache_object_size(cmd_parms *parms,
+                                             void *in_struct_ptr,
+                                             const char *arg)
 {
     apr_size_t val;
 
@@ -988,8 +1012,9 @@
     sconf->max_cache_object_size = val;
     return NULL;
 }
-static const char
-*set_max_object_count(cmd_parms *parms, void *in_struct_ptr, const char *arg)
+
+static const char *set_max_object_count(cmd_parms *parms, void *in_struct_ptr,
+                                        const char *arg)
 {
     apr_size_t val;
 
@@ -1000,8 +1025,8 @@
     return NULL;
 }
 
-static const char
-*set_cache_removal_algorithm(cmd_parms *parms, void *name, const char *arg)
+static const char *set_cache_removal_algorithm(cmd_parms *parms, void *name,
+                                               const char *arg)
 {
     if (strcasecmp("LRU", arg)) {
         sconf->cache_remove_algorithm = memcache_lru_algorithm;
@@ -1028,25 +1053,23 @@
     return NULL;
 }
 
-static const command_rec cache_cmds[] =
-{
+static const command_rec cache_cmds[] = {
     AP_INIT_TAKE1("MCacheSize", set_max_cache_size, NULL, RSRC_CONF,
-     "The maximum amount of memory used by the cache in KBytes"),
+                  "The maximum amount of memory used by the cache in KBytes"),
     AP_INIT_TAKE1("MCacheMaxObjectCount", set_max_object_count, NULL, RSRC_CONF,
-     "The maximum number of objects allowed to be placed in the cache"),
+                  "The maximum number of objects allowed to be placed in the cache"),
     AP_INIT_TAKE1("MCacheMinObjectSize", set_min_cache_object_size, NULL, RSRC_CONF,
-     "The minimum size (in bytes) of an object to be placed in the cache"),
+                  "The minimum size (in bytes) of an object to be placed in the cache"),
     AP_INIT_TAKE1("MCacheMaxObjectSize", set_max_cache_object_size, NULL, RSRC_CONF,
-     "The maximum size (in bytes) of an object to be placed in the cache"),
+                  "The maximum size (in bytes) of an object to be placed in the cache"),
     AP_INIT_TAKE1("MCacheRemovalAlgorithm", set_cache_removal_algorithm, NULL, RSRC_CONF,
-     "The algorithm used to remove entries from the cache (default: GDSF)"),
+                  "The algorithm used to remove entries from the cache (default: GDSF)"),
     AP_INIT_TAKE1("MCacheMaxStreamingBuffer", set_max_streaming_buffer, NULL, RSRC_CONF,
-     "Maximum number of bytes of content to buffer for a streamed response"),
+                  "Maximum number of bytes of content to buffer for a streamed response"),
     {NULL}
 };
 
-static const cache_provider cache_mem_provider =
-{
+static const cache_provider cache_mem_provider = {
     &remove_entity,
     &store_headers,
     &store_body,
@@ -1057,8 +1080,7 @@
     &remove_url,
 };
 
-static const cache_provider cache_fd_provider =
-{
+static const cache_provider cache_fd_provider = {
     &remove_entity,
     &store_headers,
     &store_body,
@@ -1069,29 +1091,21 @@
     &remove_url,
 };
 
-static void register_hooks(apr_pool_t *p)
+static void register_hooks(apr_pool_t * p)
 {
     ap_hook_post_config(mem_cache_post_config, NULL, NULL, APR_HOOK_MIDDLE);
-    /* cache initializer */
-    /* cache_hook_init(cache_mem_init, NULL, NULL, APR_HOOK_MIDDLE);  */
-    /*
-    cache_hook_create_entity(create_entity, NULL, NULL, APR_HOOK_MIDDLE);
-    cache_hook_open_entity(open_entity,  NULL, NULL, APR_HOOK_MIDDLE);
-    cache_hook_remove_url(remove_url, NULL, NULL, APR_HOOK_MIDDLE);
-    */
     ap_register_provider(p, CACHE_PROVIDER_GROUP, "mem", "0",
                          &cache_mem_provider);
     ap_register_provider(p, CACHE_PROVIDER_GROUP, "fd", "0",
                          &cache_fd_provider);
 }
 
-module AP_MODULE_DECLARE_DATA mem_cache_module =
-{
+module AP_MODULE_DECLARE_DATA mem_cache_module = {
     STANDARD20_MODULE_STUFF,
-    NULL,                    /* create per-directory config structure */
-    NULL,                    /* merge per-directory config structures */
-    create_cache_config,     /* create per-server config structure */
-    NULL,                    /* merge per-server config structures */
-    cache_cmds,              /* command apr_table_t */
+    NULL,                       /* create per-directory config structure */
+    NULL,                       /* merge per-directory config structures */
+    create_cache_config,        /* create per-server config structure */
+    NULL,                       /* merge per-server config structures */
+    cache_cmds,                 /* command apr_table_t */
     register_hooks
 };

--

[patch 02/16] revamped mod_disk_cache directory structure

Posted by Davi Arnaut <da...@haxent.com.br>.
This patch converts the mod_disk_cache cache directory structure to a
uniformly distributed N level hierarchy. The admin specifies the number
of levels (or directories) and the files are scattered across the
directories. Example:

CacheRoot /cache/
# CacheDirLevels n l1 l2 ln
CacheDirLevels 2 4 256 16

In this example, the directory tree will be three levels deep. The first
level will have 4 directories, each of the 4 first level directories will
have 256 directories (second level) and so on to the last level.

Directory tree layout for the example:

/cache/
/cache/0d
/cache/0d/36
/cache/0d/36/06
/cache/0d/36/06/7a50a5c38a73abdb6db28a2b0f6881e5.data
/cache/0d/36/06/7a50a5c38a73abdb6db28a2b0f6881e5.header

This patch adds support for symlinking the directories to separate disk
or partitions by creating the cache files on their destination directory.
The symlinks can also be used to load balance the cache files between
disks because each last level directory gets the same (on average) number
of files -- on a cache setup with 4 first level directories each one
receives 25%, linking the three of them to disk A and the last one to
disk B yields a 75/25 distribution between disks A (75) and B (25).

Index: modules/cache/cache_util.c
===================================================================
--- modules/cache/cache_util.c.orig
+++ modules/cache/cache_util.c
@@ -19,6 +19,7 @@
 #include "mod_cache.h"
 
 #include <ap_provider.h>
+#include <util_md5.h>
 
 /* -------------------------------------------------------------- */
 
@@ -489,54 +490,49 @@
     y[sizeof(j) * 2] = '\0';
 }
 
-static void cache_hash(const char *it, char *val, int ndepth, int nlength)
+static unsigned int cdb_string_hash(const char *str)
 {
-    apr_md5_ctx_t context;
-    unsigned char digest[16];
-    char tmp[22];
-    int i, k, d;
-    unsigned int x;
-    static const char enc_table[64] =
-    "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789_@";
-
-    apr_md5_init(&context);
-    apr_md5_update(&context, (const unsigned char *) it, strlen(it));
-    apr_md5_final(digest, &context);
-
-    /* encode 128 bits as 22 characters, using a modified uuencoding
-     * the encoding is 3 bytes -> 4 characters* i.e. 128 bits is
-     * 5 x 3 bytes + 1 byte -> 5 * 4 characters + 2 characters
-     */
-    for (i = 0, k = 0; i < 15; i += 3) {
-        x = (digest[i] << 16) | (digest[i + 1] << 8) | digest[i + 2];
-        tmp[k++] = enc_table[x >> 18];
-        tmp[k++] = enc_table[(x >> 12) & 0x3f];
-        tmp[k++] = enc_table[(x >> 6) & 0x3f];
-        tmp[k++] = enc_table[x & 0x3f];
-    }
-
-    /* one byte left */
-    x = digest[15];
-    tmp[k++] = enc_table[x >> 2];    /* use up 6 bits */
-    tmp[k++] = enc_table[(x << 4) & 0x3f];
-
-    /* now split into directory levels */
-    for (i = k = d = 0; d < ndepth; ++d) {
-        memcpy(&val[i], &tmp[k], nlength);
-        k += nlength;
-        val[i + nlength] = '/';
-        i += nlength + 1;
-    }
-    memcpy(&val[i], &tmp[k], 22 - k);
-    val[i + 22 - k] = '\0';
-}
-
-CACHE_DECLARE(char *)ap_cache_generate_name(apr_pool_t *p, int dirlevels,
-                                            int dirlength, const char *name)
-{
-    char hashfile[66];
-    cache_hash(name, hashfile, dirlevels, dirlength);
-    return apr_pstrdup(p, hashfile);
+    unsigned int hash = 5381;
+
+    while (*str) {
+        hash += (hash << 5);
+        hash ^= *str++;
+    }
+
+    return hash;
+}
+
+#define MD5_HEXDIGESTSIZE   (APR_MD5_DIGESTSIZE * 2 + 1)
+
+CACHE_DECLARE(char *)ap_cache_generate_name(apr_pool_t *p, unsigned int nlevels,
+                                            unsigned int *dirlevels,
+                                            unsigned int *divisors,
+                                            const char *name)
+{
+    char *md5_hash;
+    unsigned int i;
+    char *ptr, *key;
+    unsigned char h;
+    unsigned int cdb_hash;
+    static const char hex[] = "0123456789abcdef";
+
+    md5_hash = ap_md5_binary(p, (unsigned char *) name, (int) strlen(name));
+
+    cdb_hash = cdb_string_hash(name);
+
+    key = ptr = apr_palloc(p, nlevels * LEVEL_DIR_LENGTH +
+                           MD5_HEXDIGESTSIZE);
+
+    for (i = 0; i < nlevels; i++) {
+        h = (cdb_hash / divisors[i]) % dirlevels[i];
+        *ptr++ = hex[h >> 4];
+        *ptr++ = hex[h & 0xf];
+        *ptr++ = '/';
+    }
+
+    memcpy(ptr, md5_hash, MD5_HEXDIGESTSIZE);
+
+    return key;
 }
 
 /* Create a new table consisting of those elements from an input
Index: modules/cache/mod_cache.h
===================================================================
--- modules/cache/mod_cache.h.orig
+++ modules/cache/mod_cache.h
@@ -72,6 +72,8 @@
 
 #include "apr_atomic.h"
 
+#define LEVEL_DIR_LENGTH    3
+
 #ifndef MAX
 #define MAX(a,b)                ((a) > (b) ? (a) : (b))
 #endif
@@ -274,8 +276,9 @@
 
 CACHE_DECLARE(apr_time_t) ap_cache_hex2usec(const char *x);
 CACHE_DECLARE(void) ap_cache_usec2hex(apr_time_t j, char *y);
-CACHE_DECLARE(char *) ap_cache_generate_name(apr_pool_t *p, int dirlevels, 
-                                             int dirlength, 
+CACHE_DECLARE(char *) ap_cache_generate_name(apr_pool_t *p, unsigned int nlevels,
+                                             unsigned int *dirlevels,
+                                             unsigned int *divisors,
                                              const char *name);
 CACHE_DECLARE(cache_provider_list *)ap_cache_get_providers(request_rec *r, cache_server_conf *conf, apr_uri_t uri);
 CACHE_DECLARE(int) ap_cache_liststr(apr_pool_t *p, const char *list,
Index: modules/cache/mod_disk_cache.c
===================================================================
--- modules/cache/mod_disk_cache.c.orig
+++ modules/cache/mod_disk_cache.c
@@ -70,13 +70,14 @@
                          disk_cache_object_t *dobj, const char *name)
 {
     if (!dobj->hashfile) {
-        dobj->hashfile = ap_cache_generate_name(p, conf->dirlevels,
-                                                conf->dirlength, name);
+        dobj->hashfile = ap_cache_generate_name(p, conf->nlevels, conf->dirlevels,
+                                                conf->divisors, name);
     }
 
     if (dobj->prefix) {
         return apr_pstrcat(p, dobj->prefix, CACHE_VDIR_SUFFIX, "/",
-                           dobj->hashfile, CACHE_HEADER_SUFFIX, NULL);
+                           dobj->hashfile + (conf->nlevels + LEVEL_DIR_LENGTH),
+                           CACHE_HEADER_SUFFIX, NULL);
      }
      else {
         return apr_pstrcat(p, conf->cache_root, "/", dobj->hashfile,
@@ -88,13 +89,14 @@
                        disk_cache_object_t *dobj, const char *name)
 {
     if (!dobj->hashfile) {
-        dobj->hashfile = ap_cache_generate_name(p, conf->dirlevels,
-                                                conf->dirlength, name);
+        dobj->hashfile = ap_cache_generate_name(p, conf->nlevels, conf->dirlevels,
+                                                conf->divisors, name);
     }
 
     if (dobj->prefix) {
         return apr_pstrcat(p, dobj->prefix, CACHE_VDIR_SUFFIX, "/",
-                           dobj->hashfile, CACHE_DATA_SUFFIX, NULL);
+                           dobj->hashfile + (conf->nlevels + LEVEL_DIR_LENGTH),
+                           CACHE_DATA_SUFFIX, NULL);
      }
      else {
         return apr_pstrcat(p, conf->cache_root, "/", dobj->hashfile,
@@ -102,7 +104,7 @@
      }
 }
 
-static void mkdir_structure(disk_cache_conf *conf, const char *file, apr_pool_t *pool)
+static void mkdir_structure(disk_cache_conf *conf, char *file, apr_pool_t *pool)
 {
     apr_status_t rv;
     char *p;
@@ -123,11 +125,40 @@
     }
 }
 
+static apr_status_t disk_mktemp(apr_file_t **fp, const char *dest, char **tempfile,
+                                apr_int32_t flags, disk_cache_conf *conf,
+                                apr_pool_t *p)
+{
+    char *temp;
+    apr_status_t rv;
+    struct iovec iov[2];
+
+    iov[0].iov_base = (char *) dest;
+    iov[0].iov_len  = ap_strrchr_c(dest, '/') - dest;
+
+    iov[1].iov_base = AP_TEMPFILE;
+    iov[1].iov_len  = sizeof AP_TEMPFILE;
+
+    temp = apr_pstrcatv(p, iov, 2, NULL);
+
+    rv = apr_file_mktemp(fp, temp, flags, p);
+
+    if (APR_STATUS_IS_ENOENT(rv)) {
+        mkdir_structure(conf, temp, p);
+        memcpy(temp + iov[0].iov_len, AP_TEMPFILE, sizeof AP_TEMPFILE);
+        rv = apr_file_mktemp(fp, temp, flags, p);
+    }
+
+    *tempfile = temp;
+
+    return rv;
+}
+
 /* htcacheclean may remove directories underneath us.
  * So, we'll try renaming three times at a cost of 0.002 seconds.
  */
 static apr_status_t safe_file_rename(disk_cache_conf *conf,
-                                     const char *src, const char *dest,
+                                     const char *src, char *dest,
                                      apr_pool_t *pool)
 {
     apr_status_t rv;
@@ -359,7 +390,6 @@
     dobj->root_len = conf->cache_root_len;
     dobj->datafile = data_file(r->pool, conf, dobj, key);
     dobj->hdrsfile = header_file(r->pool, conf, dobj, key);
-    dobj->tempfile = apr_pstrcat(r->pool, conf->cache_root, AP_TEMPFILE, NULL);
 
     return OK;
 }
@@ -467,7 +497,6 @@
     dobj->key = nkey;
     dobj->name = key;
     dobj->datafile = data_file(r->pool, conf, dobj, nkey);
-    dobj->tempfile = apr_pstrcat(r->pool, conf->cache_root, AP_TEMPFILE, NULL);
 
     /* Open the data file */
     flags = APR_READ|APR_BINARY;
@@ -841,11 +870,9 @@
             apr_array_header_t* varray;
             apr_uint32_t format = VARY_FORMAT_VERSION;
 
-            mkdir_structure(conf, dobj->hdrsfile, r->pool);
-
-            rv = apr_file_mktemp(&dobj->tfd, dobj->tempfile,
-                                 APR_CREATE | APR_WRITE | APR_BINARY | APR_EXCL,
-                                 r->pool);
+            rv = disk_mktemp(&dobj->tfd, dobj->hdrsfile, &dobj->tempfile,
+                             APR_CREATE | APR_WRITE | APR_BINARY | APR_EXCL,
+                             conf, r->pool);
 
             if (rv != APR_SUCCESS) {
                 return rv;
@@ -876,7 +903,6 @@
                 return rv;
             }
 
-            dobj->tempfile = apr_pstrcat(r->pool, conf->cache_root, AP_TEMPFILE, NULL);
             tmp = regen_key(r->pool, r->headers_in, varray, dobj->name);
             dobj->prefix = dobj->hdrsfile;
             dobj->hashfile = NULL;
@@ -885,10 +911,9 @@
         }
     }
 
-
-    rv = apr_file_mktemp(&dobj->hfd, dobj->tempfile,
-                         APR_CREATE | APR_WRITE | APR_BINARY |
-                         APR_BUFFERED | APR_EXCL, r->pool);
+    rv = disk_mktemp(&dobj->hfd, dobj->hdrsfile, &dobj->tempfile,
+                     APR_CREATE | APR_WRITE | APR_BINARY | APR_BUFFERED |
+                     APR_EXCL, conf, r->pool);
 
     if (rv != APR_SUCCESS) {
         return rv;
@@ -956,9 +981,6 @@
      * about to write the new headers file.
      */
     rv = apr_file_remove(dobj->hdrsfile, r->pool);
-    if (rv != APR_SUCCESS) {
-        mkdir_structure(conf, dobj->hdrsfile, r->pool);
-    }
 
     rv = safe_file_rename(conf, dobj->tempfile, dobj->hdrsfile, r->pool);
     if (rv != APR_SUCCESS) {
@@ -969,8 +991,6 @@
         return rv;
     }
 
-    dobj->tempfile = apr_pstrcat(r->pool, conf->cache_root, AP_TEMPFILE, NULL);
-
     ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, r->server,
                  "disk_cache: Stored headers for URL %s",  dobj->name);
     return APR_SUCCESS;
@@ -989,9 +1009,9 @@
      * in file_cache_el_final().
      */
     if (!dobj->tfd) {
-        rv = apr_file_mktemp(&dobj->tfd, dobj->tempfile,
-                             APR_CREATE | APR_WRITE | APR_BINARY |
-                             APR_BUFFERED | APR_EXCL, r->pool);
+        rv = disk_mktemp(&dobj->tfd, dobj->datafile, &dobj->tempfile,
+                         APR_CREATE | APR_WRITE | APR_BINARY |
+                         APR_BUFFERED | APR_EXCL, conf, r->pool);
         if (rv != APR_SUCCESS) {
             return rv;
         }
@@ -1067,19 +1087,35 @@
     return APR_SUCCESS;
 }
 
+static void calculate_divisors(unsigned int nlevels, unsigned int *dirlevels,
+                               unsigned int *divisors)
+{
+    unsigned int i;
+
+    divisors[0] = 1;
+
+    for (i = 1; i < nlevels; i++) {
+        divisors[i] = divisors[i-1] * dirlevels[i-1];
+    }
+}
+
 static void *create_config(apr_pool_t *p, server_rec *s)
 {
+    unsigned int divisors[DEFAULT_NLEVELS];
     disk_cache_conf *conf = apr_pcalloc(p, sizeof(disk_cache_conf));
+    unsigned int dirlevels[] = { DEFAULT_DIRLEVEL1, DEFAULT_DIRLEVEL2 };
 
-    /* XXX: Set default values */
-    conf->dirlevels = DEFAULT_DIRLEVELS;
-    conf->dirlength = DEFAULT_DIRLENGTH;
+    conf->nlevels = sizeof dirlevels/sizeof *dirlevels;
+    conf->dirlevels = dirlevels;
+    conf->divisors = divisors;
     conf->maxfs = DEFAULT_MAX_FILE_SIZE;
     conf->minfs = DEFAULT_MIN_FILE_SIZE;
 
     conf->cache_root = NULL;
     conf->cache_root_len = 0;
 
+    calculate_divisors(conf->nlevels, conf->dirlevels, conf->divisors);
+
     return conf;
 }
 
@@ -1098,40 +1134,36 @@
     return NULL;
 }
 
-/*
- * Consider eliminating the next two directives in favor of
- * Ian's prime number hash...
- * key = hash_fn( r->uri)
- * filename = "/key % prime1 /key %prime2/key %prime3"
- */
 static const char
-*set_cache_dirlevels(cmd_parms *parms, void *in_struct_ptr, const char *arg)
+*set_cache_dirlevels(cmd_parms *parms, void *in_struct_ptr, int argc,
+                     char *const argv[])
 {
+    unsigned int len;
     disk_cache_conf *conf = ap_get_module_config(parms->server->module_config,
                                                  &disk_cache_module);
-    int val = atoi(arg);
-    if (val < 1)
-        return "CacheDirLevels value must be an integer greater than 0";
-    if (val * conf->dirlength > CACHEFILE_LEN)
-        return "CacheDirLevels*CacheDirLength value must not be higher than 20";
-    conf->dirlevels = val;
-    return NULL;
-}
-static const char
-*set_cache_dirlength(cmd_parms *parms, void *in_struct_ptr, const char *arg)
-{
-    disk_cache_conf *conf = ap_get_module_config(parms->server->module_config,
-                                                 &disk_cache_module);
-    int val = atoi(arg);
-    if (val < 1)
-        return "CacheDirLength value must be an integer greater than 0";
-    if (val * conf->dirlevels > CACHEFILE_LEN)
-        return "CacheDirLevels*CacheDirLength value must not be higher than 20";
 
-    conf->dirlength = val;
+    if (!argc)
+        return "The number of levels was not specified.";
+
+    conf->nlevels = atoi(*argv);
+
+    if (conf->nlevels != --argc)
+        return apr_psprintf(parms->pool, "%u levels, but only %u were given.",
+                            conf->nlevels, argc);
+
+    len = conf->nlevels * sizeof(*conf->dirlevels);
+
+    conf->dirlevels = apr_palloc(parms->pool, len);
+    conf->divisors = apr_palloc(parms->pool, len);
+
+    while (argc--) {
+        conf->dirlevels[argc] = atoi(argv[argc+1]);
+    }
+
+    calculate_divisors(conf->nlevels, conf->dirlevels, conf->divisors);
+
     return NULL;
 }
-
 static const char
 *set_cache_minfs(cmd_parms *parms, void *in_struct_ptr, const char *arg)
 {
@@ -1153,14 +1185,12 @@
 {
     AP_INIT_TAKE1("CacheRoot", set_cache_root, NULL, RSRC_CONF,
                  "The directory to store cache files"),
-    AP_INIT_TAKE1("CacheDirLevels", set_cache_dirlevels, NULL, RSRC_CONF,
-                  "The number of levels of subdirectories in the cache"),
-    AP_INIT_TAKE1("CacheDirLength", set_cache_dirlength, NULL, RSRC_CONF,
-                  "The number of characters in subdirectory names"),
     AP_INIT_TAKE1("CacheMinFileSize", set_cache_minfs, NULL, RSRC_CONF,
                   "The minimum file size to cache a document"),
     AP_INIT_TAKE1("CacheMaxFileSize", set_cache_maxfs, NULL, RSRC_CONF,
                   "The maximum file size to cache a document"),
+    AP_INIT_TAKE_ARGV("CacheDirLevels", set_cache_dirlevels, NULL, RSRC_CONF,
+                      "The number of levels of subdirectories in the cache"),
     {NULL}
 };
 
Index: modules/cache/mod_disk_cache.h
===================================================================
--- modules/cache/mod_disk_cache.h.orig
+++ modules/cache/mod_disk_cache.h
@@ -61,10 +61,10 @@
     char *tempfile;    /* temp file tohold the content */
     const char *prefix;
     const char *datafile;    /* name of file where the data will go */
-    const char *hdrsfile;    /* name of file where the hdrs will go */
+    char *hdrsfile;          /* name of file where the hdrs will go */
     const char *hashfile;    /* Computed hash key for this URI */
-    const char *name;   /* Requested URI without vary bits - suitable for mortals. */
-    const char *key;    /* On-disk prefix; URI with Vary bits (if present) */
+    const char *name;        /* Requested URI without vary bits - suitable for mortals. */
+    const char *key;         /* On-disk prefix; URI with Vary bits (if present) */
     apr_file_t *fd;          /* data file */
     apr_file_t *hfd;         /* headers file */
     apr_file_t *tfd;         /* temporary file for data */
@@ -78,16 +78,19 @@
  */
 /* TODO: Make defaults OS specific */
 #define CACHEFILE_LEN 20        /* must be less than HASH_LEN/2 */
-#define DEFAULT_DIRLEVELS 3
-#define DEFAULT_DIRLENGTH 2
+#define DEFAULT_NLEVELS 2
+#define DEFAULT_DIRLEVEL1 16
+#define DEFAULT_DIRLEVEL2 256
+#define DEFAULT_DIRLEVEL_MAX 256
 #define DEFAULT_MIN_FILE_SIZE 1
 #define DEFAULT_MAX_FILE_SIZE 1000000
 
 typedef struct {
     const char* cache_root;
     apr_size_t cache_root_len;
-    int dirlevels;               /* Number of levels of subdirectories */
-    int dirlength;               /* Length of subdirectory names */
+    unsigned int nlevels;        /* number of directories levels       */
+    unsigned int *dirlevels;     /* number of subdirs for each level   */
+    unsigned int *divisors;      /* divisor for each level             */
     apr_size_t minfs;            /* minumum file size for cached files */
     apr_size_t maxfs;            /* maximum file size for cached files */
 } disk_cache_conf;

--

[patch 01/16] dont cache expired objects

Posted by Davi Arnaut <da...@haxent.com.br>.
Don't cache requests with a expires date in the past; otherwise mod_cache will
always try to cache the URL. This bug might lead to numerous rename() errors
on win32 if the URL was previously cached.

Index: modules/cache/mod_cache.c
===================================================================
--- modules/cache/mod_cache.c.orig
+++ modules/cache/mod_cache.c
@@ -445,6 +445,11 @@
         /* if a broken Expires header is present, don't cache it */
         reason = apr_pstrcat(p, "Broken expires header: ", exps, NULL);
     }
+    else if (exp != APR_DATE_BAD && exp < r->request_time)
+    {
+        /* if a Expires header is in the past, don't cache it */
+        reason = "Expires header already expired, not cacheable";
+    }
     else if (r->args && exps == NULL) {
         /* if query string present but no expiration time, don't cache it
          * (RFC 2616/13.9)

--

[patch 07/16] shrink cache_select

Posted by Davi Arnaut <da...@haxent.com.br>.
Move parts of cache_select() to a smaller cache_check_request()
function that is easier to read and understand.

Index: modules/cache/cache_storage.c
===================================================================
--- modules/cache/cache_storage.c.orig
+++ modules/cache/cache_storage.c
@@ -169,6 +169,115 @@
     }
 }
 
+static int cache_check_request(cache_handle_t *h, request_rec *r,
+                               cache_request_rec *cache,
+                               cache_provider_list *list)
+{
+    int fresh;
+    char *vary = NULL;
+
+    /*
+     * Check Content-Negotiation - Vary
+     *
+     * At this point we need to make sure that the object we found in
+     * the cache is the same object that would be delivered to the
+     * client, when the effects of content negotiation are taken into
+     * effect.
+     *
+     * In plain english, we want to make sure that a language-negotiated
+     * document in one language is not given to a client asking for a
+     * language negotiated document in a different language by mistake.
+     *
+     * This code makes the assumption that the storage manager will
+     * cache the req_hdrs if the response contains a Vary
+     * header.
+     *
+     * RFC2616 13.6 and 14.44 describe the Vary mechanism.
+     */
+    vary = apr_pstrdup(r->pool, apr_table_get(h->resp_hdrs, "Vary"));
+    while (vary && *vary) {
+        char *name = vary;
+        const char *h1, *h2;
+
+        /* isolate header name */
+        while (*vary && !apr_isspace(*vary) && (*vary != ','))
+            ++vary;
+        while (*vary && (apr_isspace(*vary) || (*vary == ','))) {
+            *vary = '\0';
+            ++vary;
+        }
+
+        /*
+         * is this header in the request and the header in the cached
+         * request identical? If not, we give up and do a straight get
+         */
+        h1 = apr_table_get(r->headers_in, name);
+        h2 = apr_table_get(h->req_hdrs, name);
+        if (h1 == h2) {
+            /* both headers NULL, so a match - do nothing */
+        }
+        else if (h1 && h2 && !strcmp(h1, h2)) {
+            /* both headers exist and are equal - do nothing */
+        }
+        else {
+            /* headers do not match, so Vary failed */
+            ap_log_error(APLOG_MARK, APLOG_DEBUG, APR_SUCCESS,
+                        r->server,
+                        "cache_select_url(): Vary header mismatch.");
+            return DECLINED;
+        }
+    }
+
+    cache->provider = list->provider;
+    cache->provider_name = list->provider_name;
+
+    /* Is our cached response fresh enough? */
+    fresh = ap_cache_check_freshness(h, r);
+    if (!fresh) {
+        const char *etag, *lastmod;
+
+        ap_log_error(APLOG_MARK, APLOG_DEBUG, APR_SUCCESS, r->server,
+            "Cached response for %s isn't fresh.  Adding/replacing "
+            "conditional request headers.", r->uri);
+
+        /* Make response into a conditional */
+        cache->stale_headers = apr_table_copy(r->pool,
+                                                r->headers_in);
+
+        /* We can only revalidate with our own conditionals: remove the
+         * conditions from the original request.
+         */
+        apr_table_unset(r->headers_in, "If-Match");
+        apr_table_unset(r->headers_in, "If-Modified-Since");
+        apr_table_unset(r->headers_in, "If-None-Match");
+        apr_table_unset(r->headers_in, "If-Range");
+        apr_table_unset(r->headers_in, "If-Unmodified-Since");
+
+        etag = apr_table_get(h->resp_hdrs, "ETag");
+        lastmod = apr_table_get(h->resp_hdrs, "Last-Modified");
+
+        if (etag || lastmod) {
+            /* If we have a cached etag and/or Last-Modified add in
+             * our own conditionals.
+             */
+
+            if (etag) {
+                apr_table_set(r->headers_in, "If-None-Match", etag);
+            }
+
+            if (lastmod) {
+                apr_table_set(r->headers_in, "If-Modified-Since",
+                                lastmod);
+            }
+            cache->stale_handle = h;
+        }
+
+        return DECLINED;
+    }
+
+    return OK;
+}
+
 /*
  * select a specific URL entity in the cache
  *
@@ -201,110 +310,13 @@
     while (list) {
         switch ((rv = list->provider->open_entity(h, r, key))) {
         case OK: {
-            char *vary = NULL;
-            int fresh;
 
             if (list->provider->recall_headers(h, r) != APR_SUCCESS) {
                 /* TODO: Handle this error */
                 return DECLINED;
             }
 
-            /*
-             * Check Content-Negotiation - Vary
-             *
-             * At this point we need to make sure that the object we found in
-             * the cache is the same object that would be delivered to the
-             * client, when the effects of content negotiation are taken into
-             * effect.
-             *
-             * In plain english, we want to make sure that a language-negotiated
-             * document in one language is not given to a client asking for a
-             * language negotiated document in a different language by mistake.
-             *
-             * This code makes the assumption that the storage manager will
-             * cache the req_hdrs if the response contains a Vary
-             * header.
-             *
-             * RFC2616 13.6 and 14.44 describe the Vary mechanism.
-             */
-            vary = apr_pstrdup(r->pool, apr_table_get(h->resp_hdrs, "Vary"));
-            while (vary && *vary) {
-                char *name = vary;
-                const char *h1, *h2;
-
-                /* isolate header name */
-                while (*vary && !apr_isspace(*vary) && (*vary != ','))
-                    ++vary;
-                while (*vary && (apr_isspace(*vary) || (*vary == ','))) {
-                    *vary = '\0';
-                    ++vary;
-                }
-
-                /*
-                 * is this header in the request and the header in the cached
-                 * request identical? If not, we give up and do a straight get
-                 */
-                h1 = apr_table_get(r->headers_in, name);
-                h2 = apr_table_get(h->req_hdrs, name);
-                if (h1 == h2) {
-                    /* both headers NULL, so a match - do nothing */
-                }
-                else if (h1 && h2 && !strcmp(h1, h2)) {
-                    /* both headers exist and are equal - do nothing */
-                }
-                else {
-                    /* headers do not match, so Vary failed */
-                    ap_log_error(APLOG_MARK, APLOG_DEBUG, APR_SUCCESS,
-                                r->server,
-                                "cache_select_url(): Vary header mismatch.");
-                    return DECLINED;
-                }
-            }
-
-            cache->provider = list->provider;
-            cache->provider_name = list->provider_name;
-
-            /* Is our cached response fresh enough? */
-            fresh = ap_cache_check_freshness(h, r);
-            if (!fresh) {
-                const char *etag, *lastmod;
-
-                ap_log_error(APLOG_MARK, APLOG_DEBUG, APR_SUCCESS, r->server,
-                  "Cached response for %s isn't fresh.  Adding/replacing "
-                  "conditional request headers.", r->uri);
-
-                /* Make response into a conditional */
-                cache->stale_headers = apr_table_copy(r->pool,
-                                                      r->headers_in);
-
-                /* We can only revalidate with our own conditionals: remove the
-                 * conditions from the original request.
-                 */
-                apr_table_unset(r->headers_in, "If-Match");
-                apr_table_unset(r->headers_in, "If-Modified-Since");
-                apr_table_unset(r->headers_in, "If-None-Match");
-                apr_table_unset(r->headers_in, "If-Range");
-                apr_table_unset(r->headers_in, "If-Unmodified-Since");
-
-                etag = apr_table_get(h->resp_hdrs, "ETag");
-                lastmod = apr_table_get(h->resp_hdrs, "Last-Modified");
-
-                if (etag || lastmod) {
-                    /* If we have a cached etag and/or Last-Modified add in
-                     * our own conditionals.
-                     */
-
-                    if (etag) {
-                        apr_table_set(r->headers_in, "If-None-Match", etag);
-                    }
-
-                    if (lastmod) {
-                        apr_table_set(r->headers_in, "If-Modified-Since",
-                                      lastmod);
-                    }
-                    cache->stale_handle = h;
-                }
-
+            if (cache_check_request(h, r, cache, list) != OK) {
                 return DECLINED;
             }
 

--

[patch 05/16] pass uri_meets_conditions values by reference

Posted by Davi Arnaut <da...@haxent.com.br>.
Don't pass 'large' objects by value when not needed, it wastes time and space.

Index: modules/cache/cache_util.c
===================================================================
--- modules/cache/cache_util.c.orig
+++ modules/cache/cache_util.c
@@ -28,40 +28,41 @@
 /* Determine if "url" matches the hostname, scheme and port and path
  * in "filter". All but the path comparisons are case-insensitive.
  */
-static int uri_meets_conditions(apr_uri_t filter, int pathlen, apr_uri_t url)
+static int uri_meets_conditions(const apr_uri_t *filter, apr_size_t pathlen,
+                                const apr_uri_t *url)
 {
     /* Compare the hostnames */
-    if(filter.hostname) {
-        if (!url.hostname) {
+    if(filter->hostname) {
+        if (!url->hostname) {
             return 0;
         }
-        else if (strcasecmp(filter.hostname, url.hostname)) {
+        else if (strcasecmp(filter->hostname, url->hostname)) {
             return 0;
         }
     }
 
     /* Compare the schemes */
-    if(filter.scheme) {
-        if (!url.scheme) {
+    if(filter->scheme) {
+        if (!url->scheme) {
             return 0;
         }
-        else if (strcasecmp(filter.scheme, url.scheme)) {
+        else if (strcasecmp(filter->scheme, url->scheme)) {
             return 0;
         }
     }
 
     /* Compare the ports */
-    if(filter.port_str) {
-        if (url.port_str && filter.port != url.port) {
+    if(filter->port_str) {
+        if (url->port_str && filter->port != url->port) {
             return 0;
         }
         /* NOTE:  ap_port_of_scheme will return 0 if given NULL input */
-        else if (filter.port != apr_uri_port_of_scheme(url.scheme)) {
+        else if (filter->port != apr_uri_port_of_scheme(url->scheme)) {
             return 0;
         }
     }
-    else if(url.port_str && filter.scheme) {
-        if (apr_uri_port_of_scheme(filter.scheme) == url.port) {
+    else if(url->port_str && filter->scheme) {
+        if (apr_uri_port_of_scheme(filter->scheme) == url->port) {
             return 0;
         }
     }
@@ -69,12 +70,11 @@
     /* Url has met all of the filter conditions so far, determine
      * if the paths match.
      */
-    return !strncmp(filter.path, url.path, pathlen);
+    return !strncmp(filter->path, url->path, pathlen);
 }
 
 CACHE_DECLARE(cache_provider_list *)ap_cache_get_providers(request_rec *r,
-                                                  cache_server_conf *conf,
-                                                  apr_uri_t uri)
+                                                  cache_server_conf *conf)
 {
     cache_provider_list *providers = NULL;
     int i;
@@ -83,7 +83,7 @@
     for (i = 0; i < conf->cacheenable->nelts; i++) {
         struct cache_enable *ent =
                                 (struct cache_enable *)conf->cacheenable->elts;
-        if (uri_meets_conditions(ent[i].url, ent[i].pathlen, uri)) {
+        if (uri_meets_conditions(&ent[i].url, ent[i].pathlen, &r->parsed_uri)) {
             /* Fetch from global config and add to the list. */
             cache_provider *provider;
             provider = ap_lookup_provider(CACHE_PROVIDER_GROUP, ent[i].type,
@@ -120,7 +120,7 @@
     for (i = 0; i < conf->cachedisable->nelts; i++) {
         struct cache_disable *ent =
                                (struct cache_disable *)conf->cachedisable->elts;
-        if (uri_meets_conditions(ent[i].url, ent[i].pathlen, uri)) {
+        if (uri_meets_conditions(&ent[i].url, ent[i].pathlen, &r->parsed_uri)) {
             /* Stop searching now. */
             return NULL;
         }
Index: modules/cache/mod_cache.c
===================================================================
--- modules/cache/mod_cache.c.orig
+++ modules/cache/mod_cache.c
@@ -108,7 +108,7 @@
     /*
      * Which cache module (if any) should handle this request?
      */
-    if (!(providers = ap_cache_get_providers(r, conf, r->parsed_uri))) {
+    if (!(providers = ap_cache_get_providers(r, conf))) {
         return DECLINED;
     }
 
Index: modules/cache/mod_cache.h
===================================================================
--- modules/cache/mod_cache.h.orig
+++ modules/cache/mod_cache.h
@@ -280,7 +280,7 @@
                                              unsigned int *dirlevels,
                                              unsigned int *divisors,
                                              const char *name);
-CACHE_DECLARE(cache_provider_list *)ap_cache_get_providers(request_rec *r, cache_server_conf *conf, apr_uri_t uri);
+CACHE_DECLARE(cache_provider_list *)ap_cache_get_providers(request_rec *r, cache_server_conf *conf);
 CACHE_DECLARE(int) ap_cache_liststr(apr_pool_t *p, const char *list,
                                     const char *key, char **val);
 CACHE_DECLARE(const char *)ap_cache_tokstr(apr_pool_t *p, const char *list, const char **str);

--

Re: [patch 16/16] remove duplicated defines

Posted by Garrett Rooney <ro...@electricjellyfish.net>.
On 9/19/06, Davi Arnaut <da...@haxent.com.br> wrote:
> Remove duplicated defines.

Applied in r448226.  Thanks,

-garrett

[patch 16/16] remove duplicated defines

Posted by Davi Arnaut <da...@haxent.com.br>.
Remove duplicated defines.

Index: modules/cache/mod_cache.h
===================================================================
--- modules/cache/mod_cache.h.orig
+++ modules/cache/mod_cache.h
@@ -311,27 +311,6 @@
 
 /* hooks */
 
-/* Create a set of CACHE_DECLARE(type), CACHE_DECLARE_NONSTD(type) and 
- * CACHE_DECLARE_DATA with appropriate export and import tags for the platform
- */
-#if !defined(WIN32)
-#define CACHE_DECLARE(type)            type
-#define CACHE_DECLARE_NONSTD(type)     type
-#define CACHE_DECLARE_DATA
-#elif defined(CACHE_DECLARE_STATIC)
-#define CACHE_DECLARE(type)            type __stdcall
-#define CACHE_DECLARE_NONSTD(type)     type
-#define CACHE_DECLARE_DATA
-#elif defined(CACHE_DECLARE_EXPORT)
-#define CACHE_DECLARE(type)            __declspec(dllexport) type __stdcall
-#define CACHE_DECLARE_NONSTD(type)     __declspec(dllexport) type
-#define CACHE_DECLARE_DATA             __declspec(dllexport)
-#else
-#define CACHE_DECLARE(type)            __declspec(dllimport) type __stdcall
-#define CACHE_DECLARE_NONSTD(type)     __declspec(dllimport) type
-#define CACHE_DECLARE_DATA             __declspec(dllimport)
-#endif
-
 APR_DECLARE_OPTIONAL_FN(apr_status_t, 
                         ap_cache_generate_key, 
                         (request_rec *r, apr_pool_t*p, char**key ));

--

Re: [patch 09/16] simplify array and table serialization

Posted by Davi Arnaut <da...@haxent.com.br>.
On 20/09/2006, at 11:58, Davi Arnaut wrote:

>
> On 20/09/2006, at 11:00, Brian Akins wrote:
>
>> Davi Arnaut wrote:
>>> On 20/09/2006, at 10:16, Brian Akins wrote:
>>>  > Davi Arnaut wrote:
>>>  >> Simplify the array and table serialization code, separating  
>>> it from
>>>  >> the underlying I/O operations.
>>>  >
>>>  > Probably faster to just put every thing in an iovec (think  
>>> writev).
>>> Probably no, apr_brigade_writev does (quite) the same.
>>
>> Doesn't mean apr_brigade_writev does it "fast" either...
>>
>>
>> If the serialization simply returned an iovec, mod_mem_cache could  
>> use apr_pstrcatv and mod_disk_cache could use apr_file_writev.
>>
>
> There are no brigades _yet_, also pay attention to the comment:
> /* XXX this is a temporary function, it will be removed later */
>

And yes, once we have brigades between then it is much better/faster  
to use the writev
functions.

--
Davi Arnaut



Re: [patch 09/16] simplify array and table serialization

Posted by Davi Arnaut <da...@haxent.com.br>.
On 20/09/2006, at 11:00, Brian Akins wrote:

> Davi Arnaut wrote:
>> On 20/09/2006, at 10:16, Brian Akins wrote:
>>  > Davi Arnaut wrote:
>>  >> Simplify the array and table serialization code, separating it  
>> from
>>  >> the underlying I/O operations.
>>  >
>>  > Probably faster to just put every thing in an iovec (think  
>> writev).
>> Probably no, apr_brigade_writev does (quite) the same.
>
> Doesn't mean apr_brigade_writev does it "fast" either...
>
>
> If the serialization simply returned an iovec, mod_mem_cache could  
> use apr_pstrcatv and mod_disk_cache could use apr_file_writev.
>

There are no brigades _yet_, also pay attention to the comment:
/* XXX this is a temporary function, it will be removed later */

--
Davi Arnaut




Re: [patch 09/16] simplify array and table serialization

Posted by Brian Akins <br...@turner.com>.
Davi Arnaut wrote:
> 
> On 20/09/2006, at 10:16, Brian Akins wrote:
> 
>  > Davi Arnaut wrote:
>  >> Simplify the array and table serialization code, separating it from
>  >> the underlying I/O operations.
>  >
>  > Probably faster to just put every thing in an iovec (think writev).
> 
> Probably no, apr_brigade_writev does (quite) the same.

Doesn't mean apr_brigade_writev does it "fast" either...


If the serialization simply returned an iovec, mod_mem_cache could use 
apr_pstrcatv and mod_disk_cache could use apr_file_writev.


-- 
Brian Akins
Chief Operations Engineer
Turner Digital Media Technologies

Re: [patch 09/16] simplify array and table serialization

Posted by Davi Arnaut <da...@haxent.com.br>.
On 20/09/2006, at 10:16, Brian Akins wrote:

> Davi Arnaut wrote:
>> Simplify the array and table serialization code, separating it from
>> the underlying I/O operations.
>
> Probably faster to just put every thing in an iovec (think writev).

Probably no, apr_brigade_writev does (quite) the same.

--
Davi Arnaut



Re: [patch 09/16] simplify array and table serialization

Posted by Brian Akins <br...@turner.com>.
Davi Arnaut wrote:
> Simplify the array and table serialization code, separating it from
> the underlying I/O operations.
> 

Probably faster to just put every thing in an iovec (think writev).

-- 
Brian Akins
Chief Operations Engineer
Turner Digital Media Technologies

[patch 09/16] simplify array and table serialization

Posted by Davi Arnaut <da...@haxent.com.br>.
Simplify the array and table serialization code, separating it from
the underlying I/O operations.

Index: modules/cache/cache_util.c
===================================================================
--- modules/cache/cache_util.c.orig
+++ modules/cache/cache_util.c
@@ -25,6 +25,126 @@
 
 extern module AP_MODULE_DECLARE_DATA cache_module;
 
+#define CLSP        ": "
+#define ARRAY_SEP   ','
+
+static APR_INLINE void *ap_mempcpy(void *dest, const void *src, size_t n)
+{
+    return memcpy(dest, src, n) + n;
+}
+
+/* XXX this function will change later */
+char *table_serialize(apr_pool_t *p, apr_table_t *table, apr_size_t *len)
+{
+    char *str, *pstr;
+    apr_size_t *tlen;
+    const apr_array_header_t *header = apr_table_elts(table);
+    apr_table_entry_t *entry = (apr_table_entry_t *) header->elts;
+    int i, n, nelts = header->nelts;
+
+    tlen = apr_palloc(p, sizeof(int) * nelts * 2);
+
+    *len = 0;
+
+    for (i = 0, n = 0; i < nelts; i++) {
+        *len += tlen[n++] = strlen(entry[i].key);
+        *len += tlen[n++] = strlen(entry[i].val);
+    }
+
+    /* key ': ' val '\r\n' */
+    *len += nelts * 4;
+
+    pstr = str = apr_palloc(p, *len + 1);
+
+    for (i = 0, n = 0; i < nelts; i++) {
+        pstr = ap_mempcpy(pstr, entry[i].key, tlen[n++]);
+        pstr = ap_mempcpy(pstr, CLSP, sizeof(CLSP) - 1);
+        pstr = ap_mempcpy(pstr, entry[i].val, tlen[n++]);
+        pstr = ap_mempcpy(pstr, CRLF, sizeof(CRLF) - 1);
+    }
+
+    *pstr = '\0';
+
+    return str;
+}
+
+/* XXX this function will change later */
+char *array_serialize(apr_pool_t *p, apr_array_header_t *array, apr_size_t *len)
+{
+    char *str;
+
+    str = apr_array_pstrcat(p, array, ARRAY_SEP);
+
+    *len = strlen(str);
+
+    return str;
+}
+
+char *ap_cache_strtok(char *str, apr_size_t *slen, const char *token,
+                      apr_size_t tlen)
+{
+    char *pstr = str;
+    apr_size_t i, len = *slen - tlen;
+
+    for (i = 0; i <= len; i++, pstr++) {
+        if (!memcmp(pstr, token, tlen)) {
+            break;
+        }
+    }
+
+    if (i > len)
+        return NULL;
+
+    *pstr = '\0';
+
+    *slen -= i + tlen;
+
+    return pstr + tlen;
+}
+
+/* XXX this function will change later */
+apr_size_t table_unserialize(apr_table_t *table, char *str, apr_size_t len)
+{
+    char *key, *val, *pstr = str;
+
+    do {
+        key = pstr;
+
+        pstr = ap_cache_strtok(pstr, &len, CLSP, sizeof CLSP - 1);
+
+        if (pstr == NULL)
+            break;
+
+        val = pstr;
+
+        pstr = ap_cache_strtok(pstr, &len, CRLF, sizeof CRLF - 1);
+
+        if (pstr == NULL)
+            break;
+
+        apr_table_add(table, key, val);
+    } while (len);
+
+    return len;
+}
+
+/* XXX this function will change later */
+void array_unserialize(apr_array_header_t *array, char *str)
+{
+    char *entry, *pstr = str;
+
+    do {
+        APR_ARRAY_PUSH(array, char *) = entry = pstr;
+
+        pstr = strchr(pstr, ARRAY_SEP);
+
+        if (pstr == NULL)
+            break;
+
+        *pstr++ = '\0';
+    } while (1);
+}
+
 /* Determine if "url" matches the hostname, scheme and port and path
  * in "filter". All but the path comparisons are case-insensitive.
  */
Index: modules/cache/mod_cache.h
===================================================================
--- modules/cache/mod_cache.h.orig
+++ modules/cache/mod_cache.h
@@ -252,6 +252,13 @@
 
 
 /* cache_util.c */
+char *ap_cache_strtok(char *str, apr_size_t *slen, const char *token,
+                      apr_size_t tlen);
+char *table_serialize(apr_pool_t *p, apr_table_t *table, apr_size_t *len);
+apr_size_t table_unserialize(apr_table_t *table, char *str, apr_size_t len);
+char *array_serialize(apr_pool_t *p, apr_array_header_t *array, apr_size_t *len);
+void array_unserialize(apr_array_header_t *array, char *str);
+
 /* do a HTTP/1.1 age calculation */
 CACHE_DECLARE(apr_time_t) ap_cache_current_age(cache_info *info, const apr_time_t age_value,
                                                apr_time_t now);
Index: modules/cache/mod_disk_cache.c
===================================================================
--- modules/cache/mod_disk_cache.c.orig
+++ modules/cache/mod_disk_cache.c
@@ -45,10 +45,10 @@
  * Format #2:
  *   disk_cache_info_t (first sizeof(apr_uint32_t) bytes is the format)
  *   entity name (dobj->name) [length is in disk_cache_info_t->name_len]
+ *   size of headers_out (delimited by CRLF)
  *   r->headers_out (delimited by CRLF)
- *   CRLF
+ *   size of headers_in (delimited by CRLF)
  *   r->headers_in (delimited by CRLF)
- *   CRLF
  */
 
 module AP_MODULE_DECLARE_DATA disk_cache_module;
@@ -59,8 +59,7 @@
 static apr_status_t store_body(cache_handle_t *h, request_rec *r, apr_bucket_brigade *b);
 static apr_status_t recall_headers(cache_handle_t *h, request_rec *r);
 static apr_status_t recall_body(cache_handle_t *h, apr_pool_t *p, apr_bucket_brigade *bb);
-static apr_status_t read_array(request_rec *r, apr_array_header_t* arr,
-                               apr_file_t *file);
+static apr_status_t read_array(apr_array_header_t* arr, apr_file_t *file);
 
 /*
  * Local static functions
@@ -457,7 +456,7 @@
         }
 
         varray = apr_array_make(r->pool, 5, sizeof(char*));
-        rc = read_array(r, varray, dobj->hfd);
+        rc = read_array(varray, dobj->hfd);
         if (rc != APR_SUCCESS) {
             ap_log_error(APLOG_MARK, APLOG_ERR, rc, r->server,
                          "disk_cache: Cannot parse vary header file: %s",
@@ -582,149 +581,127 @@
     return OK;
 }
 
-static apr_status_t read_array(request_rec *r, apr_array_header_t* arr,
-                               apr_file_t *file)
+/* XXX this is a temporary function, it will be removed later */
+static apr_status_t read_array(apr_array_header_t* array, apr_file_t *file)
 {
-    char w[MAX_STRING_LEN];
-    int p;
     apr_status_t rv;
+    apr_size_t nbytes;
+    apr_finfo_t finfo;
+    char *buffer, *end;
 
-    while (1) {
-        rv = apr_file_gets(w, MAX_STRING_LEN - 1, file);
-        if (rv != APR_SUCCESS) {
-            ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r,
-                          "Premature end of vary array.");
-            return rv;
-        }
+    rv = apr_file_info_get(&finfo, APR_FINFO_SIZE, file);
 
-        p = strlen(w);
-        if (p > 0 && w[p - 1] == '\n') {
-            if (p > 1 && w[p - 2] == CR) {
-                w[p - 2] = '\0';
-            }
-            else {
-                w[p - 1] = '\0';
-            }
-        }
+    if (rv != APR_SUCCESS) {
+        return rv;
+    }
 
-        /* If we've finished reading the array, break out of the loop. */
-        if (w[0] == '\0') {
-            break;
-        }
+    nbytes = finfo.size;
 
-       *((const char **) apr_array_push(arr)) = apr_pstrdup(r->pool, w);
+    buffer = apr_palloc(array->pool, nbytes);
+
+    rv = apr_file_read(file, buffer, &nbytes);
+
+    if (rv != APR_SUCCESS) {
+        return rv;
     }
 
+    end = memchr(buffer, CR, nbytes);
+
+    if (end == NULL) {
+        return APR_EGENERAL;
+    }
+
+    *end = '\0';
+
+    array_unserialize(array, buffer);
+
     return APR_SUCCESS;
 }
 
-static apr_status_t store_array(apr_file_t *fd, apr_array_header_t* arr)
+/* XXX this is a temporary function, it will be removed later */
+static apr_status_t store_array(apr_pool_t *p, apr_file_t *fd,
+                                apr_array_header_t* array)
 {
-    int i;
+    char *str;
+    apr_size_t len;
+
+    str = array_serialize(p, array, &len);
+
+    if (apr_file_printf(fd, "%s" CRLF, str) < 0)
+        return APR_EGENERAL;
+    else
+        return APR_SUCCESS;
+}
+
+/* XXX this is a temporary function, it will be removed later */
+static apr_status_t read_table(apr_pool_t *p, apr_table_t *table, apr_file_t *fd)
+{
+    char *end, *buf;
     apr_status_t rv;
-    struct iovec iov[2];
-    apr_size_t amt;
-    const char **elts;
+    apr_size_t nbytes;
+    apr_off_t offset = 0;
+    apr_size_t bytes_read = 0;
+    char buffer[HUGE_STRING_LEN];
 
-    elts = (const char **) arr->elts;
+    buf = buffer;
+    nbytes = sizeof buffer;
 
-    for (i = 0; i < arr->nelts; i++) {
-        iov[0].iov_base = (char*) elts[i];
-        iov[0].iov_len = strlen(elts[i]);
-        iov[1].iov_base = CRLF;
-        iov[1].iov_len = sizeof(CRLF) - 1;
+    rv = apr_file_seek(fd, APR_CUR, &offset);
 
-        rv = apr_file_writev(fd, (const struct iovec *) &iov, 2,
-                             &amt);
-        if (rv != APR_SUCCESS) {
-            return rv;
-        }
+    if (rv != APR_SUCCESS) {
+        return rv;
     }
 
-    iov[0].iov_base = CRLF;
-    iov[0].iov_len = sizeof(CRLF) - 1;
+    /* read the table length */
+    rv = apr_file_read(fd, buf, &nbytes);
 
-    return apr_file_writev(fd, (const struct iovec *) &iov, 1,
-                         &amt);
-}
+    if (rv != APR_SUCCESS) {
+        return rv;
+    }
 
-static apr_status_t read_table(cache_handle_t *handle, request_rec *r,
-                               apr_table_t *table, apr_file_t *file)
-{
-    char w[MAX_STRING_LEN];
-    char *l;
-    int p;
-    apr_status_t rv;
+    end = ap_cache_strtok(buf, &nbytes, CRLF, sizeof CRLF - 1);
 
-    while (1) {
+    if (end == NULL) {
+        return APR_EGENERAL;
+    }
 
-        /* ### What about APR_EOF? */
-        rv = apr_file_gets(w, MAX_STRING_LEN - 1, file);
-        if (rv != APR_SUCCESS) {
-            ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r,
-                          "Premature end of cache headers.");
-            return rv;
-        }
+    offset += end - buf;
 
-        /* Delete terminal (CR?)LF */
+    /* set the offset past the table size */
+    rv = apr_file_seek(fd, APR_SET, &offset);
 
-        p = strlen(w);
-        /* Indeed, the host's '\n':
-           '\012' for UNIX; '\015' for MacOS; '\025' for OS/390
-           -- whatever the script generates.
-        */
-        if (p > 0 && w[p - 1] == '\n') {
-            if (p > 1 && w[p - 2] == CR) {
-                w[p - 2] = '\0';
-            }
-            else {
-                w[p - 1] = '\0';
-            }
-        }
+    if (rv != APR_SUCCESS) {
+        return rv;
+    }
 
-        /* If we've finished reading the headers, break out of the loop. */
-        if (w[0] == '\0') {
-            break;
-        }
+    rv = apr_strtoff(&offset, buf, &end, 0);
 
-#if APR_CHARSET_EBCDIC
-        /* Chances are that we received an ASCII header text instead of
-         * the expected EBCDIC header lines. Try to auto-detect:
-         */
-        if (!(l = strchr(w, ':'))) {
-            int maybeASCII = 0, maybeEBCDIC = 0;
-            unsigned char *cp, native;
-            apr_size_t inbytes_left, outbytes_left;
-
-            for (cp = w; *cp != '\0'; ++cp) {
-                native = apr_xlate_conv_byte(ap_hdrs_from_ascii, *cp);
-                if (apr_isprint(*cp) && !apr_isprint(native))
-                    ++maybeEBCDIC;
-                if (!apr_isprint(*cp) && apr_isprint(native))
-                    ++maybeASCII;
-            }
-            if (maybeASCII > maybeEBCDIC) {
-                ap_log_error(APLOG_MARK, APLOG_ERR, 0, r->server,
-                             "CGI Interface Error: Script headers apparently ASCII: (CGI = %s)",
-                             r->filename);
-                inbytes_left = outbytes_left = cp - w;
-                apr_xlate_conv_buffer(ap_hdrs_from_ascii,
-                                      w, &inbytes_left, w, &outbytes_left);
-            }
-        }
-#endif /*APR_CHARSET_EBCDIC*/
+    if (rv != APR_SUCCESS) {
+        return rv;
+    }
 
-        /* if we see a bogus header don't ignore it. Shout and scream */
-        if (!(l = strchr(w, ':'))) {
-            return APR_EGENERAL;
-        }
+    /* let's read and parse the table */
+    nbytes = (apr_size_t) offset;
 
-        *l++ = '\0';
-        while (*l && apr_isspace(*l)) {
-            ++l;
-        }
+    buf = buffer;
+
+    if (nbytes > sizeof buffer) {
+        buf = apr_palloc(p, nbytes);
+    }
+
+    rv = apr_file_read_full(fd, buf, nbytes, &bytes_read);
 
-        apr_table_add(table, w, l);
+    if (rv != APR_SUCCESS) {
+        return rv;
+    }
+    else if (nbytes != bytes_read) {
+        return APR_EGENERAL;
+    }
+
+    nbytes = table_unserialize(table, buf, bytes_read);
+
+    if (nbytes) {
+        return APR_EGENERAL;
     }
 
     return APR_SUCCESS;
@@ -751,8 +728,8 @@
     h->resp_hdrs = apr_table_make(r->pool, 20);
 
     /* Call routine to read the header lines/status line */
-    read_table(h, r, h->resp_hdrs, dobj->hfd);
-    read_table(h, r, h->req_hdrs, dobj->hfd);
+    read_table(r->pool, h->resp_hdrs, dobj->hfd);
+    read_table(r->pool, h->req_hdrs, dobj->hfd);
 
     apr_file_close(dobj->hfd);
 
@@ -774,38 +751,25 @@
     return APR_SUCCESS;
 }
 
-static apr_status_t store_table(apr_file_t *fd, apr_table_t *table)
+static apr_status_t store_table(apr_pool_t *p, apr_file_t *fd, apr_table_t *table)
 {
-    int i;
+    char *pstr;
+    apr_size_t len;
     apr_status_t rv;
-    struct iovec iov[4];
-    apr_size_t amt;
-    apr_table_entry_t *elts;
 
-    elts = (apr_table_entry_t *) apr_table_elts(table)->elts;
-    for (i = 0; i < apr_table_elts(table)->nelts; ++i) {
-        if (elts[i].key != NULL) {
-            iov[0].iov_base = elts[i].key;
-            iov[0].iov_len = strlen(elts[i].key);
-            iov[1].iov_base = ": ";
-            iov[1].iov_len = sizeof(": ") - 1;
-            iov[2].iov_base = elts[i].val;
-            iov[2].iov_len = strlen(elts[i].val);
-            iov[3].iov_base = CRLF;
-            iov[3].iov_len = sizeof(CRLF) - 1;
+    pstr = table_serialize(p, table, &len);
 
-            rv = apr_file_writev(fd, (const struct iovec *) &iov, 4,
-                                 &amt);
-            if (rv != APR_SUCCESS) {
-                return rv;
-            }
-        }
+    if (pstr == NULL) {
+        return APR_ENOMEM;
     }
-    iov[0].iov_base = CRLF;
-    iov[0].iov_len = sizeof(CRLF) - 1;
-    rv = apr_file_writev(fd, (const struct iovec *) &iov, 1,
-                         &amt);
-    return rv;
+
+    rv = apr_file_printf(fd, "%" APR_SIZE_T_FMT CRLF, len);
+
+    if (rv < 0) {
+        return APR_EGENERAL;
+    }
+
+    return apr_file_write_full(fd, pstr, len, NULL);
 }
 
 static apr_status_t store_headers(cache_handle_t *h, request_rec *r, cache_info *info)
@@ -848,7 +812,7 @@
             varray = apr_array_make(r->pool, 6, sizeof(char*));
             tokens_to_array(r->pool, tmp, varray);
 
-            store_array(dobj->tfd, varray);
+            store_array(r->pool, dobj->tfd, varray);
 
             apr_file_close(dobj->tfd);
 
@@ -916,7 +880,7 @@
 
         headers_out = apr_table_overlay(r->pool, headers_out,
                                         r->err_headers_out);
-        rv = store_table(dobj->hfd, headers_out);
+        rv = store_table(r->pool, dobj->hfd, headers_out);
         if (rv != APR_SUCCESS) {
             return rv;
         }
@@ -929,7 +893,7 @@
 
         headers_in = ap_cache_cacheable_hdrs_out(r->pool, r->headers_in,
                                                  r->server);
-        rv = store_table(dobj->hfd, headers_in);
+        rv = store_table(r->pool, dobj->hfd, headers_in);
         if (rv != APR_SUCCESS) {
             return rv;
         }

--

[patch 14/16] remove unneeded casts

Posted by Davi Arnaut <da...@haxent.com.br>.
Remove unneeded casts.

Index: modules/cache/mod_disk_cache.c
===================================================================
--- modules/cache/mod_disk_cache.c.orig
+++ modules/cache/mod_disk_cache.c
@@ -116,7 +116,7 @@
     }
 }
 
-static apr_status_t disk_mktemp(apr_file_t **fp, const char *dest, char **tempfile,
+static apr_status_t disk_mktemp(apr_file_t **fp, char *dest, char **tempfile,
                                 apr_int32_t flags, disk_cache_conf *conf,
                                 apr_pool_t *p)
 {
@@ -124,7 +124,7 @@
     apr_status_t rv;
     struct iovec iov[2];
 
-    iov[0].iov_base = (char *) dest;
+    iov[0].iov_base = dest;
     iov[0].iov_len  = ap_strrchr_c(dest, '/') - dest;
 
     iov[1].iov_base = AP_TEMPFILE;
@@ -382,12 +382,11 @@
     char *token;
 
     while ((token = ap_get_list_item(p, &data)) != NULL) {
-        *((const char **) apr_array_push(arr)) = token;
+        APR_ARRAY_PUSH(arr, char *) = 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);
+    qsort(arr->elts, arr->nelts, sizeof(char *), array_alphasort);
 }
 
 /*

--

[patch 15/16] fix a FIXME (clean cache_object_t)

Posted by Davi Arnaut <da...@haxent.com.br>.
Move members from cache_object_t to mem_cache_object_t that are only
required for mod_mem_cache.

Index: modules/cache/mod_cache.h
===================================================================
--- modules/cache/mod_cache.h.orig
+++ modules/cache/mod_cache.h
@@ -181,23 +181,12 @@
 
 /* cache handle information */
 
-/* XXX TODO On the next structure change/MMN bump, 
- * count must become an apr_off_t, representing
- * the potential size of disk cached objects.
- * Then dig for
- * "XXX Bad Temporary Cast - see cache_object_t notes" 
- */
 typedef struct cache_object cache_object_t;
 struct cache_object {
     const char *key;
-    cache_object_t *next;
     cache_info info;
     /* Opaque portion (specific to the implementation) of the cache object */
     void *vobj;
-    /* FIXME: These are only required for mod_mem_cache. */
-    apr_size_t count;   /* Number of body bytes written to the cache so far */
-    int complete;
-    apr_uint32_t refcount;  /* refcount and bit flag to cleanup object */
 };
 
 typedef struct cache_handle cache_handle_t;
Index: modules/cache/mod_mem_cache.c
===================================================================
--- modules/cache/mod_mem_cache.c.orig
+++ modules/cache/mod_mem_cache.c
@@ -71,9 +71,10 @@
     apr_int32_t flags;          /* File open flags */
     long priority;      /**< the priority of this entry */
     long total_refs;          /**< total number of references this entry has had */
-
     apr_uint32_t pos;   /**< the position of this entry in the cache */
-
+    apr_off_t count;   /* Number of body bytes written to the cache so far */
+    int complete;
+    apr_uint32_t refcount;  /* refcount and bit flag to cleanup object */
 } mem_cache_object_t;
 
 typedef struct
@@ -162,11 +163,12 @@
 static void memcache_cache_free(void *a)
 {
     cache_object_t *obj = (cache_object_t *) a;
+    mem_cache_object_t *mobj = obj->vobj;
 
     /* Decrement the refcount to account for the object being ejected
      * from the cache. If the refcount is 0, free the object.
      */
-    if (!apr_atomic_dec32(&obj->refcount)) {
+    if (!apr_atomic_dec32(&mobj->refcount)) {
         cleanup_cache_object(obj);
     }
 }
@@ -225,6 +227,7 @@
 static apr_status_t decrement_refcount(void *arg)
 {
     cache_object_t *obj = (cache_object_t *) arg;
+    mem_cache_object_t *mobj = obj->vobj;
 
     /* If obj->complete is not set, the cache update failed and the
      * object needs to be removed from the cache then cleaned up.
@@ -232,7 +235,7 @@
      * cache already, so make sure it is really still in the cache
      * before attempting to remove it.
      */
-    if (!obj->complete) {
+    if (!mobj->complete) {
         cache_object_t *tobj = NULL;
         if (sconf->lock) {
             apr_thread_mutex_lock(sconf->lock);
@@ -240,7 +243,7 @@
         tobj = cache_find(sconf->cache_cache, obj->key);
         if (tobj == obj) {
             cache_remove(sconf->cache_cache, obj);
-            apr_atomic_dec32(&obj->refcount);
+            apr_atomic_dec32(&mobj->refcount);
         }
         if (sconf->lock) {
             apr_thread_mutex_unlock(sconf->lock);
@@ -248,7 +251,7 @@
     }
 
     /* If the refcount drops to 0, cleanup the cache object */
-    if (!apr_atomic_dec32(&obj->refcount)) {
+    if (!apr_atomic_dec32(&mobj->refcount)) {
         cleanup_cache_object(obj);
     }
     return APR_SUCCESS;
@@ -257,6 +260,7 @@
 static apr_status_t cleanup_cache_mem(void *sconfv)
 {
     cache_object_t *obj;
+    mem_cache_object_t *mobj;
     mem_cache_conf *co = (mem_cache_conf *) sconfv;
 
     if (!co) {
@@ -271,8 +275,9 @@
     }
     obj = cache_pop(co->cache_cache);
     while (obj) {
+        mobj = obj->vobj;
         /* Iterate over the cache and clean up each unreferenced entry */
-        if (!apr_atomic_dec32(&obj->refcount)) {
+        if (!apr_atomic_dec32(&mobj->refcount)) {
             cleanup_cache_object(obj);
         }
         obj = cache_pop(co->cache_cache);
@@ -364,9 +369,9 @@
     mobj->pool = pool;
 
     /* Finish initing the cache object */
-    apr_atomic_set32(&obj->refcount, 1);
+    apr_atomic_set32(&mobj->refcount, 1);
     mobj->total_refs = 1;
-    obj->complete = 0;
+    mobj->complete = 0;
     obj->vobj = mobj;
     /* Safe cast: We tested < sconf->max_cache_object_size above */
     mobj->m_len = (apr_size_t) len;
@@ -390,12 +395,13 @@
     tmp_obj = (cache_object_t *) cache_find(sconf->cache_cache, key);
 
     if (!tmp_obj) {
+        mem_cache_object_t *tmp_mobj = obj->vobj;
         cache_insert(sconf->cache_cache, obj);
         /* Add a refcount to account for the reference by the
          * hashtable in the cache. Refcount should be 2 now, one
          * for this thread, and one for the cache.
          */
-        apr_atomic_inc32(&obj->refcount);
+        apr_atomic_inc32(&tmp_mobj->refcount);
     }
     if (sconf->lock) {
         apr_thread_mutex_unlock(sconf->lock);
@@ -434,6 +440,7 @@
 static int open_entity(cache_handle_t * h, request_rec *r, const char *key)
 {
     cache_object_t *obj;
+    mem_cache_object_t *mobj;
 
     /* Look up entity keyed to 'url' */
     if (sconf->lock) {
@@ -441,9 +448,10 @@
     }
     obj = (cache_object_t *) cache_find(sconf->cache_cache, key);
     if (obj) {
-        if (obj->complete) {
+        mobj = obj->vobj;
+        if (mobj->complete) {
             request_rec *rmain = r, *rtmp;
-            apr_atomic_inc32(&obj->refcount);
+            apr_atomic_inc32(&mobj->refcount);
             /* cache is worried about overall counts, not 'open' ones */
             cache_update(sconf->cache_cache, obj);
 
@@ -502,8 +510,9 @@
      */
     tobj = cache_find(sconf->cache_cache, obj->key);
     if (tobj == obj) {
+        mem_cache_object_t *mobj = obj->vobj;
         cache_remove(sconf->cache_cache, obj);
-        apr_atomic_dec32(&obj->refcount);
+        apr_atomic_dec32(&mobj->refcount);
     }
 
     if (sconf->lock) {
@@ -525,9 +534,10 @@
 
     obj = h->cache_obj;
     if (obj) {
+        mem_cache_object_t *mobj = obj->vobj;
         cache_remove(sconf->cache_cache, obj);
         /* For performance, cleanup cache object after releasing the lock */
-        cleanup = !apr_atomic_dec32(&obj->refcount);
+        cleanup = !apr_atomic_dec32(&mobj->refcount);
     }
     if (sconf->lock) {
         apr_thread_mutex_unlock(sconf->lock);
@@ -651,7 +661,7 @@
          * so decrement the refcount
          * apr_atomic_dec32(&obj->refcount);
          */
-        mobj->m_len = obj->count;
+        mobj->m_len = mobj->count;
 
         cache_insert(sconf->cache_cache, obj);
         /* For illustration, cache now has reference to the object, so
@@ -668,9 +678,9 @@
     }
     else {
         /* Object has been ejected from the cache, add it back to the cache */
-        mobj->m_len = obj->count;
+        mobj->m_len = mobj->count;
         cache_insert(sconf->cache_cache, obj);
-        apr_atomic_inc32(&obj->refcount);
+        apr_atomic_inc32(&mobj->refcount);
     }
 
     if (sconf->lock) {
@@ -733,7 +743,7 @@
             ap_log_error(APLOG_MARK, APLOG_INFO, 0, r->server,
                          "mem_cache: Cached file: %s with key: %s", name,
                          obj->key);
-            obj->complete = 1;
+            mobj->complete = 1;
             return APR_SUCCESS;
         }
 
@@ -750,9 +760,9 @@
         if (mobj->m == NULL) {
             return APR_ENOMEM;
         }
-        obj->count = 0;
+        mobj->count = 0;
     }
-    cur = (char *) mobj->m + obj->count;
+    cur = (char *) mobj->m + mobj->count;
 
     /* Iterate accross the brigade and populate the cache storage */
     for (e = APR_BRIGADE_FIRST(b);
@@ -761,11 +771,11 @@
         apr_size_t len;
 
         if (APR_BUCKET_IS_EOS(e)) {
-            if (mobj->m_len > obj->count) {
+            if (mobj->m_len > mobj->count) {
                 /* Caching a streamed response. Reallocate a buffer of the
                  * correct size and copy the streamed response into that
                  * buffer */
-                mobj->m = realloc(mobj->m, obj->count);
+                mobj->m = realloc(mobj->m, mobj->count);
 
                 if (!mobj->m) {
                     return APR_ENOMEM;
@@ -776,7 +786,7 @@
             /* Open for business */
             ap_log_error(APLOG_MARK, APLOG_INFO, 0, r->server,
                          "mem_cache: Cached url: %s", obj->key);
-            obj->complete = 1;
+            mobj->complete = 1;
             break;
         }
         rv = apr_bucket_read(e, &s, &len, eblock);
@@ -785,19 +795,19 @@
         }
         if (len) {
             /* Check for buffer overflow */
-            if ((obj->count + len) > mobj->m_len) {
+            if ((mobj->count + len) > mobj->m_len) {
                 return APR_ENOMEM;
             }
             else {
                 memcpy(cur, s, len);
                 cur += len;
-                obj->count += len;
+                mobj->count += len;
             }
         }
         /* This should not fail, but if it does, we are in BIG trouble
          * cause we just stomped all over the heap.
          */
-        AP_DEBUG_ASSERT(obj->count <= mobj->m_len);
+        AP_DEBUG_ASSERT(mobj->count <= mobj->m_len);
     }
     return APR_SUCCESS;
 }
Index: include/ap_mmn.h
===================================================================
--- include/ap_mmn.h.orig
+++ include/ap_mmn.h
@@ -123,12 +123,14 @@
  * 20060110.4 (2.3.0-dev)  Added server_scheme member to server_rec (minor)
  * 20060905.0 (2.3.0-dev)  Replaced ap_get_server_version() with
  *                         ap_get_server_banner() and ap_get_server_description()
+ * 20060917.0 (2.3.0-dev)  Moved members from cache_object_t to mem_cache_object_t
+ *                         that are only required for mod_mem_cache
  */
 
 #define MODULE_MAGIC_COOKIE 0x41503234UL /* "AP24" */
 
 #ifndef MODULE_MAGIC_NUMBER_MAJOR
-#define MODULE_MAGIC_NUMBER_MAJOR 20060905
+#define MODULE_MAGIC_NUMBER_MAJOR 20060917
 #endif
 #define MODULE_MAGIC_NUMBER_MINOR 0                     /* 0...n */
 

--

[patch 04/16] shrink cache_url_handler

Posted by Davi Arnaut <da...@haxent.com.br>.
Move parts of cache_url_handler() to a smaller add_cache_filters()
function that is easier to read and understand.

Index: modules/cache/mod_cache.c
===================================================================
--- modules/cache/mod_cache.c.orig
+++ modules/cache/mod_cache.c
@@ -48,6 +48,44 @@
  *     oh well.
  */
 
+static void add_cache_filters(request_rec *r, cache_request_rec *cache)
+{
+    /*
+     * Add cache_save filter to cache this request. Choose
+     * the correct filter by checking if we are a subrequest
+     * or not.
+     */
+    if (r->main) {
+        ap_log_error(APLOG_MARK, APLOG_DEBUG, APR_SUCCESS,
+                        r->server,
+                        "Adding CACHE_SAVE_SUBREQ filter for %s",
+                        r->uri);
+        ap_add_output_filter_handle(cache_save_subreq_filter_handle,
+                                    NULL, r, r->connection);
+    }
+    else {
+        ap_log_error(APLOG_MARK, APLOG_DEBUG, APR_SUCCESS,
+                        r->server, "Adding CACHE_SAVE filter for %s",
+                        r->uri);
+        ap_add_output_filter_handle(cache_save_filter_handle,
+                                    NULL, r, r->connection);
+    }
+
+    ap_log_error(APLOG_MARK, APLOG_DEBUG, APR_SUCCESS, r->server,
+                    "Adding CACHE_REMOVE_URL filter for %s",
+                    r->uri);
+
+    /* Add cache_remove_url filter to this request to remove a
+     * stale cache entry if needed. Also put the current cache
+     * request rec in the filter context, as the request that
+     * is available later during running the filter maybe
+     * different due to an internal redirect.
+     */
+    cache->remove_url_filter =
+        ap_add_output_filter_handle(cache_remove_url_filter_handle,
+                                    cache, r, r->connection);
+}
+
 static int cache_url_handler(request_rec *r, int lookup)
 {
     apr_status_t rv;
@@ -113,41 +151,7 @@
     if (rv != OK) {
         if (rv == DECLINED) {
             if (!lookup) {
-
-                /*
-                 * Add cache_save filter to cache this request. Choose
-                 * the correct filter by checking if we are a subrequest
-                 * or not.
-                 */
-                if (r->main) {
-                    ap_log_error(APLOG_MARK, APLOG_DEBUG, APR_SUCCESS,
-                                 r->server,
-                                 "Adding CACHE_SAVE_SUBREQ filter for %s",
-                                 r->uri);
-                    ap_add_output_filter_handle(cache_save_subreq_filter_handle,
-                                                NULL, r, r->connection);
-                }
-                else {
-                    ap_log_error(APLOG_MARK, APLOG_DEBUG, APR_SUCCESS,
-                                 r->server, "Adding CACHE_SAVE filter for %s",
-                                 r->uri);
-                    ap_add_output_filter_handle(cache_save_filter_handle,
-                                                NULL, r, r->connection);
-                }
-
-                ap_log_error(APLOG_MARK, APLOG_DEBUG, APR_SUCCESS, r->server,
-                             "Adding CACHE_REMOVE_URL filter for %s",
-                             r->uri);
-
-                /* Add cache_remove_url filter to this request to remove a
-                 * stale cache entry if needed. Also put the current cache
-                 * request rec in the filter context, as the request that
-                 * is available later during running the filter maybe
-                 * different due to an internal redirect.
-                 */
-                cache->remove_url_filter =
-                    ap_add_output_filter_handle(cache_remove_url_filter_handle,
-                                                cache, r, r->connection);
+                add_cache_filters(r, cache);
             }
             else {
                 if (cache->stale_headers) {

--

[patch 11/16] fix memleak in mod_mem_cache

Posted by Davi Arnaut <da...@haxent.com.br>.
Unconditionally free the buffer.

Index: modules/cache/mod_mem_cache.c
===================================================================
--- modules/cache/mod_mem_cache.c.orig
+++ modules/cache/mod_mem_cache.c
@@ -227,7 +227,7 @@
 
     /* Cleanup the mem_cache_object_t */
     if (mobj) {
-        if (mobj->type == CACHE_TYPE_HEAP && mobj->m) {
+        if (mobj->m) {
             free(mobj->m);
         }
         if (mobj->type == CACHE_TYPE_FILE && mobj->fd) {

--

[patch 06/16] dont choke on empty URI path

Posted by Davi Arnaut <da...@haxent.com.br>.
According to my reading of RFC3986 (section 6.2.3.) if a URI contains an
authority component and an empty path, the empty path is to be equivalent
to "/".

It explicitly cites the following four URIs as equivalents:

    http://example.com
    http://example.com/
    http://example.com:/
    http://example.com:80/

Index: modules/cache/cache_util.c
===================================================================
--- modules/cache/cache_util.c.orig
+++ modules/cache/cache_util.c
@@ -67,9 +67,23 @@
         }
     }
 
+    /* For HTTP caching purposes, an empty (NULL) path is equivalent to
+     * a single "/" path. RFCs 3986/2396
+     */
+
+    if (!url->path) {
+        if (*filter->path == '/' && pathlen == 1) {
+            return 1;
+        }
+        else {
+            return 0;
+        }
+    }
+
     /* Url has met all of the filter conditions so far, determine
      * if the paths match.
      */
+
     return !strncmp(filter->path, url->path, pathlen);
 }
 

--

[patch 08/16] dont delete empty cache directories

Posted by Davi Arnaut <da...@haxent.com.br>.
Don't delete empty cache directories, it is too expensive and unnecessary.
Later they can be removed with htcacheclean.

Index: modules/cache/mod_disk_cache.c
===================================================================
--- modules/cache/mod_disk_cache.c.orig
+++ modules/cache/mod_disk_cache.c
@@ -579,45 +579,6 @@
         }
     }
 
-    /* now delete directories as far as possible up to our cache root */
-    if (dobj->root) {
-        const char *str_to_copy;
-
-        str_to_copy = dobj->hdrsfile ? dobj->hdrsfile : dobj->datafile;
-        if (str_to_copy) {
-            char *dir, *slash, *q;
-
-            dir = apr_pstrdup(p, str_to_copy);
-
-            /* remove filename */
-            slash = strrchr(dir, '/');
-            *slash = '\0';
-
-            /*
-             * now walk our way back to the cache root, delete everything
-             * in the way as far as possible
-             *
-             * Note: due to the way we constructed the file names in
-             * header_file and data_file, we are guaranteed that the
-             * cache_root is suffixed by at least one '/' which will be
-             * turned into a terminating null by this loop.  Therefore,
-             * we won't either delete or go above our cache root.
-             */
-            for (q = dir + dobj->root_len; *q ; ) {
-                 ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, NULL,
-                              "disk_cache: Deleting directory %s from cache",
-                              dir);
-
-                 rc = apr_dir_remove(dir, p);
-                 if (rc != APR_SUCCESS && !APR_STATUS_IS_ENOENT(rc)) {
-                    break;
-                 }
-                 slash = strrchr(q, '/');
-                 *slash = '\0';
-            }
-        }
-    }
-
     return OK;
 }
 

--

[patch 12/16] use APR pools in mod_mem_cache

Posted by Davi Arnaut <da...@haxent.com.br>.
Convert mod_mem_cache to use APR memory pool functions by creating
a root pool for object persistence across requests. This also
eliminates the need for custom serialization code.

Index: modules/cache/mod_mem_cache.c
===================================================================
--- modules/cache/mod_mem_cache.c.orig
+++ modules/cache/mod_mem_cache.c
@@ -59,19 +59,12 @@
     CACHE_TYPE_MMAP
 } cache_type_e;
 
-typedef struct
-{
-    char *hdr;
-    char *val;
-} cache_header_tbl_t;
-
 typedef struct mem_cache_object
 {
+    apr_pool_t *pool;
     cache_type_e type;
-    apr_ssize_t num_header_out;
-    apr_ssize_t num_req_hdrs;
-    cache_header_tbl_t *header_out;
-    cache_header_tbl_t *req_hdrs;       /* for Vary negotiation */
+    apr_table_t *header_out;
+    apr_table_t *req_hdrs; /* for Vary negotiation */
     apr_size_t m_len;
     void *m;
     apr_os_file_t fd;
@@ -212,19 +205,6 @@
 {
     mem_cache_object_t *mobj = obj->vobj;
 
-    /* TODO:
-     * We desperately need a more efficient way of allocating objects. We're
-     * making way too many malloc calls to create a fully populated
-     * cache object...
-     */
-
-    /* Cleanup the cache_object_t */
-    if (obj->key) {
-        free((void *) obj->key);
-    }
-
-    free(obj);
-
     /* Cleanup the mem_cache_object_t */
     if (mobj) {
         if (mobj->m) {
@@ -237,18 +217,9 @@
             close(mobj->fd);
 #endif
         }
-        if (mobj->header_out) {
-            if (mobj->header_out[0].hdr)
-                free(mobj->header_out[0].hdr);
-            free(mobj->header_out);
-        }
-        if (mobj->req_hdrs) {
-            if (mobj->req_hdrs[0].hdr)
-                free(mobj->req_hdrs[0].hdr);
-            free(mobj->req_hdrs);
-        }
-        free(mobj);
     }
+
+    apr_pool_destroy(mobj->pool);
 }
 
 static apr_status_t decrement_refcount(void *arg)
@@ -339,9 +310,10 @@
 static int create_entity(cache_handle_t * h, cache_type_e type_e,
                          request_rec *r, const char *key, apr_off_t len)
 {
+    apr_status_t rv;
+    apr_pool_t *pool;
     cache_object_t *obj, *tmp_obj;
     mem_cache_object_t *mobj;
-    apr_size_t key_len;
 
     if (len == -1) {
         /* Caching a streaming response. Assume the response is
@@ -375,25 +347,21 @@
         }
     }
 
-    /* Allocate and initialize cache_object_t */
-    obj = calloc(1, sizeof(*obj));
-    if (!obj) {
-        return DECLINED;
-    }
-    key_len = strlen(key) + 1;
-    obj->key = malloc(key_len);
-    if (!obj->key) {
-        cleanup_cache_object(obj);
+    rv = apr_pool_create(&pool, NULL);
+
+    if (rv != APR_SUCCESS) {
+        ap_log_error(APLOG_MARK, APLOG_WARNING, rv, r->server,
+                     "mem_cache: Failed to create memory pool.");
         return DECLINED;
     }
-    memcpy((void *) obj->key, key, key_len);
+
+    /* Allocate and initialize cache_object_t */
+    obj = apr_pcalloc(pool, sizeof(*obj));
+    obj->key = apr_pstrdup(pool, key);
 
     /* Allocate and init mem_cache_object_t */
-    mobj = calloc(1, sizeof(*mobj));
-    if (!mobj) {
-        cleanup_cache_object(obj);
-        return DECLINED;
-    }
+    mobj = apr_pcalloc(pool, sizeof(*mobj));
+    mobj->pool = pool;
 
     /* Finish initing the cache object */
     apr_atomic_set32(&obj->refcount, 1);
@@ -545,65 +513,6 @@
     return OK;
 }
 
-static apr_status_t serialize_table(cache_header_tbl_t ** obj,
-                                    apr_ssize_t * nelts, apr_table_t * table)
-{
-    const apr_array_header_t *elts_arr = apr_table_elts(table);
-    apr_table_entry_t *elts = (apr_table_entry_t *) elts_arr->elts;
-    apr_ssize_t i;
-    apr_size_t len = 0;
-    apr_size_t idx = 0;
-    char *buf;
-
-    *nelts = elts_arr->nelts;
-    if (*nelts == 0) {
-        *obj = NULL;
-        return APR_SUCCESS;
-    }
-    *obj = malloc(sizeof(cache_header_tbl_t) * elts_arr->nelts);
-    if (NULL == *obj) {
-        return APR_ENOMEM;
-    }
-    for (i = 0; i < elts_arr->nelts; ++i) {
-        len += strlen(elts[i].key);
-        len += strlen(elts[i].val);
-        len += 2;               /* Extra space for NULL string terminator for key and val */
-    }
-
-    /* Transfer the headers into a contiguous memory block */
-    buf = malloc(len);
-    if (!buf) {
-        free(*obj);
-        *obj = NULL;
-        return APR_ENOMEM;
-    }
-
-    for (i = 0; i < *nelts; ++i) {
-        (*obj)[i].hdr = &buf[idx];
-        len = strlen(elts[i].key) + 1;  /* Include NULL terminator */
-        memcpy(&buf[idx], elts[i].key, len);
-        idx += len;
-
-        (*obj)[i].val = &buf[idx];
-        len = strlen(elts[i].val) + 1;
-        memcpy(&buf[idx], elts[i].val, len);
-        idx += len;
-    }
-    return APR_SUCCESS;
-}
-
-static int unserialize_table(cache_header_tbl_t * ctbl, int num_headers,
-                             apr_table_t * t)
-{
-    int i;
-
-    for (i = 0; i < num_headers; ++i) {
-        apr_table_addn(t, ctbl[i].hdr, ctbl[i].val);
-    }
-
-    return APR_SUCCESS;
-}
-
 /* Define request processing hook handlers */
 static int remove_url(cache_handle_t * h, apr_pool_t * p)
 {
@@ -633,17 +542,12 @@
 
 static apr_status_t recall_headers(cache_handle_t *h, request_rec *r)
 {
-    int rc;
     mem_cache_object_t *mobj = (mem_cache_object_t*) h->cache_obj->vobj;
 
-    h->req_hdrs = apr_table_make(r->pool, mobj->num_req_hdrs);
-    h->resp_hdrs = apr_table_make(r->pool, mobj->num_header_out);
-
-    rc = unserialize_table(mobj->req_hdrs, mobj->num_req_hdrs, h->req_hdrs);
-    rc = unserialize_table(mobj->header_out, mobj->num_header_out,
-                           h->resp_hdrs);
+    h->req_hdrs = apr_table_copy(r->pool, mobj->req_hdrs);
+    h->resp_hdrs = apr_table_copy(r->pool, mobj->header_out);
 
-    return rc;
+    return OK;
 }
 
 static apr_status_t recall_body(cache_handle_t * h, apr_pool_t * p,
@@ -677,7 +581,6 @@
 {
     cache_object_t *obj = h->cache_obj;
     mem_cache_object_t *mobj = (mem_cache_object_t *) obj->vobj;
-    int rc;
     apr_table_t *headers_out;
 
     /*
@@ -687,10 +590,7 @@
      * - The original response headers (for returning with a cached response)
      * - The body of the message
      */
-    rc = serialize_table(&mobj->req_hdrs, &mobj->num_req_hdrs, r->headers_in);
-    if (rc != APR_SUCCESS) {
-        return rc;
-    }
+     mobj->req_hdrs = apr_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,
@@ -704,12 +604,7 @@
     }
 
     headers_out = apr_table_overlay(r->pool, headers_out, r->err_headers_out);
-
-    rc = serialize_table(&mobj->header_out, &mobj->num_header_out,
-                         headers_out);
-    if (rc != APR_SUCCESS) {
-        return rc;
-    }
+    mobj->header_out = apr_table_copy(mobj->pool, headers_out);
 
     /* Init the info struct */
     obj->info.status = info->status;
@@ -870,16 +765,12 @@
                 /* Caching a streamed response. Reallocate a buffer of the
                  * correct size and copy the streamed response into that
                  * buffer */
-                char *buf = malloc(obj->count);
+                mobj->m = realloc(mobj->m, obj->count);
 
-                if (!buf) {
+                if (!mobj->m) {
                     return APR_ENOMEM;
                 }
 
-                memcpy(buf, mobj->m, obj->count);
-                free(mobj->m);
-                mobj->m = buf;
-
                 update_object_on_cache(obj, mobj);
             }
             /* Open for business */

--

[patch 03/16] shrink cache_save_filter

Posted by Davi Arnaut <da...@haxent.com.br>.
Move parts of cache_save_filter() to a smaller check_cacheable_request()
function that is easier to read and understand. Also, a useless assignment
is removed.


Index: modules/cache/mod_cache.c
===================================================================
--- modules/cache/mod_cache.c.orig
+++ modules/cache/mod_cache.c
@@ -313,65 +313,11 @@
  *   Finally, pass the data to the next filter (the network or whatever)
  */
 
-static int cache_save_filter(ap_filter_t *f, apr_bucket_brigade *in)
+static const char* check_cacheable_request(request_rec *r,
+    cache_request_rec *cache, cache_server_conf *conf)
 {
-    int rv = !OK;
-    int date_in_errhdr = 0;
-    request_rec *r = f->r;
-    cache_request_rec *cache;
-    cache_server_conf *conf;
-    const char *cc_out, *cl;
-    const char *exps, *lastmods, *dates, *etag;
-    apr_time_t exp, date, lastmod, now;
-    apr_off_t size;
-    cache_info *info = NULL;
-    char *reason;
-    apr_pool_t *p;
-
-    conf = (cache_server_conf *) ap_get_module_config(r->server->module_config,
-                                                      &cache_module);
-
-    /* Setup cache_request_rec */
-    cache = (cache_request_rec *) ap_get_module_config(r->request_config,
-                                                       &cache_module);
-    if (!cache) {
-        /* user likely configured CACHE_SAVE manually; they should really use
-         * mod_cache configuration to do that
-         */
-        cache = apr_pcalloc(r->pool, sizeof(cache_request_rec));
-        ap_set_module_config(r->request_config, &cache_module, cache);
-    }
-
-    reason = NULL;
-    p = r->pool;
-    /*
-     * Pass Data to Cache
-     * ------------------
-     * This section passes the brigades into the cache modules, but only
-     * if the setup section (see below) is complete.
-     */
-    if (cache->block_response) {
-        /* We've already sent down the response and EOS.  So, ignore
-         * whatever comes now.
-         */
-        return APR_SUCCESS;
-    }
-
-    /* have we already run the cachability check and set up the
-     * cached file handle?
-     */
-    if (cache->in_checked) {
-        /* pass the brigades into the cache, then pass them
-         * up the filter stack
-         */
-        rv = cache->provider->store_body(cache->handle, r, in);
-        if (rv != APR_SUCCESS) {
-            ap_log_error(APLOG_MARK, APLOG_DEBUG, rv, r->server,
-                         "cache: Cache provider's store_body failed!");
-            ap_remove_output_filter(f);
-        }
-        return ap_pass_brigade(f->next, in);
-    }
+    apr_time_t exp, lastmod;
+    const char *exps, *lastmods, *etag, *cc_out, *reason = NULL;
 
     /*
      * Setup Data in Cache
@@ -439,11 +385,11 @@
          * We include 304 Not Modified here too as this is the origin server
          * telling us to serve the cached copy.
          */
-        reason = apr_psprintf(p, "Response status %d", r->status);
+        reason = apr_psprintf(r->pool, "Response status %d", r->status);
     }
     else if (exps != NULL && exp == APR_DATE_BAD) {
         /* if a broken Expires header is present, don't cache it */
-        reason = apr_pstrcat(p, "Broken expires header: ", exps, NULL);
+        reason = apr_pstrcat(r->pool, "Broken expires header: ", exps, NULL);
     }
     else if (exp != APR_DATE_BAD && exp < r->request_time)
     {
@@ -522,20 +468,16 @@
         reason = "r->no_cache present";
     }
 
-    if (reason) {
-        ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, r->server,
-                     "cache: %s not cached. Reason: %s", r->unparsed_uri,
-                     reason);
+    cache->exp = exp;
+    cache->lastmod = lastmod;
 
-        /* remove this filter from the chain */
-        ap_remove_output_filter(f);
-
-        /* ship the data up the stack */
-        return ap_pass_brigade(f->next, in);
-    }
+    return reason;
+}
 
-    /* Make it so that we don't execute this path again. */
-    cache->in_checked = 1;
+static apr_off_t request_cl(request_rec *r, apr_bucket_brigade *in)
+{
+    const char *cl;
+    apr_off_t size;
 
     /* Set the content length if known.
      */
@@ -580,48 +522,18 @@
         }
     }
 
-    /* It's safe to cache the response.
-     *
-     * There are two possiblities at this point:
-     * - cache->handle == NULL. In this case there is no previously
-     * cached entity anywhere on the system. We must create a brand
-     * new entity and store the response in it.
-     * - cache->stale_handle != NULL. In this case there is a stale
-     * entity in the system which needs to be replaced by new
-     * content (unless the result was 304 Not Modified, which means
-     * the cached entity is actually fresh, and we should update
-     * the headers).
-     */
-
-    /* Did we have a stale cache entry that really is stale? */
-    if (cache->stale_handle) {
-        if (r->status == HTTP_NOT_MODIFIED) {
-            /* Oh, hey.  It isn't that stale!  Yay! */
-            cache->handle = cache->stale_handle;
-            info = &cache->handle->cache_obj->info;
-            rv = OK;
-        }
-        else {
-            /* Oh, well.  Toss it. */
-            cache->provider->remove_entity(cache->stale_handle);
-            /* Treat the request as if it wasn't conditional. */
-            cache->stale_handle = NULL;
-        }
-    }
+    return size;
+}
 
-    /* no cache handle, create a new entity */
-    if (!cache->handle) {
-        rv = cache_create_entity(r, size);
-        info = apr_pcalloc(r->pool, sizeof(cache_info));
-        /* We only set info->status upon the initial creation. */
-        info->status = r->status;
-    }
+static void prepare_cacheable_request(request_rec *r, cache_request_rec *cache,
+    cache_info *info, cache_server_conf *conf)
+{
+    const char *dates;
+    int date_in_errhdr = 0;
+    apr_time_t exp, lastmod, now, date;
 
-    if (rv != OK) {
-        /* Caching layer declined the opportunity to cache the response */
-        ap_remove_output_filter(f);
-        return ap_pass_brigade(f->next, in);
-    }
+    exp = cache->exp;
+    lastmod = cache->lastmod;
 
     ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, r->server,
                  "cache: Caching url: %s", r->unparsed_uri);
@@ -690,7 +602,6 @@
     if (lastmod != APR_DATE_BAD && lastmod > date) {
         /* if it's in the future, then replace by date */
         lastmod = date;
-        lastmods = dates;
         ap_log_error(APLOG_MARK, APLOG_DEBUG, 0,
                      r->server,
                      "cache: Last modified is in the future, "
@@ -729,7 +640,129 @@
             apr_table_set(r->headers_out, "Expires", expire_hdr);
         }
     }
+
     info->expire = exp;
+}
+
+static int cache_save_filter(ap_filter_t *f, apr_bucket_brigade *in)
+{
+    int rv = !OK;
+    request_rec *r = f->r;
+    cache_request_rec *cache;
+    cache_server_conf *conf;
+    apr_off_t size;
+    cache_info *info = NULL;
+    const char *reason;
+    apr_pool_t *p;
+
+    conf = (cache_server_conf *) ap_get_module_config(r->server->module_config,
+                                                      &cache_module);
+
+    /* Setup cache_request_rec */
+    cache = (cache_request_rec *) ap_get_module_config(r->request_config,
+                                                       &cache_module);
+    if (!cache) {
+        /* user likely configured CACHE_SAVE manually; they should really use
+         * mod_cache configuration to do that
+         */
+        cache = apr_pcalloc(r->pool, sizeof(cache_request_rec));
+        ap_set_module_config(r->request_config, &cache_module, cache);
+    }
+
+    p = r->pool;
+
+    /*
+     * Pass Data to Cache
+     * ------------------
+     * This section passes the brigades into the cache modules, but only
+     * if the setup section (see below) is complete.
+     */
+    if (cache->block_response) {
+        /* We've already sent down the response and EOS.  So, ignore
+         * whatever comes now.
+         */
+        return APR_SUCCESS;
+    }
+
+    /* have we already run the cachability check and set up the
+     * cached file handle?
+     */
+    if (cache->in_checked) {
+        /* pass the brigades into the cache, then pass them
+         * up the filter stack
+         */
+        rv = cache->provider->store_body(cache->handle, r, in);
+        if (rv != APR_SUCCESS) {
+            ap_log_error(APLOG_MARK, APLOG_DEBUG, rv, r->server,
+                         "cache: Cache provider's store_body failed!");
+            ap_remove_output_filter(f);
+        }
+        return ap_pass_brigade(f->next, in);
+    }
+
+    reason = check_cacheable_request(r, cache, conf);
+
+    if (reason) {
+        ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, r->server,
+                     "cache: %s not cached. Reason: %s", r->unparsed_uri,
+                     reason);
+
+        /* remove this filter from the chain */
+        ap_remove_output_filter(f);
+
+        /* ship the data up the stack */
+        return ap_pass_brigade(f->next, in);
+    }
+
+    /* Make it so that we don't execute this path again. */
+    cache->in_checked = 1;
+
+    size = request_cl(r, in);
+
+    /* It's safe to cache the response.
+     *
+     * There are two possiblities at this point:
+     * - cache->handle == NULL. In this case there is no previously
+     * cached entity anywhere on the system. We must create a brand
+     * new entity and store the response in it.
+     * - cache->stale_handle != NULL. In this case there is a stale
+     * entity in the system which needs to be replaced by new
+     * content (unless the result was 304 Not Modified, which means
+     * the cached entity is actually fresh, and we should update
+     * the headers).
+     */
+
+    /* Did we have a stale cache entry that really is stale? */
+    if (cache->stale_handle) {
+        if (r->status == HTTP_NOT_MODIFIED) {
+            /* Oh, hey.  It isn't that stale!  Yay! */
+            cache->handle = cache->stale_handle;
+            info = &cache->handle->cache_obj->info;
+            rv = OK;
+        }
+        else {
+            /* Oh, well.  Toss it. */
+            cache->provider->remove_entity(cache->stale_handle);
+            /* Treat the request as if it wasn't conditional. */
+            cache->stale_handle = NULL;
+        }
+    }
+
+    /* no cache handle, create a new entity */
+    if (!cache->handle) {
+        rv = cache_create_entity(r, size);
+        info = apr_pcalloc(r->pool, sizeof(cache_info));
+        /* We only set info->status upon the initial creation. */
+        info->status = r->status;
+    }
+
+    if (rv != OK) {
+        /* Caching layer declined the opportunity to cache the response */
+        ap_remove_output_filter(f);
+        return ap_pass_brigade(f->next, in);
+    }
+
+    prepare_cacheable_request(r, cache, info, conf);
 
     /* We found a stale entry which wasn't really stale. */
     if (cache->stale_handle) {

--