| Message ID | 20200227025055.14341-6-pannengyuan@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | delay timer_new from init to realize to fix memleaks. | expand |
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
On 02/03/2020 13:21, Peter Maydell wrote: > 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. 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.
On 3/3/2020 3:17 AM, Mark Cave-Ayland wrote: > On 02/03/2020 13:21, Peter Maydell wrote: > >> 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. > > 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. Hmm, maybe you can take this other changes(fix memleaks) into your local patches and send it together? Or If you have no time, I can help to do it about this device. :) > >> 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. > . >
On 03/03/2020 01:36, Pan Nengyuan wrote: > On 3/3/2020 3:17 AM, Mark Cave-Ayland wrote: >> On 02/03/2020 13:21, Peter Maydell wrote: >> >>> 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. >> >> 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. > > Hmm, maybe you can take this other changes(fix memleaks) into your local patches and send it together? > Or If you have no time, I can help to do it about this device. :) My patches are part of various q800 branches I have been playing with over the past couple of months, and so are lagging quite far behind master. If you are able to update them based upon Peter's comments them I'm happy to review them (and perhaps could perhaps take them along with my cmd646 patchset if required). ATB, Mark.
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), diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c index 19e154b870..c1cd154a84 100644 --- a/hw/misc/mos6522.c +++ b/hw/misc/mos6522.c @@ -485,6 +485,11 @@ static void mos6522_init(Object *obj) for (i = 0; i < ARRAY_SIZE(s->timers); i++) { s->timers[i].index = i; } +} + +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;
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. --- hw/misc/mac_via.c | 5 +++++ hw/misc/mos6522.c | 6 ++++++ 2 files changed, 11 insertions(+)