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