On Mon, Aug 08, 2005 at 12:45:21AM +0200, [EMAIL PROTECTED] 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: [EMAIL PROTECTED]
