On Tue, Aug 16, 2005 at 01:32:03PM +0100, Colm MacCarthaigh wrote:
>
> More mod_cache fix-ups;
>
> "CacheEnable /"
>
> isn't very useful for forward proxy servers. This patch makes;
>
> CacheEnable /
> CacheEnable ftp://
> CacheEnable http://somesite/
> CacheDisable www.apache.org/blah/
> CacheDisable ftp://
> CacheDisable http://httpd.apache.org/manual/
>
> work.
Comments inline.
> Index: docs/manual/mod/mod_cache.html.en
> ===================================================================
> --- docs/manual/mod/mod_cache.html.en (revision 232960)
> +++ docs/manual/mod/mod_cache.html.en (working copy)
> @@ -87,14 +96,14 @@
> <IfModule mod_cache.c><br />
> <span class="indent">
> #LoadModule disk_cache_module modules/mod_disk_cache.so<br />
> - # If you want to use mod_disk_cache instead of mod_mem_cache,
> - # uncomment the line above and comment out the LoadModule line below.
> + # If you want to use mod_disk_cache instead of mod_mem_cache,<br />
> + # uncomment the line above and comment out the LoadModule line
> below.<br />
> <IfModule mod_disk_cache.c><br />
> <span class="indent">
> CacheRoot c:/cacheroot<br />
> CacheEnable disk /<br />
> - CacheDirLevels 5<br />
> - CacheDirLength 3<br />
> + CacheDirLevels 2<br />
> + CacheDirLength 1<br />
Why are you changing these? (I don't think it's relevant to this patch.)
..snip...
> Index: modules/cache/cache_util.c
> ===================================================================
> --- modules/cache/cache_util.c (revision 232960)
> +++ modules/cache/cache_util.c (working copy)
> @@ -24,23 +24,64 @@
>
> extern module AP_MODULE_DECLARE_DATA cache_module;
>
> +/* Determine if "url" matches the hostname, scheme and port found
> + * in "filter". Comparisons are case-insensitive.
> + */
> +static int uri_hsp_meets_conditions(apr_uri_t filter, apr_uri_t url)
I find 'hsp' to be rather non-intuitive. (It took me a bit to figure out what
it meant.) How about just uri_meets_conditions? And, why not push the path
check to this function?
...snip...
> @@ -114,7 +106,7 @@
> * add cache_out filter
> * return OK
> */
> - rv = cache_select_url(r, url);
> + rv = cache_select_url(r);
> if (rv != OK) {
> if (rv == DECLINED) {
> if (!lookup) {
This cache_select_url function probably needs to get a new name. It may have
made sense a while ago; but the name doesn't seem to jive with its
implementation now. (AFAICT, this change is unrelated to the rest of the
patch.)
...snip...
The rest of it looks fine. +1. -- justin
