httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Ruediger Pluem <rpl...@apache.org>
Subject Re: Patch: Centralize some Cache header handling
Date Fri, 08 Feb 2008 23:17:54 GMT


On 02/08/2008 11:23 PM, Dirk-Willem van Gulik wrote:
> On Feb 8, 2008, at 6:26 PM, Dirk-Willem van Gulik wrote:
> 
>> Right now we have a nice utility function:
>>
>>     ap_cache_cacheable_hdrs_out()
>>         Make a copy of the headers, and remove from
>>         the copy any hop-by-hop headers, as defined in Section
>>         13.5.1 of RFC 2616
>>
>> (in cache_util.c, part of mod_cache.h public headers) which we can
>> apply to both the in and outbound headers.
>>
>> Now as far as I can see -every- caching module will have to take care
>> of the error headers and the content -- and filtering both in-and-out
>> that way.
>>
>> So how about changing this to:
>>
>> -    ap_cache_cacheable_hdrs_out(r);
>>         calls ap_cache_cacheable_hdrs
>>     AND
>>         Set Content-Type if not set
>>     AND
>>         overlays r->err_headers_out
>>
>> -    ap_cache_cacheable_hdrs_in(r);
>>         calles ap_cache_cacheable_hdrs
>>
>> -    ap_cache_cacheable_hdrs(pool, headers, server) (private)
>>         delete/cleanse as per RFC 2616
>>     and as per CacheIgnoreHeaders
>>
>> Or is there a fundamental reason as to why some caches will not want
>> to do the 'usual'* ?
>>
>> Thoughts ?
> 
> Love to hear caching experts their thoughts -- esp. to hear confirmed
> that it is correct that mod_cache.c    needs indeed just to touch
> headers_out (and not in) at that point and has no need to touch up
> content-type.
> 


Looks good except for one small nitpick below:


> 
> 
> Index: mod_cache.h
> ===================================================================
> --- mod_cache.h    (revision 618646)
> +++ mod_cache.h    (working copy)
> @@ -289,12 +289,23 @@
>  CACHE_DECLARE(const char *)ap_cache_tokstr(apr_pool_t *p, const char
> *list, const char **str);
> 
>  /* Create a new table consisting of those elements from a request_rec's
> - * headers_out that are allowed to be stored in a cache
> + * headers that are allowed to be stored in a cache
>   */
> -CACHE_DECLARE(apr_table_t *)ap_cache_cacheable_hdrs_out(apr_pool_t *pool,
> +CACHE_DECLARE(apr_table_t *)ap_cache_cacheable_hdrs(apr_pool_t *pool,
>                                                          apr_table_t *t,
>                                                          server_rec *s);
> 
> +/* Create a new table consisting of those elements from an input
> + * headers table that are allowed to be stored in a cache.
> + */
> +CACHE_DECLARE(apr_table_t *)ap_cache_cacheable_hdrs_in(request_rec *);
> +
> +/* Create a new table consisting of those elements from an output
> + * headers table that are allowed to be stored in a cache;
> + * ensure there is a content type and capture any errors.
> + */
> +CACHE_DECLARE(apr_table_t *)ap_cache_cacheable_hdrs_out(request_rec *);
> +
>  /**
>   * cache_storage.c
>   */
> Index: cache_util.c
> ===================================================================
> --- cache_util.c    (revision 618646)
> +++ cache_util.c    (working copy)
> @@ -571,10 +573,7 @@
>      return apr_pstrdup(p, hashfile);
>  }
> 
> -/* Create a new table consisting of those elements from an input
> - * headers table that are allowed to be stored in a cache.
> - */

Nitpicking: Why not using the comment from mod_cache.h here?

> -CACHE_DECLARE(apr_table_t *)ap_cache_cacheable_hdrs_out(apr_pool_t *pool,
> +CACHE_DECLARE(apr_table_t *)ap_cache_cacheable_hdrs(apr_pool_t *pool,
>                                                          apr_table_t *t,
>                                                          server_rec *s)
>  {
> @@ -608,3 +607,34 @@
>      }
>      return headers_out;
>  }
> +
> +/* Create a new table consisting of those elements from an input
> + * headers table that are allowed to be stored in a cache.
> + */
> +CACHE_DECLARE(apr_table_t *)ap_cache_cacheable_hdrs_in(request_rec * r)
> +{
> +    return ap_cache_cacheable_hdrs(r->pool, r->headers_in, r->server);
> +}
> +
> +/* Create a new table consisting of those elements from an output
> + * headers table that are allowed to be stored in a cache;
> + * ensure there is a content type and capture any errors.
> + */
> +CACHE_DECLARE(apr_table_t *)ap_cache_cacheable_hdrs_out(request_rec * r)
> +{
> +    apr_table_t *headers_out;
> +
> +    headers_out = ap_cache_cacheable_hdrs(r->pool, r->headers_out,
> +                                                  r->server);
> +
> +        if (!apr_table_get(headers_out, "Content-Type")
> +            && r->content_type) {
> +            apr_table_setn(headers_out, "Content-Type",
> +                           ap_make_content_type(r, r->content_type));
> +        }
> +
> +        headers_out = apr_table_overlay(r->pool, headers_out,
> +                                        r->err_headers_out);
> +
> +    return headers_out;
> +}

Regards

RĂ¼diger

Mime
View raw message