headers
Stefan Weil | 18 Aug 2012 22:51
Picon
Favicon

[PATCH] qapi: Fix memory leak

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 <at> 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
 <at>  <at>  -416,7 +416,7  <at>  <at>  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);
 }
(Continue reading)

Permalink | Reply |
headers
Stefan Weil | 18 Aug 2012 23:01
Picon
Favicon

Re: [PATCH] qapi: Fix memory leak

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
(Continue reading)

Permalink | Reply |
headers
Peter Maydell | 19 Aug 2012 12:37
Favicon
Gravatar

Re: [PATCH] qapi: Fix memory leak

On 18 August 2012 22:01, Stefan Weil <sw <at> 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
Permalink | Reply |
headers
Michael Tokarev | 18 Aug 2012 23:06
Picon

Re: [PATCH] qapi: Fix memory leak

On 19.08.2012 00:51, Stefan Weil wrote:

> +++ b/qapi/opts-visitor.c
>  <at>  <at>  -416,7 +416,7  <at>  <at>  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
Permalink | Reply |
headers
Laszlo Ersek | 19 Aug 2012 12:15
Picon
Favicon

Re: [PATCH] qapi: Fix memory leak

On 08/18/12 23:06, Michael Tokarev wrote:
> On 19.08.2012 00:51, Stefan Weil wrote:
> 
>> +++ b/qapi/opts-visitor.c
>>  <at>  <at>  -416,7 +416,7  <at>  <at>  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
(Continue reading)

Permalink | Reply |
headers
Laszlo Ersek | 19 Aug 2012 12:12
Picon
Favicon

Re: [PATCH] qapi: Fix memory leak

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 <at> 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
>  <at>  <at>  -416,7 +416,7  <at>  <at>  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);
(Continue reading)

Permalink | Reply |
headers
Luiz Capitulino | 20 Aug 2012 16:05
Picon
Favicon

Re: [PATCH] qapi: Fix memory leak

On Sun, 19 Aug 2012 12:12:29 +0200
Laszlo Ersek <lersek <at> 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 <at> 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
> >  <at>  <at>  -416,7 +416,7  <at>  <at>  opts_visitor_cleanup(OptsVisitor *ov)
> >          g_hash_table_destroy(ov->unprocessed_opts);
> >      }
(Continue reading)

Permalink | Reply |

Gmane