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

[Mark Cave-Ayland]

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 <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.
> 
> 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.