| Message ID | 1345323089-26232-1-git-send-email-sw@weilnetz.de |
|---|---|
| State | Accepted |
| Headers | show |
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
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
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
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
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
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.
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); }
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(-)