Re: [Qemu-devel] [PATCH] page_cache: Fix memory leak

[Peter Maydell]

On 25 February 2013 11:42, Peter Lieven <address@hidden> wrote:
> XBZRLE encoded migration introduced a MRU page cache meachnism.
> Unfortunately, cached items where never freed on a collision.
>
> This lead to out of memory conditions during XBZRLE migration
> if the page cache was small and there where a lot of collisions.
>
> Signed-off-by: Peter Lieven <address@hidden>
> ---
>  page_cache.c |    4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/page_cache.c b/page_cache.c
> index ba5640b..a6c3a15 100644
> --- a/page_cache.c
> +++ b/page_cache.c
> @@ -152,8 +152,10 @@ void cache_insert(PageCache *cache, uint64_t addr,
> uint8_t *pdata)
>      /* actual update of entry */
>      it = cache_get_by_addr(cache, addr);
>
> -    if (!it->it_data) {
> +    if (it->it_data == NULL) {

Please don't make minor syntactic tweaks in patches like this,
it makes them harder to review.

>          cache->num_items++;
> +    } else {
> +        g_free(it->it_data);
>      }

g_free(NULL) is OK so you could just make it unconditional.

Looking at the code in general I think it is probably the
right thing to move it to "cache owns (ie duplicates
and frees) the data", since the code is already most of
the way there and just has some bits which aren't.

-- PMM