Mailing-List: contact dev-help@httpd.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@httpd.apache.org
Received-SPF: pass (asf.osuosl.org: local policy)
Date: Mon, 8 Aug 2005 01:01:25 +0100
From: Colm MacCarthaigh <colm@stdlib.net>
To: dev@httpd.apache.org
Subject: Re: [PATCH] fix incorrect 304's responses when cache is unwritable
Message-ID: <20050808000125.GB25723@stdlib.net>
Reply-To: colm@stdlib.net
References: <20050807164843.GA22232@stdlib.net> <42F66813.3020606@gmx.de>
 <20050807212924.GA24773@stdlib.net> <42F68F01.5020508@gmx.de>
Mime-Version: 1.0
Content-Type: text/plain; charset=iso-8859-15
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <42F68F01.5020508@gmx.de>
User-Agent: Mutt/1.5.9i

On Mon, Aug 08, 2005 at 12:45:21AM +0200, r.pluem@t-online.de wrote:
> Is a traversal really needed? What about going back the full path of the
> header / data file to the cache root and removing each component on the
> way by calling apr_dir_remove on each component until it fails?

I'm not sure if apr_dir_remove guarantees failure when operated on
non-empty directories. If it does then that's an easy enough way.

> > Are 404's being served incorrectly in some circumstances? 
> 
> You are right that 404's do not get cached. But if a cached resource
> vanishes on the backend the cache entry is not removed. 

Aha, now I understand what this patch is meant to do. 

> It is needed because:
> 
> - In the case of an internal Apache 404 error page the content filter
> chain is not run (especially not CACHE_SAVE_FILTER). This is the
> reason why cache_removal_url is a protocol filter.
> 
> - In the case of an user specified error page with ErrorDocument the
> CACHE_SAVE_FILTER is run with the wrong request (the one that belongs
> to the custom error page, not the one of the original request).

Makes sense, O.k., now looking at it and knowing what it is supposed to
do, it looks fine. The only things I've noticed are;

	the obviously mis-copied CACHE_SAVE coment in 
	cache_remove_url_filter()  :-)

	The extraneous "-e debug" comments in mod_disk_cache

	In mod_disk_cache, you try to delete the data file even
	if removing the header file was unsuccesful. Personally
	I wouldn't be very comfortable with this, as the header
	is a useful source of information to an adminstrator 
	tracking down problems and it's only easy way to determine
	what the data file is. If you can't delete the header 
	file, I'd recommend leaving the data file in-place. They 
	make more sense if they are both in the same state.

	In cache_remove_url, you have code which tries to
	determine if the cache->handle or cache->stale_handle
	should be removed, but shouldn't it always be the
	stale_handle? You only add the remove_url filter if
	cache_select_url() != OK, which means cache->handle
	will always be NULL.

But apart from those looks fine. I'll merge it with my small
patch and test it properly tomorrow.

-- 
Colm MacC�rthaigh                        Public Key: colm+pgp@stdlib.net