On Wed, Dec 16, 2020 at 11:53:06AM +0400, Marc-André Lureau wrote:
> Hi
>
> On Wed, Dec 16, 2020 at 2:41 AM Eduardo Habkost <ehabk...@redhat.com> wrote:
>
> > QOM reference counting bugs are often hard to detect, but there's
> > one kind of bug that's easier: if we are freeing an object but is
> > still attached to a parent, it means the reference count is wrong
> > (because the parent always hold a reference to their children).
> >
> > Add an assertion to make sure we detect those cases.
> >
> > Signed-off-by: Eduardo Habkost <ehabk...@redhat.com>
> >
>
> On the principle, I fully agree. But the risk is high to introduce
> regression if objects are manipulated in strange ways.
Isn't the point that we're broken already. We have a QOM instance
in the tree which has a zero reference count and has been freed.
As soon as something touches that object in the tree, we're liable
to crash & burn touching free'd memory. So it seems the choices are
between crash fast where we see the problem, or crash eventually
at a place where we can't easily trace back to the root cause.
> I remember I wanted object_unref() to automatically remove itself from the
> parent when the last ref is dropped. I think there were similar concerns.
Automatically removing itself would be hiding the bug in whatever
code has mistakenly removed a reference it didn't own.
>
> Maybe with --enable-qom-debug ? (removing the -cast)
>
> ---
> > qom/object.c | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/qom/object.c b/qom/object.c
> > index f2ae6e6b2a..5cfed6d7c6 100644
> > --- a/qom/object.c
> > +++ b/qom/object.c
> > @@ -685,6 +685,7 @@ static void object_finalize(void *data)
> > object_deinit(obj, ti);
> >
> > g_assert(obj->ref == 0);
> > + g_assert(obj->parent == NULL);
> > if (obj->free) {
> > obj->free(obj);
> > }
> > --
> > 2.28.0
> >
> >
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
