You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by "Akins, Brian" <Br...@turner.com> on 2005/08/17 21:01:03 UTC
[PATCH] mod_cache. Allow override of some vary headers
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.
Re: [PATCH] mod_cache. Allow override of some vary headers
Posted by Brian Akins <br...@turner.com>.
Justin Erenkrantz wrote:
> The concept is fine. (And as Joshua pointed out 'CacheVaryOverride'
> would be a better name.)
I'm not attatched to the name. So that sounds good to me.
> A few issues with the implementation (modulo style nits)...
This was all done in a few hours today, including testing. Some stuff,
especially in cache_util, looks really ugly :)
>> + if((header = apr_table_get(r->subprocess_env, o)) == NULL) {
>> + header = "-";
>> + }
>> + }
>
>
> I don't understand what the assignment to "-" is doing here.
Just a default value when the environment is not set. Need to have a
default to handle cases when header is not set. I thought about having
this default be configurable per override. In short, "-" was just as
good a default as any.
>> + 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.
Probably not necessary to log at all. I left this in here until me/we
feel confident with it.
>> -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
Sure. I just noticed everytime regen_key was called, it was called with
r->headers_in.
--
Brian Akins
Lead Systems Engineer
CNN Internet Technologies
Re: [PATCH] mod_cache. Allow override of some vary headers
Posted by Brian Akins <ba...@web.turner.com>.
Another one. Can we get a vote?
Sorry to be a pest, just don't want these to be forgotten.
Brian Akins wrote:
> Here's a new patch that changes the option name to CacheVaryOverride and
> does some of the stuff Justin recommened.
>
>
>
> ------------------------------------------------------------------------
>
> diff -ru httpd-trunk.orig/modules/cache/cache_storage.c httpd-trunk.new/modules/cache/cache_storage.c
> --- httpd-trunk.orig/modules/cache/cache_storage.c 2005-07-13 15:23:03.892378000 -0400
> +++ httpd-trunk.new/modules/cache/cache_storage.c 2005-08-18 08:13:30.098771299 -0400
> @@ -230,7 +230,7 @@
> * 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);
> + h1 = ap_cache_override_header(r, r->headers_in, name);
> h2 = apr_table_get(h->req_hdrs, name);
> if (h1 == h2) {
> /* both headers NULL, so a match - do nothing */
> diff -ru httpd-trunk.orig/modules/cache/cache_util.c httpd-trunk.new/modules/cache/cache_util.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-18 08:15:05.349865494 -0400
> @@ -534,3 +534,49 @@
> }
> 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 = "-";
> + }
> + }
> +
> + if(!header) {
> + header = apr_table_get(t, key);
> + }
> +
> + return header;
> +}
> +
> +/* replace headers based on environment overrides
> + * modifies t in place
> + */
> +CACHE_DECLARE(apr_status_t )ap_cache_override_hdrs(request_rec *r,
> + apr_table_t *t)
> +{
> + int i;
> + apr_table_entry_t *elts;
> + cache_server_conf *conf = ap_get_module_config(r->server->module_config,
> + &cache_module);
> +
> + /* replace headers with environment overrides*/
> + elts = (apr_table_entry_t *) apr_table_elts(conf->override_headers)->elts;
> +
> + for (i = 0; i < apr_table_elts(conf->override_headers)->nelts; ++i) {
> + const char *val = NULL;
> +
> + val = ap_cache_override_header(r, t, elts[i].key);
> + apr_table_set(t, elts[i].key, val);
> +
> + }
> + return APR_SUCCESS;
> +}
> +
> diff -ru httpd-trunk.orig/modules/cache/mod_cache.c httpd-trunk.new/modules/cache/mod_cache.c
> --- httpd-trunk.orig/modules/cache/mod_cache.c 2005-08-09 11:51:09.471251000 -0400
> +++ httpd-trunk.new/modules/cache/mod_cache.c 2005-08-18 08:14:39.755139238 -0400
> @@ -745,6 +745,7 @@
> /* array of headers that should not be stored in cache */
> ps->ignore_headers = apr_array_make(p, 10, sizeof(char *));
> ps->ignore_headers_set = CACHE_IGNORE_HEADERS_UNSET;
> + ps->override_headers = apr_table_make(p, 10);
> return ps;
> }
>
> @@ -790,6 +791,9 @@
> (overrides->ignore_headers_set == CACHE_IGNORE_HEADERS_UNSET)
> ? base->ignore_headers
> : overrides->ignore_headers;
> + ps->override_headers = apr_table_copy(p, base->override_headers);
> + apr_table_overlap(ps->override_headers, overrides->override_headers, 0);
> +
> return ps;
> }
> static const char *set_cache_ignore_no_last_mod(cmd_parms *parms, void *dummy,
> @@ -873,6 +877,19 @@
> return NULL;
> }
>
> +static const char *add_override_header(cmd_parms *parms, void *dummy,
> + const char *header, const char* env)
> +{
> + cache_server_conf *conf;
> + conf =
> + (cache_server_conf *)ap_get_module_config(parms->server->module_config,
> + &cache_module);
> +
> + apr_table_set(conf->override_headers, header, env);
> +
> + return NULL;
> +}
> +
> static const char *add_cache_enable(cmd_parms *parms, void *dummy,
> const char *type,
> const char *url)
> @@ -1002,6 +1019,9 @@
> AP_INIT_ITERATE("CacheIgnoreHeaders", add_ignore_header, NULL, RSRC_CONF,
> "A space separated list of headers that should not be "
> "stored by the cache"),
> + AP_INIT_TAKE2("CacheVaryOverride", add_override_header, NULL, RSRC_CONF,
> + "A header that should be replaced by the value of"
> + " the given environment variable"),
> AP_INIT_TAKE1("CacheLastModifiedFactor", set_cache_factor, NULL, RSRC_CONF,
> "The factor used to estimate Expires date from "
> "LastModified date"),
> diff -ru httpd-trunk.orig/modules/cache/mod_cache.h httpd-trunk.new/modules/cache/mod_cache.h
> --- httpd-trunk.orig/modules/cache/mod_cache.h 2005-07-13 15:23:03.882379000 -0400
> +++ httpd-trunk.new/modules/cache/mod_cache.h 2005-08-18 08:13:30.101770798 -0400
> @@ -141,6 +141,8 @@
> int store_nostore_set;
> /** store the headers that should not be stored in the cache */
> apr_array_header_t *ignore_headers;
> + /** environment header overrides **/
> + apr_table_t *override_headers;
> /* flag if CacheIgnoreHeader has been set */
> #define CACHE_IGNORE_HEADERS_SET 1
> #define CACHE_IGNORE_HEADERS_UNSET 0
> @@ -256,6 +258,11 @@
> CACHE_DECLARE(char *) ap_cache_generate_name(apr_pool_t *p, int dirlevels,
> int dirlength,
> const char *name);
> +CACHE_DECLARE(const char* )ap_cache_override_header(request_rec *r,
> + apr_table_t *t,
> + const char* key);
> +CACHE_DECLARE(apr_status_t )ap_cache_override_hdrs(request_rec *r,
> + apr_table_t *t);
> CACHE_DECLARE(cache_provider_list *)ap_cache_get_providers(request_rec *r, cache_server_conf *conf, const char *url);
> CACHE_DECLARE(int) ap_cache_liststr(apr_pool_t *p, const char *list,
> const char *key, char **val);
> 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-18 08:16:00.780610483 -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_table_t *headers,
> apr_array_header_t *varray, const char *oldkey)
> {
> struct iovec *iov;
> @@ -280,9 +280,8 @@
> int nvec;
> const char *header;
> const char **elts;
> -
> nvec = (varray->nelts * 2) + 1;
> - iov = apr_palloc(p, sizeof(struct iovec) * nvec);
> + iov = apr_palloc(r->pool, sizeof(struct iovec) * nvec);
> elts = (const char **) varray->elts;
>
> /* TODO:
> @@ -308,9 +307,9 @@
> * tokens (including the 100-continue token), and is case-sensitive for
> * quoted-string expectation-extensions.
> */
> -
> for(i=0, k=0; i < varray->nelts; i++) {
> - header = apr_table_get(headers, elts[i]);
> + header = ap_cache_override_header(r, headers, elts[i]);
> +
> if (!header) {
> header = "";
> }
> @@ -325,7 +324,7 @@
> iov[k].iov_len = strlen(oldkey);
> k++;
>
> - return apr_pstrcatv(p, iov, k, NULL);
> + return apr_pstrcatv(r->pool, iov, k, NULL);
> }
>
> static int array_alphasort(const void *fn1, const void *fn2)
> @@ -445,7 +444,7 @@
> }
> apr_file_close(dobj->hfd);
>
> - nkey = regen_key(r->pool, r->headers_in, varray, key);
> + nkey = regen_key(r, r->headers_in, varray, key);
>
> dobj->hashfile = NULL;
> dobj->prefix = dobj->hdrsfile;
> @@ -804,7 +803,7 @@
> }
>
> dobj->tempfile = apr_pstrcat(r->pool, conf->cache_root, AP_TEMPFILE, NULL);
> - tmp = regen_key(r->pool, r->headers_in, varray, dobj->name);
> + tmp = regen_key(r, r->headers_in, varray, dobj->name);
> dobj->prefix = dobj->hdrsfile;
> dobj->hashfile = NULL;
> dobj->datafile = data_file(r->pool, conf, dobj, tmp);
> @@ -870,6 +869,8 @@
>
> headers_in = ap_cache_cacheable_hdrs_out(r->pool, r->headers_in,
> r->server);
> + ap_cache_override_hdrs(r, headers_in);
> +
> rv = store_table(dobj->hfd, headers_in);
> if (rv != APR_SUCCESS) {
> return rv;
--
Brian Akins
Lead Systems Engineer
CNN Internet Technologies
Re: [PATCH] mod_cache. Allow override of some vary headers
Posted by Brian Akins <br...@turner.com>.
Here's a new patch that changes the option name to CacheVaryOverride and
does some of the stuff Justin recommened.
--
Brian Akins
Lead Systems Engineer
CNN Internet Technologies
Re: [PATCH] mod_cache. Allow override of some vary headers
Posted by Justin Erenkrantz <ju...@erenkrantz.com>.
--On August 17, 2005 3:01:03 PM -0400 "Akins, Brian"
<Br...@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
Re: [PATCH] mod_cache. Allow override of some vary headers
Posted by Joshua Slive <jo...@slive.ca>.
Akins, Brian wrote:
> This patch allows one to override the values of some headers so that they
> "vary" to the same value.
> CacheOverrideHeader Accept-Encoding gzip
> CacheOverrideHeader User-Agent gzip
Very useful looking. I suggest "CacheVaryOverride" as a much more clear
and precise name. The directive doesn't really override a header, but
just overrides the way Vary handling works for a specific header.
Joshua.