[1.3,1/5] qom: fix refcount of non-heap-allocated objects

The reference count for embedded objects is always one too low, because
object_initialize_with_type returns with zero references to the object.
This causes premature finalization of the object (or an assertion failure)
after calling object_ref to add an extra reference and object_unref to
remove it.

The fix is to move the initial object_ref call from object_new_with_type
to object_initialize_with_type.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 qom/object.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Am 23.11.2012 09:47, schrieb Paolo Bonzini:
> The reference count for embedded objects is always one too low, because
> object_initialize_with_type returns with zero references to the object.
> This causes premature finalization of the object (or an assertion failure)
> after calling object_ref to add an extra reference and object_unref to
> remove it.
> 
> The fix is to move the initial object_ref call from object_new_with_type
> to object_initialize_with_type.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Acked-by: Andreas Färber <afaerber@suse.de>

Andreas

Paolo Bonzini <pbonzini@redhat.com> writes:

> The reference count for embedded objects is always one too low, because
> object_initialize_with_type returns with zero references to the object.
> This causes premature finalization of the object (or an assertion failure)
> after calling object_ref to add an extra reference and object_unref to
> remove it.
>
> The fix is to move the initial object_ref call from object_new_with_type
> to object_initialize_with_type.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  qom/object.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/qom/object.c b/qom/object.c
> index d7092b0..6a8c02a 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -307,6 +307,7 @@ void object_initialize_with_type(void *data, TypeImpl *type)
>  
>      memset(obj, 0, type->instance_size);
>      obj->class = type->class;
> +    object_ref(obj);
>      QTAILQ_INIT(&obj->properties);
>      object_init_with_type(obj, type);
>  }

But object_property_add_child() will take a reference.

When the parent object goes away, this will cause that reference to get
dropped and ultimately the child object to be destroyed.

IOW, this change causes embedded objects to get leaked AFAICT.

Regards,

Anthony Liguori

> @@ -395,7 +396,6 @@ Object *object_new_with_type(Type type)
>  
>      obj = g_malloc(type->instance_size);
>      object_initialize_with_type(obj, type);
> -    object_ref(obj);
>  
>      return obj;
>  }
> -- 
> 1.8.0

Il 26/11/2012 16:49, Anthony Liguori ha scritto:
> But object_property_add_child() will take a reference.
> When the parent object goes away, this will cause that reference to get
> dropped and ultimately the child object to be destroyed.

This is still wrong if you have:

  object_init(&obj->subobj, ...)
  foo(&obj->subobj);
  object_property_add_child()

where foo() does a ref+unref pair (see the pattern of scsi_req_enqueue
for example, even though it's not QOM).

The object's memory area is being kept live by the parent, so it
logically has a nonzero reference count.

We don't have any example of embedding yet in the tree, except for
buses, so I think we are somewhat free to set the rules.

As I said elsewhere in the thread, the rule I'd prefer the most is "it
doesn't matter whether an object is statically- or
dynamically-allocated".  That is, even for an embedded object which you
create in instance_init, you should object_unref it explicitly, either
in instance_finalize or after initializing it (as you prefer).

Note that instance_finalize will anyway have an object_unparent of the
embedded object, in order to remove any cyclical references, so it's not
a big change.

> IOW, this change causes embedded objects to get leaked AFAICT.

Hmm, patch 4 in the series offsets this by doing

 void qbus_free(BusState *bus)
 {
-    if (bus->qom_allocated) {
-        object_delete(OBJECT(bus));
-    } else {
-        object_finalize(OBJECT(bus));
-        if (bus->glib_allocated) {
-            g_free(bus);
-        }
-    }
+    object_delete(OBJECT(bus));
 }

So at the end of the series there is no leak.

Paolo