Re: [PATCH v3 5/6] hw/misc/mos6522: move timer_new from init() into realize() to avoid memleaks

[Peter Maydell <peter.maydell@linaro.org>]

On Thu, 27 Feb 2020 at 02:35, Pan Nengyuan <pannengyuan@huawei.com> wrote:
>
> There are some memleaks when we call 'device_list_properties'. This patch move timer_new from init into realize to fix it.
> Meanwhile, add calls to mos6522_realize() in mac_via_realize to make this move to be valid.
>
> Reported-by: Euler Robot <euler.robot@huawei.com>
> Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com>
> ---
> Cc: Laurent Vivier <laurent@vivier.eu>
> ---
> v2->v1:
> - no changes in this patch.
> v3->v2:
> - remove null check in reset, and add calls to mos6522_realize() in mac_via_realize to make this move to be valid.

Hi; this is really fixing two bugs in one patch:

> ---
>  hw/misc/mac_via.c | 5 +++++
>  hw/misc/mos6522.c | 6 ++++++
>  2 files changed, 11 insertions(+)
>
> diff --git a/hw/misc/mac_via.c b/hw/misc/mac_via.c
> index b7d0012794..1d72d4ef35 100644
> --- a/hw/misc/mac_via.c
> +++ b/hw/misc/mac_via.c
> @@ -879,6 +879,11 @@ static void mac_via_realize(DeviceState *dev, Error **errp)
>      sysbus_init_child_obj(OBJECT(dev), "via2", &m->mos6522_via2,
>                            sizeof(m->mos6522_via2), TYPE_MOS6522_Q800_VIA2);
>
> +    object_property_set_bool(OBJECT(&m->mos6522_via1), true, "realized",
> +                             &error_abort);
> +    object_property_set_bool(OBJECT(&m->mos6522_via2), true, "realized",
> +                             &error_abort);
> +
>      /* Pass through mos6522 output IRQs */
>      ms = MOS6522(&m->mos6522_via1);
>      object_property_add_alias(OBJECT(dev), "irq[0]", OBJECT(ms),

This is fixing a bug in mac_via where it failed to actually
realize devices it was using. That's a dependency for the bug
you're trying to fix, but it's a separate one and should be
in its own patch.

You also want to pass the error back up to the caller, rather
than using error_abort. To do that, at the top of the function:

    Error *err = NULL;

and then here you can do:
    object_property_set_bool(OBJECT(&m->mos6522_via1), true, "realized", &err);
    if (err) {
        error_propagate(errp, err);
        return;
    }

The existing code which inits the mos6522 objects and
calls object_property_add_alias on them which is in
the mac_via realize function should be moved to the init
function. (init should init child objects and set up
properties; realize should realize them.)

This is all fixing the incorrect creation of the mos6522
devices in mac_via (which has been wrong since the mac_via
was first added in commit 6dca62a0000f9), so that can all
be one patch I think.

> +static void mos6522_realize(DeviceState *dev, Error **errp)
> +{
> +    MOS6522State *s = MOS6522(dev);
>
>      s->timers[0].timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, mos6522_timer1, s);
>      s->timers[1].timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, mos6522_timer2, s);
> @@ -502,6 +507,7 @@ static void mos6522_class_init(ObjectClass *oc, void *data)
>
>      dc->reset = mos6522_reset;
>      dc->vmsd = &vmstate_mos6522;
> +    dc->realize = mos6522_realize;
>      device_class_set_props(dc, mos6522_properties);
>      mdc->parent_reset = dc->reset;
>      mdc->set_sr_int = mos6522_set_sr_int;

The changes to hw/misc/mos_6522.c are good, but should be in
their own patch.

thanks
-- PMM