[Qemu-devel] [PATCH] qapi: Fix memory leak

[Qemu-devel] [PATCH] qapi: Fix memory leak

[Stefan Weil]

valgrind report:

==24534== 232 bytes in 2 blocks are definitely lost in loss record 1,245 of 1,601
==24534==    at 0x4824F20: malloc (vg_replace_malloc.c:236)
==24534==    by 0x293C88: malloc_and_trace (vl.c:2281)
==24534==    by 0x489AD99: ??? (in /lib/libglib-2.0.so.0.2400.1)
==24534==    by 0x489B23B: g_malloc0 (in /lib/libglib-2.0.so.0.2400.1)
==24534==    by 0x2B4EFC: opts_visitor_new (opts-visitor.c:376)
==24534==    by 0x29DEA5: net_client_init (net.c:708)
==24534==    by 0x29E6C7: net_init_client (net.c:966)
==24534==    by 0x2C2179: qemu_opts_foreach (qemu-option.c:1114)
==24534==    by 0x29E85B: net_init_clients (net.c:1008)
==24534==    by 0x296F40: main (vl.c:3463)

Signed-off-by: Stefan Weil <sw@weilnetz.de>
---
 qapi/opts-visitor.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/qapi/opts-visitor.c b/qapi/opts-visitor.c
index a59d306..e048b6c 100644
--- a/qapi/opts-visitor.c
+++ b/qapi/opts-visitor.c
@@ -416,7 +416,7 @@ opts_visitor_cleanup(OptsVisitor *ov)
         g_hash_table_destroy(ov->unprocessed_opts);
     }
     g_free(ov->fake_id_opt);
-    memset(ov, '\0', sizeof *ov);
+    g_free(ov);
 }
 
 
-- 
1.7.0.4

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

[Stefan Weil]

Am 18.08.2012 22:51, schrieb Stefan Weil:
> valgrind report:
>
> ==24534== 232 bytes in 2 blocks are definitely lost in loss record 1,245 of 1,601
> ==24534==    at 0x4824F20: malloc (vg_replace_malloc.c:236)
> ==24534==    by 0x293C88: malloc_and_trace (vl.c:2281)
> ==24534==    by 0x489AD99: ??? (in /lib/libglib-2.0.so.0.2400.1)
> ==24534==    by 0x489B23B: g_malloc0 (in /lib/libglib-2.0.so.0.2400.1)
> ==24534==    by 0x2B4EFC: opts_visitor_new (opts-visitor.c:376)
> ==24534==    by 0x29DEA5: net_client_init (net.c:708)
> ==24534==    by 0x29E6C7: net_init_client (net.c:966)
> ==24534==    by 0x2C2179: qemu_opts_foreach (qemu-option.c:1114)
> ==24534==    by 0x29E85B: net_init_clients (net.c:1008)
> ==24534==    by 0x296F40: main (vl.c:3463)
>

valgrind reports a lot more memory leaks which are related to
function qemu_allocate_irqs. In many cases, its return value
should be free'd. g_malloc / g_free can be avoided by adding
a new function

void qemu_init_irqs(qemu_irq_handler handler, void *opaque,
                     qemu_irq *irqs, int n);

If this is ok, I'll send patches which add and use the new
function instead of qemu_allocate_irqs, too.

Regards,

Stefan Weil

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

[Peter Maydell]

On 18 August 2012 22:01, Stefan Weil <sw@weilnetz.de> wrote:
> valgrind reports a lot more memory leaks which are related to
> function qemu_allocate_irqs. In many cases, its return value
> should be free'd. g_malloc / g_free can be avoided by adding
> a new function
>
> void qemu_init_irqs(qemu_irq_handler handler, void *opaque,
>                     qemu_irq *irqs, int n);
>
> If this is ok, I'll send patches which add and use the new
> function instead of qemu_allocate_irqs, too.

So I think the long term plan is that these will basically go
away in favour of some kind of Pin based infrastructure.
Given that, it might not be worth doing unless these leaks
are more than "memory lasts for lifetime of qemu and we
don't free it explicitly" (maybe you could actual leaks in a
hotplug scenario?)

-- PMM

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

[Michael Tokarev]

On 19.08.2012 00:51, Stefan Weil wrote:

> +++ b/qapi/opts-visitor.c
> @@ -416,7 +416,7 @@ opts_visitor_cleanup(OptsVisitor *ov)

>          g_hash_table_destroy(ov->unprocessed_opts);
>      }
>      g_free(ov->fake_id_opt);
> -    memset(ov, '\0', sizeof *ov);
> +    g_free(ov);

Shouldn't the function be named opts_visitor_free() or .._destroy()
in this case?  Or should maybe the caller free "ov" instead of
this function?  To me it looks like either both free+rename shoud
be made, or none.

Thanks,

/mjt

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

[Laszlo Ersek]

