On Thu, 27 Feb 2020 at 02:35, Pan Nengyuan <pannengy...@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.ro...@huawei.com> > Signed-off-by: Pan Nengyuan <pannengy...@huawei.com> > --- > Cc: Laurent Vivier <laur...@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