On 25 February 2013 12:17, Peter Lieven <p...@dlhnet.de> wrote:
> Am 25.02.2013 um 13:13 schrieb Peter Maydell <peter.mayd...@linaro.org>:
>> Doesn't this introduce a leak on cache resize in the case where
>> the element being moved from the old cache to the new does not
>> collide with any element we've already moved? [ie the code
>> path where we just cache_insert() the old item's data].
>
> you are right. maybe we need different functions for inserts made
> during resize and inserts from outside.
Looking a little more closely, I think you need that anyway,
because cache_insert() updates the it_age field, when I would
have expected that in the "just moving everything over to the
resized cache" case we would want to retain the existing age.
Since cache_resize() already has code in it that does a
direct access and copy of CacheItem* fields [for the "copy
old_it over new_it" case] it might be cleaner to either
(a) have all the cases open-coded in cache_resize() rather
than calling cache_insert(), or (b) abstract out the
whole of the resize inner loop into something like
cache_insert_item(PageCache *cache, CacheItem *olditem)
{
/* Insert olditem (an item from a different page cache) into this one.
* If there is a collision then we keep the data from whichever of
* olditem and the existing entry is newer. In either case, olditem's
* data pointer is either copied into the new cache or freed, so
* the caller must do nothing further with it.
*/
}
Extra bonus leak: I think cache_resize() is leaking
cache->page_cache.
-- PMM
