httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Justin Erenkrantz <jus...@erenkrantz.com>
Subject Re: [PATCH] Make CacheEnable useful for forward proxies
Date Wed, 17 Aug 2005 01:23:28 GMT
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 @@
>        &lt;IfModule mod_cache.c&gt;<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
/>
>          &lt;IfModule mod_disk_cache.c&gt;<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

Mime
View raw message