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

[Peter Maydell]

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