httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Ruediger Pluem <rpl...@apache.org>
Subject Re: Caching the useful headers
Date Sat, 09 Feb 2008 17:35:46 GMT


On 02/09/2008 05:37 PM, Dirk-Willem van Gulik wrote:
> On Feb 9, 2008, at 12:46 AM, Ruediger Pluem wrote:
> 
>>> Or we keep the old slightly odd names.
>>
>> I currently see no essential need for these changes to be 
>> backportable. So
>> I would prefer cleaning up the names. But there might be a compromise:
>>
>> 1. Make ap_cache_cacheable_hdrs a macro that is defined as 
>> ap_cache_cacheable_hdrs_out
>> 2. Leave ap_cache_cacheable_hdrs_out as is.
>> 3. Create ap_cache_cacheable_headers_out / ap_cache_cacheable_headers_in
>>
>> This should only require a minor bump.
> 
> Bit of grepping yields very few google hits on people using it. And with 
> the #define() we'll have the
> issue going forward as well. How about below. And we wack the 
> ap_cache_cacheable_hdrs_out()
> function on the next major bump.

IMHO both approaches are somewhat equivalent: They require a minor bump
now and a major as soon as we drop the ap_cache_cacheable_hdrs_out symbol
either by deleting it or by renaming ap_cache_cacheable_hdrs_out to
ap_cache_cacheable_headers and dropping the macro.

> 
> Dw.
> 
> Index: cache_util.c
> ===================================================================
> --- cache_util.c    (revision 620145)
> +++ cache_util.c    (working copy)
> @@ -571,10 +571,10 @@
>      return apr_pstrdup(p, hashfile);
>  }
> 
> -/* Create a new table consisting of those elements from an input
> +/* Create a new table consisting of those elements from an
>   * headers table that are allowed to be stored in a cache.
>   */
> -CACHE_DECLARE(apr_table_t *)ap_cache_cacheable_hdrs_out(apr_pool_t *pool,
> +CACHE_DECLARE(apr_table_t *)ap_cache_cacheable_headers(apr_pool_t *pool,
>                                                          apr_table_t *t,
>                                                          server_rec *s)
>  {
> @@ -582,12 +582,17 @@
>      char **header;
>      int i;
> 
> +    if (t == NULL) {
> +    return apr_table_make(pool, 10);
> +    };
> +

Why this? Just some type of defensive programming if someone feeds in a t == NULL?
If yes, I would prefer return NULL instead of creating an empty table.
Garbage in, garbage 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
>       */
>      apr_table_t *headers_out;
>      headers_out = apr_table_copy(pool, t);
> +
>      apr_table_unset(headers_out, "Connection");
>      apr_table_unset(headers_out, "Keep-Alive");
>      apr_table_unset(headers_out, "Proxy-Authenticate");
> @@ -608,3 +613,46 @@
>      }
>      return headers_out;
>  }
> +
> +/* Legacy call - functionally equivalent to ap_cache_cacheable_headers.
> + * @warning this call will possibly be removed on the next API bump.

How about using @deprecated here instead of warning?

> + */
> +CACHE_DECLARE(apr_table_t *)ap_cache_cacheable_hdrs_out(apr_pool_t *pool,
> +                                                        apr_table_t *t,
> +                                                        server_rec *s)
> +{
> +    return ap_cache_cacheable_headers(p,t,s);

Style nitpick:

return ap_cache_cacheable_headers(p, t, s);



> Index: mod_cache.c
> ===================================================================
> --- mod_cache.c    (revision 620145)
> +++ mod_cache.c    (working copy)
> @@ -752,10 +752,12 @@
>           * However, before doing that, we need to first merge in
>           * err_headers_out and we also need to strip any hop-by-hop
>           * headers that might have snuck in.
> +         *
> +     * XX check -- we're not patching up content-type.

Style: Indenting

>           */
>          r->headers_out = apr_table_overlay(r->pool, r->headers_out,
>                                             r->err_headers_out);
> -        r->headers_out = ap_cache_cacheable_hdrs_out(r->pool, 
> r->headers_out,
> +        r->headers_out = ap_cache_cacheable_headers(r->pool, 
> r->headers_out,
>                                                       r->server);
>          apr_table_clear(r->err_headers_out);
> 

> Index: mod_cache.h
> ===================================================================
> --- mod_cache.h    (revision 620145)
> +++ mod_cache.h    (working copy)
> @@ -288,9 +288,27 @@
>                                      const char *key, char **val);
>  CACHE_DECLARE(const char *)ap_cache_tokstr(apr_pool_t *p, const char 
> *list, const char **str);
> 
> -/* Create a new table consisting of those elements from a request_rec's
> - * headers_out that are allowed to be stored in a cache
> +* Create a new table consisting of those elements from an

Style: Indenting

> + * headers table that are allowed to be stored in a cache.
>   */
> +CACHE_DECLARE(apr_table_t *)ap_cache_cacheable_headers(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_headers_in(request_rec * 
> r);
> +
> +/* 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_headers_out(request_rec 
> * r);
> +
> +/* Legacy call - functionally equivalent to ap_cache_cacheable_headers.
> + * @warning this call will possibly be removed on the next API bump.

See above comment.

Regards

RĂ¼diger

Mime
View raw message