[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v3 5/6] hw/misc/mos6522: move timer_new from init() into real
From: |
Mark Cave-Ayland |
Subject: |
Re: [PATCH v3 5/6] hw/misc/mos6522: move timer_new from init() into realize() to avoid memleaks |
Date: |
Mon, 2 Mar 2020 19:17:05 +0000 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.5.0 |
On 02/03/2020 13:21, Peter Maydell wrote:
> On Thu, 27 Feb 2020 at 02:35, Pan Nengyuan <address@hidden> 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 <address@hidden>
>> Signed-off-by: Pan Nengyuan <address@hidden>
>> ---
>> Cc: Laurent Vivier <address@hidden>
>> ---
>> 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.