RE: [PATCH 1/7] allwinner-a10-pit: Use ptimer_free() in the finalize function to avoid memleaks

[ganqixin]

> -----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