On 02/03/2020 13:21, Peter Maydell wrote: > 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.
Sigh. Thanks for this - I actually discovered this a little while back and have some local patches to do the same, but due to lack of time I never managed to tidy them up for submission. > 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.) And I believe at some point I had a patch lying around to do this too... > 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. ATB, Mark.