httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Colm MacCarthaigh <c...@stdlib.net>
Subject Re: [PATCH] fix incorrect 304's responses when cache is unwritable
Date Mon, 08 Aug 2005 12:25:46 GMT
On Mon, Aug 08, 2005 at 10:33:47AM +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.

I'm just being paranoid, but I'm not certain of the behaviour of the
win32, beos or other future implementations, and the APR itself makes no
claims about apr_dir_remove's behaviour - it just says "Remove directory
from the file system" which is pretty ambiguous. I've now checked the
win32 and beos implementations, and they seem safe, so I've submitted a
silly doxygen comment patch to dev@apr to try and get rid of the problem
for good.

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

x. At the very least, I'd definitely make stale_handle the default if
both pointers are non-NULL :)

O.k., I've merged our two patches, but I've changed a few things, tell
me if there's anothing you think is wrong;

  1. Did some minor whitespace/style cleanups, nothing
     big. also changed the instances of CACHE_REMOVAL_URL
     in comments to CACHE_REMOVE_URL, and a few small things like
     that cought. Also made the log messages consistent
     with the rest of mod_cache, and added some to aid 
     testing.

  2. modified the mod_disk_cache code to delete the data only
     if the header could be deleted first.

  3. renamed the cache_remove_url_filter pointer in the cache 
     struct to remove_url_filter, cache->cache_$something seemed
     redundant and it makes the formatting neater :-)

  4. Fixed a bug in the mod_disk_cache code. It was using
     dobj->datafile in the dobj->hdrsfile section for log
     output. 

  5. Swapped the order of the ternary check in cache_storage,
     so that we'll remove the stale_handle if both the stale_hand
     and handle are non-NULL.

The patch is attached, and I've been testing it for the last few hours
without problem. The code is now running on ftp.heanet.ie. (along
with htcacheclean -t).

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

Mime
View raw message