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
