Am 25.02.2013 um 13:36 schrieb Peter Maydell <peter.mayd...@linaro.org>:
> 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.
sth like this?
diff --git a/page_cache.c b/page_cache.c
index 376f1db..04205ee 100644
--- a/page_cache.c
+++ b/page_cache.c
@@ -196,21 +196,19 @@ int64_t cache_resize(PageCache *cache, int64_t
new_num_pages)
/* check for collision, if there is, keep MRU page */
new_it = cache_get_by_addr(new_cache, old_it->it_addr);
if (new_it->it_data) {
- /* keep the MRU page */
if (new_it->it_age >= old_it->it_age) {
g_free(old_it->it_data);
- } else {
- g_free(new_it->it_data);
- new_it->it_data = old_it->it_data;
- new_it->it_age = old_it->it_age;
- new_it->it_addr = old_it->it_addr;
+ continue;
}
- } else {
- cache_insert(new_cache, old_it->it_addr, old_it->it_data);
}
+ g_free(new_it->it_data);
+ new_it->it_data = old_it->it_data;
+ new_it->it_age = old_it->it_age;
+ new_it->it_addr = old_it->it_addr;
}
}
+ g_free(cache->page_cache);
cache->page_cache = new_cache->page_cache;
cache->max_num_items = new_cache->max_num_items;
cache->num_items = new_cache->num_items;
>
> -- PMM
