Re: [PATCH 2/2] qom: Assert that objects being destroyed have no parent

[Daniel P . Berrangé]

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 <ehabkost@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 <ehabkost@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 :|