You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Colm MacCarthaigh <co...@stdlib.net> on 2005/08/16 14:32:03 UTC
[PATCH] Make CacheEnable useful for forward proxies
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.
--
Colm MacCárthaigh Public Key: colm+pgp@stdlib.net
Re: [PATCH] Make CacheEnable useful for forward proxies
Posted by Graham Leggett <mi...@sharp.fm>.
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.
+1 in concept, this is very cool (haven't had a chance to test the patch
itself yet, when my inbox dips below 1000 I'll do it).
Regards,
Graham
--
Re: [PATCH] Make CacheEnable useful for forward proxies
Posted by Colm MacCarthaigh <co...@stdlib.net>.
On Tue, Aug 16, 2005 at 06:23:28PM -0700, Justin Erenkrantz wrote:
> > - 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.)
Sorry, they slipped in, I'm currently working on a patch to the
mod_cache documentation and a new Caching user guide. I thought I had
cought everything from my diff.
5 & 3 as defaults are getting caught up in a lot of places, including
mine and PaulQ's talks at ApacheConEU - and well, as I think was pointed
out on the list last week, they're awful default values.
> > +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?
Better.
> And, why not push the path check to this function?
At some point I think it was purely to lessen the ammount of confusion
by having a 3 argument function with the pathlen's. Passing the
container structure won't work, because they differ for cacheenable and
cachedisable. I'll just make it a 3-argument function.
> 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.)
That change is from getting rid of un-used variable compiler warnings.
--
Colm MacCárthaigh Public Key: colm+pgp@stdlib.net
Re: [PATCH] Make CacheEnable useful for forward proxies
Posted by Justin Erenkrantz <ju...@erenkrantz.com>.
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