httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From r.pl...@t-online.de
Subject Re: [PATCH] fix incorrect 304's responses when cache is unwritable
Date Mon, 08 Aug 2005 08:33:47 GMT


Colm MacCarthaigh wrote:
> 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.

I should be at least on Unix because it simply calls rmdir and this returns
ENOTEMPTY on the removal of non empty directories. This results in
apr_dir_remove returning APR_ENOTEMPTY. The only problem I see
with the current state of my patch is to get the information of the cache_root
passed to remove_url. I think this is needed to have a clear stop signal where
to stop deleting empty directories at least. Otherwise you may run until / in the worst
case :-).

[..cut..]


> 
> 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

        I added them because it took me some time to figure out that
        setting loglevel to debug is not enough to get these messages
        printed.

> 
> 	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.

        Agreed. Feel free to adjust this.

> 
> 	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.

        You are right. In the current usage of cache_remove_url
        this check does not make sense, but I wanted to keep the
        scope of this function somewhat broder to allow to call
        this function in the case the caller wants to remove the
        entity behind cache->handle.

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

Looking forward to this. Please let us know the results.

Regards

RĂ¼diger

Mime
View raw message