Re: [PATCH 2/3] softmmu/ioport.c: QOMify MemoryRegionPortioList

[Paolo Bonzini]

On 5/11/23 16:52, Mark Cave-Ayland wrote:
> > >       /* Copy the sub-list and null-terminate it.  */
> > > -    mrpio = g_malloc0(sizeof(MemoryRegionPortioList));
> > > +    mrpio = MEMORY_REGION_PORTIO_LIST(
> > > +                object_new(TYPE_MEMORY_REGION_PORTIO_LIST));
> >
> > Shouldn't we need to replace the g_free() call by object_unref()
> > in portio_list_destroy()?
>
> Both the existing g_free() call and replacing it with object_unref() 
> cause QEMU to segfault if you call portio_list_destroy() before the 
> final patch in this series. So in effect we'd end up causing more code 
> churn just to replace one crash with another 😉
>
> The real fix is in patch 3 which alters the refcounting/ownership in 
> order to solve the underlying issue, which I hope I have described in 
> enough detail in that commit message.

Here I agree with Philippe though, there is a mismatch between new and 
unref that would (for example) cause the finalize method not to be called.

I think this patch should already introduce the instance_finalize 
function that only does "g_free(mrpio->ports);", and include the full 
portio_list_destroy() hunk from patch 3.

Then patch 3 basically focuses on the reparenting trick (including 
adding the object_unref() call in memory_region_portio_list_finalize).

I can do the change myself when applying though.

Paolo