httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Justin Erenkrantz <jus...@erenkrantz.com>
Subject Re: [PATCH] mod_cache. Allow override of some vary headers
Date Wed, 17 Aug 2005 19:56:58 GMT
--On August 17, 2005 3:01:03 PM -0400 "Akins, Brian" 
<Brian.Akins@turner.com> wrote:

> This patch allows one to override the values of some headers so that they
> "vary" to the same value.
>
> Config Example:
>
># all lines that have gzip set one variable
> SetEnvIf Accept-Encoding gzip gzip=1
>
># browsers that have problems with gzip
> BrowserMatch "MSIE [1-3]" gzip=0
> BrowserMatch "MSIE [1-5].*Mac" gzip=0
> BrowserMatch "^Mozilla/4\.0[678]" gzip=0
>
> ...
>
>
> CacheOverrideHeader Accept-Encoding gzip
> CacheOverrideHeader User-Agent gzip
>
>
> This would allow all browsers that send "Accept-Encoding: gzip" and do not
> match the BrowserMatches to be mapped to the same cache object.  All the
> other variants would point to another object.  This would be very useful
> in reverse proxy caches.
>
> Only patched mod_disk_cache, but mod_mem_cache should be trivial.

The concept is fine.  (And as Joshua pointed out 'CacheVaryOverride' would 
be a better name.)

A few issues with the implementation (modulo style nits)...

> diff -ru httpd-trunk.orig/modules/cache/cache_storage.c
> httpd-trunk.new/modules/cache/cache_storage.c
> --- httpd-trunk.orig/modules/cache/cache_util.c	2005-07-13
> 15:23:03.869381000 -0400
> +++ httpd-trunk.new/modules/cache/cache_util.c	2005-08-17
> 14:53:29.890200893 -0400
> @@ -534,3 +534,52 @@
>      }
>      return headers_out;
>  }
> +
> +CACHE_DECLARE(const char* )ap_cache_override_header(request_rec *r,
> +                                                    apr_table_t *t,
> +                                                    const char* key) {
> +    const char *o;
> +    cache_server_conf *conf =
> ap_get_module_config(r->server->module_config,
> +                                                   &cache_module);
> +    const char *header = NULL;
> +
> +    if((o = apr_table_get(conf->override_headers, key)) != NULL) {
> +        if((header = apr_table_get(r->subprocess_env, o)) == NULL) {
> +            header = "-";
> +        }
> +    }

I don't understand what the assignment to "-" is doing here.

> +
> +    if(!header) {
> +        header = apr_table_get(t, key);
> +    }
> +
> +    ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, r->server,
> +                 "ap_cache_override_header: %s => %s", key, header);

I don't think its necessary to log in the case where it is a no-op - so I'd 
stick this in an else branch of the conditional above.

> +
> +    return header;
> +}
> +
...snip...
> diff -ru httpd-trunk.orig/modules/cache/mod_disk_cache.c
> httpd-trunk.new/modules/cache/mod_disk_cache.c
> --- httpd-trunk.orig/modules/cache/mod_disk_cache.c	2005-08-09
> 11:51:09.473251000 -0400
> +++ httpd-trunk.new/modules/cache/mod_disk_cache.c	2005-08-17
> 14:53:29.894200224 -0400
> @@ -272,7 +272,7 @@
>      return APR_SUCCESS;
>  }
>
> -static const char* regen_key(apr_pool_t *p, apr_table_t *headers,
> +static const char* regen_key(request_rec *r,
>                               apr_array_header_t *varray, const char
> *oldkey)

I would prefer that we keep the headers argument explicit rather than 
hardcoding that it is r->headers_in.  -- justin

Mime
View raw message