On 08/18/12 23:06, Michael Tokarev wrote:
> On 19.08.2012 00:51, Stefan Weil wrote:
> 
>> +++ b/qapi/opts-visitor.c
>> @@ -416,7 +416,7 @@ opts_visitor_cleanup(OptsVisitor *ov)
> 
>>          g_hash_table_destroy(ov->unprocessed_opts);
>>      }
>>      g_free(ov->fake_id_opt);
>> -    memset(ov, '\0', sizeof *ov);
>> +    g_free(ov);
> 
> Shouldn't the function be named opts_visitor_free() or .._destroy()
> in this case?  Or should maybe the caller free "ov" instead of
> this function?  To me it looks like either both free+rename shoud
> be made, or none.

All of

- string-output-visitor.c
- string-input-visitor.c
- qmp-output-visitor.c
- qmp-input-visitor.c
- qapi-dealloc-visitor.c

free the visitor in *_cleanup(). (Which is not to say they shouldn't all
be renamed, only that the patch uni-forms opts-visitor with the rest.)

Thanks,
Laszlo

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

[Laszlo Ersek]

On 08/18/12 22:51, Stefan Weil wrote:
> valgrind report:
> 
> ==24534== 232 bytes in 2 blocks are definitely lost in loss record 1,245 of 1,601
> ==24534==    at 0x4824F20: malloc (vg_replace_malloc.c:236)
> ==24534==    by 0x293C88: malloc_and_trace (vl.c:2281)
> ==24534==    by 0x489AD99: ??? (in /lib/libglib-2.0.so.0.2400.1)
> ==24534==    by 0x489B23B: g_malloc0 (in /lib/libglib-2.0.so.0.2400.1)
> ==24534==    by 0x2B4EFC: opts_visitor_new (opts-visitor.c:376)
> ==24534==    by 0x29DEA5: net_client_init (net.c:708)
> ==24534==    by 0x29E6C7: net_init_client (net.c:966)
> ==24534==    by 0x2C2179: qemu_opts_foreach (qemu-option.c:1114)
> ==24534==    by 0x29E85B: net_init_clients (net.c:1008)
> ==24534==    by 0x296F40: main (vl.c:3463)
> 
> Signed-off-by: Stefan Weil <sw@weilnetz.de>
> ---
>  qapi/opts-visitor.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/qapi/opts-visitor.c b/qapi/opts-visitor.c
> index a59d306..e048b6c 100644
> --- a/qapi/opts-visitor.c
> +++ b/qapi/opts-visitor.c
> @@ -416,7 +416,7 @@ opts_visitor_cleanup(OptsVisitor *ov)
>          g_hash_table_destroy(ov->unprocessed_opts);
>      }
>      g_free(ov->fake_id_opt);
> -    memset(ov, '\0', sizeof *ov);
> +    g_free(ov);
>  }
>  
>  

I don't remember why I thought the object would / should live on. I must
have implemented what I thought was safe / correct for the life-cycle,
except I got the life-cycle wrong. Face, meet palm.

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks & sorry!
Laszlo

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

[Luiz Capitulino]

On Sun, 19 Aug 2012 12:12:29 +0200
Laszlo Ersek <lersek@redhat.com> wrote:

> On 08/18/12 22:51, Stefan Weil wrote:
> > valgrind report:
> > 
> > ==24534== 232 bytes in 2 blocks are definitely lost in loss record 1,245 of 1,601
> > ==24534==    at 0x4824F20: malloc (vg_replace_malloc.c:236)
> > ==24534==    by 0x293C88: malloc_and_trace (vl.c:2281)
> > ==24534==    by 0x489AD99: ??? (in /lib/libglib-2.0.so.0.2400.1)
> > ==24534==    by 0x489B23B: g_malloc0 (in /lib/libglib-2.0.so.0.2400.1)
> > ==24534==    by 0x2B4EFC: opts_visitor_new (opts-visitor.c:376)
> > ==24534==    by 0x29DEA5: net_client_init (net.c:708)
> > ==24534==    by 0x29E6C7: net_init_client (net.c:966)
> > ==24534==    by 0x2C2179: qemu_opts_foreach (qemu-option.c:1114)
> > ==24534==    by 0x29E85B: net_init_clients (net.c:1008)
> > ==24534==    by 0x296F40: main (vl.c:3463)
> > 
> > Signed-off-by: Stefan Weil <sw@weilnetz.de>
> > ---
> >  qapi/opts-visitor.c |    2 +-
> >  1 files changed, 1 insertions(+), 1 deletions(-)
> > 
> > diff --git a/qapi/opts-visitor.c b/qapi/opts-visitor.c
> > index a59d306..e048b6c 100644
> > --- a/qapi/opts-visitor.c
> > +++ b/qapi/opts-visitor.c
> > @@ -416,7 +416,7 @@ opts_visitor_cleanup(OptsVisitor *ov)
> >          g_hash_table_destroy(ov->unprocessed_opts);
> >      }
> >      g_free(ov->fake_id_opt);
> > -    memset(ov, '\0', sizeof *ov);
> > +    g_free(ov);
> >  }
> >  
> >  
> 
> I don't remember why I thought the object would / should live on. I must
> have implemented what I thought was safe / correct for the life-cycle,
> except I got the life-cycle wrong. Face, meet palm.
> 
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Applied to the qmp branch for 1.2.