| Message ID | 20201127071803.2479462-2-ganqixin@huawei.com |
|---|---|
| State | New |
| Headers | show |
| Series | Fix some memleaks caused by ptimer_init | expand |
On Fri, 27 Nov 2020 at 07:19, Gan Qixin <ganqixin@huawei.com> wrote: > > When running device-introspect-test, a memory leak occurred in the a10_pit_init > function, so use ptimer_free() in the finalize function to avoid it. > > ASAN shows memory leak stack: > > Indirect leak of 288 byte(s) in 6 object(s) allocated from: > #0 0xffffab97e1f0 in __interceptor_calloc (/lib64/libasan.so.5+0xee1f0) > #1 0xffffab256800 in g_malloc0 (/lib64/libglib-2.0.so.0+0x56800) > #2 0xaaabf555db84 in timer_new_full /qemu/include/qemu/timer.h:523 > #3 0xaaabf555db84 in timer_new /qemu/include/qemu/timer.h:544 > #4 0xaaabf555db84 in timer_new_ns /qemu/include/qemu/timer.h:562 > #5 0xaaabf555db84 in ptimer_init /qemu/hw/core/ptimer.c:433 > #6 0xaaabf57415e8 in a10_pit_init /qemu/hw/timer/allwinner-a10-pit.c:278 > #7 0xaaabf6339f6c in object_initialize_with_type /qemu/qom/object.c:515 > #8 0xaaabf633ca04 in object_initialize_child_with_propsv /qemu/qom/object.c:564 > #9 0xaaabf633cc08 in object_initialize_child_with_props /qemu/qom/object.c:547 > #10 0xaaabf5b94680 in aw_a10_init /qemu/hw/arm/allwinner-a10.c:49 > #11 0xaaabf6339f6c in object_initialize_with_type /qemu/qom/object.c:515 > #12 0xaaabf633a1e0 in object_new_with_type /qemu/qom/object.c:729 > > Reported-by: Euler Robot <euler.robot@huawei.com> > Signed-off-by: Gan Qixin <ganqixin@huawei.com> > --- > Cc: Beniamino Galvani <b.galvani@gmail.com> > --- > hw/timer/allwinner-a10-pit.c | 21 ++++++++++++++++----- > 1 file changed, 16 insertions(+), 5 deletions(-) > > diff --git a/hw/timer/allwinner-a10-pit.c b/hw/timer/allwinner-a10-pit.c > index f84fc0ea25..be211983b0 100644 > --- a/hw/timer/allwinner-a10-pit.c > +++ b/hw/timer/allwinner-a10-pit.c > @@ -279,6 +279,16 @@ static void a10_pit_init(Object *obj) > } > } > > +static void a10_pit_finalize(Object *obj) > +{ > + AwA10PITState *s = AW_A10_PIT(obj); > + int i; > + > + for (i = 0; i < AW_A10_PIT_TIMER_NR; i++) { > + ptimer_free(s->timer[i]); > + } > +} > + > static void a10_pit_class_init(ObjectClass *klass, void *data) > { > DeviceClass *dc = DEVICE_CLASS(klass); > @@ -290,11 +300,12 @@ static void a10_pit_class_init(ObjectClass *klass, void *data) > } > > static const TypeInfo a10_pit_info = { > - .name = TYPE_AW_A10_PIT, > - .parent = TYPE_SYS_BUS_DEVICE, > - .instance_size = sizeof(AwA10PITState), > - .instance_init = a10_pit_init, > - .class_init = a10_pit_class_init, > + .name = TYPE_AW_A10_PIT, > + .parent = TYPE_SYS_BUS_DEVICE, > + .instance_size = sizeof(AwA10PITState), > + .instance_init = a10_pit_init, > + .instance_finalize = a10_pit_finalize, > + .class_init = a10_pit_class_init, > }; Please don't make unrelated whitespace changes like this in a patch. We don't line up the assignments in this sort of struct -- this is deliberate, so that if a new line is added whose field name happens to be longer than those used already, the patch does not have to touch all the lines in the struct to maintain the formatting. Instead you get a readable diff where only the new line changes, not all the others. thanks -- PMM
On Mon, 14 Dec 2020 at 16:02, Peter Maydell <peter.maydell@linaro.org> wrote: > > On Fri, 27 Nov 2020 at 07:19, Gan Qixin <ganqixin@huawei.com> wrote: > > > > When running device-introspect-test, a memory leak occurred in the a10_pit_init > > function, so use ptimer_free() in the finalize function to avoid it. > > > > ASAN shows memory leak stack: > > > > Indirect leak of 288 byte(s) in 6 object(s) allocated from: > > #0 0xffffab97e1f0 in __interceptor_calloc (/lib64/libasan.so.5+0xee1f0) > > #1 0xffffab256800 in g_malloc0 (/lib64/libglib-2.0.so.0+0x56800) > > #2 0xaaabf555db84 in timer_new_full /qemu/include/qemu/timer.h:523 > > #3 0xaaabf555db84 in timer_new /qemu/include/qemu/timer.h:544 > > #4 0xaaabf555db84 in timer_new_ns /qemu/include/qemu/timer.h:562 > > #5 0xaaabf555db84 in ptimer_init /qemu/hw/core/ptimer.c:433 > > #6 0xaaabf57415e8 in a10_pit_init /qemu/hw/timer/allwinner-a10-pit.c:278 > > #7 0xaaabf6339f6c in object_initialize_with_type /qemu/qom/object.c:515 > > #8 0xaaabf633ca04 in object_initialize_child_with_propsv /qemu/qom/object.c:564 > > #9 0xaaabf633cc08 in object_initialize_child_with_props /qemu/qom/object.c:547 > > #10 0xaaabf5b94680 in aw_a10_init /qemu/hw/arm/allwinner-a10.c:49 > > #11 0xaaabf6339f6c in object_initialize_with_type /qemu/qom/object.c:515 > > #12 0xaaabf633a1e0 in object_new_with_type /qemu/qom/object.c:729 > > > > Reported-by: Euler Robot <euler.robot@huawei.com> > > Signed-off-by: Gan Qixin <ganqixin@huawei.com> > > --- > > Cc: Beniamino Galvani <b.galvani@gmail.com> > > --- > > hw/timer/allwinner-a10-pit.c | 21 ++++++++++++++++----- > > 1 file changed, 16 insertions(+), 5 deletions(-) > > > > diff --git a/hw/timer/allwinner-a10-pit.c b/hw/timer/allwinner-a10-pit.c > > index f84fc0ea25..be211983b0 100644 > > --- a/hw/timer/allwinner-a10-pit.c > > +++ b/hw/timer/allwinner-a10-pit.c > > @@ -279,6 +279,16 @@ static void a10_pit_init(Object *obj) > > } > > } > > > > +static void a10_pit_finalize(Object *obj) > > +{ > > + AwA10PITState *s = AW_A10_PIT(obj); > > + int i; > > + > > + for (i = 0; i < AW_A10_PIT_TIMER_NR; i++) { > > + ptimer_free(s->timer[i]); > > + } > > +} > > + > > static void a10_pit_class_init(ObjectClass *klass, void *data) > > { > > DeviceClass *dc = DEVICE_CLASS(klass); > > @@ -290,11 +300,12 @@ static void a10_pit_class_init(ObjectClass *klass, void *data) > > } > > > > static const TypeInfo a10_pit_info = { > > - .name = TYPE_AW_A10_PIT, > > - .parent = TYPE_SYS_BUS_DEVICE, > > - .instance_size = sizeof(AwA10PITState), > > - .instance_init = a10_pit_init, > > - .class_init = a10_pit_class_init, > > + .name = TYPE_AW_A10_PIT, > > + .parent = TYPE_SYS_BUS_DEVICE, > > + .instance_size = sizeof(AwA10PITState), > > + .instance_init = a10_pit_init, > > + .instance_finalize = a10_pit_finalize, > > + .class_init = a10_pit_class_init, > > }; > > Please don't make unrelated whitespace changes like this in a patch. > We don't line up the assignments in this sort of struct -- this is > deliberate, so that if a new line is added whose field name happens > to be longer than those used already, the patch does not have to > touch all the lines in the struct to maintain the formatting. > Instead you get a readable diff where only the new line changes, > not all the others. Hmm. Having said that I see that the other 6 devices touched by this series did use the line-up-the-assignments style. Anyway, the style this device was using is the right one. thanks -- PMM
> -----Original Message----- > From: Peter Maydell [mailto:peter.maydell@linaro.org] > Sent: Tuesday, December 15, 2020 12:20 AM > To: ganqixin <ganqixin@huawei.com> > Cc: qemu-arm <qemu-arm@nongnu.org>; QEMU Developers > <qemu-devel@nongnu.org>; Chenqun (kuhn) > <kuhn.chenqun@huawei.com>; Zhanghailiang > <zhang.zhanghailiang@huawei.com>; Euler Robot > <euler.robot@huawei.com>; Beniamino Galvani <b.galvani@gmail.com> > Subject: Re: [PATCH 1/7] allwinner-a10-pit: Use ptimer_free() in the > finalize function to avoid memleaks > > On Mon, 14 Dec 2020 at 16:02, Peter Maydell <peter.maydell@linaro.org> > wrote: > > > > On Fri, 27 Nov 2020 at 07:19, Gan Qixin <ganqixin@huawei.com> wrote: > > > > > > When running device-introspect-test, a memory leak occurred in the > > > a10_pit_init function, so use ptimer_free() in the finalize function to > avoid it. > > > > > > ASAN shows memory leak stack: > > > > > > Indirect leak of 288 byte(s) in 6 object(s) allocated from: > > > #0 0xffffab97e1f0 in __interceptor_calloc > (/lib64/libasan.so.5+0xee1f0) > > > #1 0xffffab256800 in g_malloc0 (/lib64/libglib-2.0.so.0+0x56800) > > > #2 0xaaabf555db84 in timer_new_full > /qemu/include/qemu/timer.h:523 > > > #3 0xaaabf555db84 in timer_new > /qemu/include/qemu/timer.h:544 > > > #4 0xaaabf555db84 in timer_new_ns > /qemu/include/qemu/timer.h:562 > > > #5 0xaaabf555db84 in ptimer_init /qemu/hw/core/ptimer.c:433 > > > #6 0xaaabf57415e8 in a10_pit_init > /qemu/hw/timer/allwinner-a10-pit.c:278 > > > #7 0xaaabf6339f6c in object_initialize_with_type > /qemu/qom/object.c:515 > > > #8 0xaaabf633ca04 in object_initialize_child_with_propsv > /qemu/qom/object.c:564 > > > #9 0xaaabf633cc08 in object_initialize_child_with_props > /qemu/qom/object.c:547 > > > #10 0xaaabf5b94680 in aw_a10_init > /qemu/hw/arm/allwinner-a10.c:49 > > > #11 0xaaabf6339f6c in object_initialize_with_type > /qemu/qom/object.c:515 > > > #12 0xaaabf633a1e0 in object_new_with_type > > > /qemu/qom/object.c:729 > > > > > > Reported-by: Euler Robot <euler.robot@huawei.com> > > > Signed-off-by: Gan Qixin <ganqixin@huawei.com> > > > --- > > > Cc: Beniamino Galvani <b.galvani@gmail.com> > > > --- > > > hw/timer/allwinner-a10-pit.c | 21 ++++++++++++++++----- > > > 1 file changed, 16 insertions(+), 5 deletions(-) > > > > > > diff --git a/hw/timer/allwinner-a10-pit.c > > > b/hw/timer/allwinner-a10-pit.c index f84fc0ea25..be211983b0 > 100644 > > > --- a/hw/timer/allwinner-a10-pit.c > > > +++ b/hw/timer/allwinner-a10-pit.c > > > @@ -279,6 +279,16 @@ static void a10_pit_init(Object *obj) > > > } > > > } > > > > > > +static void a10_pit_finalize(Object *obj) { > > > + AwA10PITState *s = AW_A10_PIT(obj); > > > + int i; > > > + > > > + for (i = 0; i < AW_A10_PIT_TIMER_NR; i++) { > > > + ptimer_free(s->timer[i]); > > > + } > > > +} > > > + > > > static void a10_pit_class_init(ObjectClass *klass, void *data) { > > > DeviceClass *dc = DEVICE_CLASS(klass); @@ -290,11 +300,12 > @@ > > > static void a10_pit_class_init(ObjectClass *klass, void *data) } > > > > > > static const TypeInfo a10_pit_info = { > > > - .name = TYPE_AW_A10_PIT, > > > - .parent = TYPE_SYS_BUS_DEVICE, > > > - .instance_size = sizeof(AwA10PITState), > > > - .instance_init = a10_pit_init, > > > - .class_init = a10_pit_class_init, > > > + .name = TYPE_AW_A10_PIT, > > > + .parent = TYPE_SYS_BUS_DEVICE, > > > + .instance_size = sizeof(AwA10PITState), > > > + .instance_init = a10_pit_init, > > > + .instance_finalize = a10_pit_finalize, > > > + .class_init = a10_pit_class_init, > > > }; > > > > Please don't make unrelated whitespace changes like this in a patch. > > We don't line up the assignments in this sort of struct -- this is > > deliberate, so that if a new line is added whose field name happens to > > be longer than those used already, the patch does not have to touch > > all the lines in the struct to maintain the formatting. > > Instead you get a readable diff where only the new line changes, not > > all the others. > > Hmm. Having said that I see that the other 6 devices touched by this series > did use the line-up-the-assignments style. > Anyway, the style this device was using is the right one. Thanks for your reply, I will resend this patch. By the way, do we need to keep the other 6 devices in the same style as this device? Gan Qixin
On Tue, 15 Dec 2020 at 02:38, ganqixin <ganqixin@huawei.com> wrote: > > > -----Original Message----- > > From: Peter Maydell [mailto:peter.maydell@linaro.org] > > Sent: Tuesday, December 15, 2020 12:20 AM > > To: ganqixin <ganqixin@huawei.com> > > Cc: qemu-arm <qemu-arm@nongnu.org>; QEMU Developers > > <qemu-devel@nongnu.org>; Chenqun (kuhn) > > <kuhn.chenqun@huawei.com>; Zhanghailiang > > <zhang.zhanghailiang@huawei.com>; Euler Robot > > <euler.robot@huawei.com>; Beniamino Galvani <b.galvani@gmail.com> > > Subject: Re: [PATCH 1/7] allwinner-a10-pit: Use ptimer_free() in the > > finalize function to avoid memleaks > > > > On Mon, 14 Dec 2020 at 16:02, Peter Maydell <peter.maydell@linaro.org> > > wrote: > > > Please don't make unrelated whitespace changes like this in a patch. > > > We don't line up the assignments in this sort of struct -- this is > > > deliberate, so that if a new line is added whose field name happens to > > > be longer than those used already, the patch does not have to touch > > > all the lines in the struct to maintain the formatting. > > > Instead you get a readable diff where only the new line changes, not > > > all the others. > > > > Hmm. Having said that I see that the other 6 devices touched by this series > > did use the line-up-the-assignments style. > > Anyway, the style this device was using is the right one. > > Thanks for your reply, I will resend this patch. By the way, do we need to keep > the other 6 devices in the same style as this device? My suggestion would be that for all these files you just add the one new line you need to add, and leave the existing lines in the struct the way they are now (regardless of which style the file is currently using). thanks -- PMM
diff --git a/hw/timer/allwinner-a10-pit.c b/hw/timer/allwinner-a10-pit.c index f84fc0ea25..be211983b0 100644 --- a/hw/timer/allwinner-a10-pit.c +++ b/hw/timer/allwinner-a10-pit.c @@ -279,6 +279,16 @@ static void a10_pit_init(Object *obj) } } +static void a10_pit_finalize(Object *obj) +{ + AwA10PITState *s = AW_A10_PIT(obj); + int i; + + for (i = 0; i < AW_A10_PIT_TIMER_NR; i++) { + ptimer_free(s->timer[i]); + } +} + static void a10_pit_class_init(ObjectClass *klass, void *data) { DeviceClass *dc = DEVICE_CLASS(klass); @@ -290,11 +300,12 @@ static void a10_pit_class_init(ObjectClass *klass, void *data) } static const TypeInfo a10_pit_info = { - .name = TYPE_AW_A10_PIT, - .parent = TYPE_SYS_BUS_DEVICE, - .instance_size = sizeof(AwA10PITState), - .instance_init = a10_pit_init, - .class_init = a10_pit_class_init, + .name = TYPE_AW_A10_PIT, + .parent = TYPE_SYS_BUS_DEVICE, + .instance_size = sizeof(AwA10PITState), + .instance_init = a10_pit_init, + .instance_finalize = a10_pit_finalize, + .class_init = a10_pit_class_init, }; static void a10_register_types(void)
When running device-introspect-test, a memory leak occurred in the a10_pit_init function, so use ptimer_free() in the finalize function to avoid it. ASAN shows memory leak stack: Indirect leak of 288 byte(s) in 6 object(s) allocated from: #0 0xffffab97e1f0 in __interceptor_calloc (/lib64/libasan.so.5+0xee1f0) #1 0xffffab256800 in g_malloc0 (/lib64/libglib-2.0.so.0+0x56800) #2 0xaaabf555db84 in timer_new_full /qemu/include/qemu/timer.h:523 #3 0xaaabf555db84 in timer_new /qemu/include/qemu/timer.h:544 #4 0xaaabf555db84 in timer_new_ns /qemu/include/qemu/timer.h:562 #5 0xaaabf555db84 in ptimer_init /qemu/hw/core/ptimer.c:433 #6 0xaaabf57415e8 in a10_pit_init /qemu/hw/timer/allwinner-a10-pit.c:278 #7 0xaaabf6339f6c in object_initialize_with_type /qemu/qom/object.c:515 #8 0xaaabf633ca04 in object_initialize_child_with_propsv /qemu/qom/object.c:564 #9 0xaaabf633cc08 in object_initialize_child_with_props /qemu/qom/object.c:547 #10 0xaaabf5b94680 in aw_a10_init /qemu/hw/arm/allwinner-a10.c:49 #11 0xaaabf6339f6c in object_initialize_with_type /qemu/qom/object.c:515 #12 0xaaabf633a1e0 in object_new_with_type /qemu/qom/object.c:729 Reported-by: Euler Robot <euler.robot@huawei.com> Signed-off-by: Gan Qixin <ganqixin@huawei.com> --- Cc: Beniamino Galvani <b.galvani@gmail.com> --- hw/timer/allwinner-a10-pit.c | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-)