[Qemu-devel] [PATCH 1.3 0/5] QOM/qdev lifetime fixes

[Qemu-devel] [PATCH 1.3 0/5] QOM/qdev lifetime fixes

[Paolo Bonzini]

These patches fix problems in the handling of freeing QOM/qdev
objects.  Together, they fix hot-unplug of USB mass storage devices,
which crashed with an assertion failure.

I'm not 100% sure, but I think we were always leaking the scsi-disk in
pre-QOM days.  Now we're freeing it properly, and the assertion proves it.

However, I don't like particularly the assertion in object_delete.  Once
we're sure we've fixed all bugs, we should remove it, because it prevents
a fully correct tracking of references.

In this case, for example, there is still one reference to the scsi-disk
in the MSDState's scsi_dev member.  We don't have neither an object_ref
nor an object_unref for it, so it happens to work.  If we had an
object_ref, the matching object_unref would be in dc->exit.  But then
we'd trip on the assertion failure again, because the SCSI bus is removed
(thus calling qdev_free on the scsi-dev) before dc->exit is called.

I have more patches to actually make the reference count of devices
and buses fully correct, but they are even more scary than these, so
they should wait for 1.4.

Paolo Bonzini (5):
  qom: fix refcount of non-heap-allocated objects
  qdev: move bus removal to object_unparent
  qom: make object_delete usable for statically-allocated objects
  qdev: simplify (de)allocation of buses
  qom: make object_finalize static

 hw/qdev-core.h        |  5 -----
 hw/qdev.c             | 26 ++++++++++++++------------
 hw/pci.c              |  2 +-
 hw/sysbus.c           |  2 +-
 include/qemu/object.h | 29 ++++++++++++++++++++---------
 qom/object.c          | 12 +++++++++---
 6 files changed, 45 insertions(+), 31 deletions(-)

-- 
1.8.0

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

[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>
---
 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);
 }
@@ -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

Re: [Qemu-devel] [PATCH 1.3 1/5] qom: fix refcount of non-heap-allocated objects

[Andreas Färber]

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

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH 1.3 1/5] qom: fix refcount of non-heap-allocated objects

[Anthony Liguori]

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

Re: [Qemu-devel] [PATCH 1.3 1/5] qom: fix refcount of non-heap-allocated objects

[Paolo Bonzini]

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

[Qemu-devel] [PATCH 1.3 2/5] qdev: move bus removal to object_unparent

[Paolo Bonzini]

Add an ObjectClass method that is done at object_unparent time.  It
should remove any backlinks to the object in the composition tree,
so that object_delete will be able to drop the last reference and
free the object.

Use it for qdev buses.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
        Ping Fan, with respect to your patch at
        http://permalink.gmane.org/gmane.comp.emulators.qemu/179687,
        this one removes the need for qdev_unset_parent.  You
        can simply call object_unparent.

 hw/qdev.c             | 16 +++++++++++++---
 include/qemu/object.h | 11 +++++++++++
 qom/object.c          |  3 +++
 3 files changed, 27 insertions(+), 3 deletions(-)

diff --git a/hw/qdev.c b/hw/qdev.c
index 7ddcd24..f43717b 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -705,9 +705,6 @@ static void device_finalize(Object *obj)
             qemu_opts_del(dev->opts);
         }
     }
-    if (dev->parent_bus) {
-        bus_remove_child(dev->parent_bus, dev);
-    }
 }
 
 static void device_class_base_init(ObjectClass *class, void *data)
@@ -720,6 +717,18 @@ static void device_class_base_init(ObjectClass *class, void *data)
     klass->props = NULL;
 }
 
+static void qdev_remove_from_bus(Object *obj)
+{
+    DeviceState *dev = DEVICE(obj);
+
+    bus_remove_child(dev->parent_bus, dev);
+}
+
+static void device_class_init(ObjectClass *class, void *data)
+{
+    class->unparent = qdev_remove_from_bus;
+}
+
 void device_reset(DeviceState *dev)
 {
     DeviceClass *klass = DEVICE_GET_CLASS(dev);
@@ -747,6 +756,7 @@ static TypeInfo device_type_info = {
     .instance_init = device_initfn,
     .instance_finalize = device_finalize,
     .class_base_init = device_class_base_init,
+    .class_init = device_class_init,
     .abstract = true,
     .class_size = sizeof(DeviceClass),
 };
diff --git a/include/qemu/object.h b/include/qemu/object.h
index be707f1..232463b 100644
--- a/include/qemu/object.h
+++ b/include/qemu/object.h
@@ -230,6 +230,15 @@ typedef struct ObjectProperty
 } ObjectProperty;
 
 /**
+ * ObjectUnparent:
+ * @obj: the object that is being removed from the composition tree
+ *
+ * Called when an object is being removed from the QOM composition tree.
+ * The function should remove any backlinks from children objects to @obj.
+ */
+typedef void (ObjectUnparent)(Object *obj);
+
+/**
  * ObjectClass:
  *
  * The base for all classes.  The only thing that #ObjectClass contains is an
@@ -240,6 +249,8 @@ struct ObjectClass
     /*< private >*/
     Type type;
     GSList *interfaces;
+
+    ObjectUnparent *unparent;
 };
 
 /**
diff --git a/qom/object.c b/qom/object.c
index 6a8c02a..f4747d0 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -363,6 +363,9 @@ void object_unparent(Object *obj)
     if (obj->parent) {
         object_property_del_child(obj->parent, obj, NULL);
     }
+    if (obj->class->unparent) {
+        (obj->class->unparent)(obj);
+    }
 }
 
 static void object_deinit(Object *obj, TypeImpl *type)
-- 
1.8.0

Re: [Qemu-devel] [PATCH 1.3 2/5] qdev: move bus removal to object_unparent

[Andreas Färber]

Am 23.11.2012 09:47, schrieb Paolo Bonzini:
> Add an ObjectClass method that is done at object_unparent time.  It
> should remove any backlinks to the object in the composition tree,
> so that object_delete will be able to drop the last reference and
> free the object.
> 
> Use it for qdev buses.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

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

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH 1.3 2/5] qdev: move bus removal to object_unparent

[Andreas Färber]

Am 23.11.2012 09:47, schrieb Paolo Bonzini:
> diff --git a/hw/qdev.c b/hw/qdev.c
> index 7ddcd24..f43717b 100644
> --- a/hw/qdev.c
> +++ b/hw/qdev.c
[...]
> @@ -720,6 +717,18 @@ static void device_class_base_init(ObjectClass *class, void *data)
>      klass->props = NULL;
>  }
>  
> +static void qdev_remove_from_bus(Object *obj)
> +{
> +    DeviceState *dev = DEVICE(obj);
> +
> +    bus_remove_child(dev->parent_bus, dev);
> +}
> +
> +static void device_class_init(ObjectClass *class, void *data)
> +{
> +    class->unparent = qdev_remove_from_bus;

Ouch, patch for 1.4 coming up. :)

Andreas

> +}
> +
>  void device_reset(DeviceState *dev)
>  {
>      DeviceClass *klass = DEVICE_GET_CLASS(dev);
> @@ -747,6 +756,7 @@ static TypeInfo device_type_info = {
>      .instance_init = device_initfn,
>      .instance_finalize = device_finalize,
>      .class_base_init = device_class_base_init,
> +    .class_init = device_class_init,
>      .abstract = true,
>      .class_size = sizeof(DeviceClass),
>  };

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

[Qemu-devel] [PATCH 1.3 3/5] qom: make object_delete usable for statically-allocated objects

[Paolo Bonzini]

Store in the object the freeing function that will be used at deletion
time.  This makes it possible to use object_delete on statically-allocated
(embedded) objects.  Dually, it makes it possible to use object_unparent
and object_unref without leaking memory, when the lifetime of object
might extend until after the call to object_delete.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
        Ping Fan, this is the patch I mentioned at
        http://permalink.gmane.org/gmane.comp.emulators.qemu/180054.
        With this patch, your call of object_unref for qdev_unplug_complete
        will not leak the device anymore (object_finalize will free it
        when the last reference is dropped with object_unref).

 include/qemu/object.h | 9 +++++++++
 qom/object.c          | 5 ++++-
 2 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/include/qemu/object.h b/include/qemu/object.h
index 232463b..5ddcb4a 100644
--- a/include/qemu/object.h
+++ b/include/qemu/object.h
@@ -239,6 +239,14 @@ typedef struct ObjectProperty
 typedef void (ObjectUnparent)(Object *obj);
 
 /**
+ * ObjectFree:
+ * @obj: the object being freed
+ *
+ * Called when an object's last reference is removed.
+ */
+typedef void (ObjectFree)(void *obj);
+
+/**
  * ObjectClass:
  *
  * The base for all classes.  The only thing that #ObjectClass contains is an
@@ -272,6 +280,7 @@ struct Object
 {
     /*< private >*/
     ObjectClass *class;
+    ObjectFree *free;
     QTAILQ_HEAD(, ObjectProperty) properties;
     uint32_t ref;
     Object *parent;
diff --git a/qom/object.c b/qom/object.c
index f4747d0..f3e9517 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -388,6 +388,9 @@ void object_finalize(void *data)
     object_property_del_all(obj);
 
     g_assert(obj->ref == 0);
+    if (obj->free) {
+        obj->free(obj);
+    }
 }
 
 Object *object_new_with_type(Type type)
@@ -399,6 +402,7 @@ Object *object_new_with_type(Type type)
 
     obj = g_malloc(type->instance_size);
     object_initialize_with_type(obj, type);
+    obj->free = g_free;
 
     return obj;
 }
@@ -415,7 +419,6 @@ void object_delete(Object *obj)
     object_unparent(obj);
     g_assert(obj->ref == 1);
     object_unref(obj);
-    g_free(obj);
 }
 
 Object *object_dynamic_cast(Object *obj, const char *typename)
-- 
1.8.0

Re: [Qemu-devel] [PATCH 1.3 3/5] qom: make object_delete usable for statically-allocated objects

[Andreas Färber]

Am 23.11.2012 09:47, schrieb Paolo Bonzini:
> Store in the object the freeing function that will be used at deletion
> time.  This makes it possible to use object_delete on statically-allocated
> (embedded) objects.  Dually, it makes it possible to use object_unparent
> and object_unref without leaking memory, when the lifetime of object
> might extend until after the call to object_delete.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

The code is

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

however I do not agree with the goal in the subject. I thought this was
to match C++ in actually deallocating the memory.

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH 1.3 3/5] qom: make object_delete usable for statically-allocated objects

[Paolo Bonzini]

Il 23/11/2012 18:02, Andreas Färber ha scritto:
>> Store in the object the freeing function that will be used at deletion
>> > time.  This makes it possible to use object_delete on statically-allocated
>> > (embedded) objects.  Dually, it makes it possible to use object_unparent
>> > and object_unref without leaking memory, when the lifetime of object
>> > might extend until after the call to object_delete.
>> > 
>> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> The code is
> 
> Reviewed-by: Andreas Färber <afaerber@suse.de>
> 
> however I do not agree with the goal in the subject. I thought this was
> to match C++ in actually deallocating the memory.

At the "real" end of the series object_delete disappears completely.  I
posted only the initial part because the rest is not appropriate for
1.3, you can see it at https://github.com/bonzini/qemu/commits/qdev-free.

You just have ref/unref to keep an object alive, and unparent to remove it.

qbus_free and qdev_free become simply synonyms for object_unparent.
After the unparent, the refcount is magically zero for the bus/device
and everything below it in the qtree, and they disappear.

Paolo

[Qemu-devel] [PATCH 1.3 4/5] qdev: simplify (de)allocation of buses

[Paolo Bonzini]

All conditional deallocation can now be done with object_delete.
Remove the @qom_allocated and @glib_allocated fields; replace the latter
with a direct assignment of the @free function pointer.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/pci.c       |  2 +-
 hw/qdev-core.h |  5 -----
 hw/qdev.c      | 10 +---------
 hw/sysbus.c    |  2 +-
 4 files changed, 3 insertions(+), 16 deletions(-)

diff --git a/hw/pci.c b/hw/pci.c
index 9841e39..97a0cd7 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -301,9 +301,9 @@ PCIBus *pci_bus_new(DeviceState *parent, const char *name,
     PCIBus *bus;
 
     bus = g_malloc0(sizeof(*bus));
-    bus->qbus.glib_allocated = true;
     pci_bus_new_inplace(bus, parent, name, address_space_mem,
                         address_space_io, devfn_min);
+    OBJECT(bus)->free = g_free;
     return bus;
 }
 
diff --git a/hw/qdev-core.h b/hw/qdev-core.h
index fce9e22..fff7f0f 100644
--- a/hw/qdev-core.h
+++ b/hw/qdev-core.h
@@ -106,17 +106,12 @@ typedef struct BusChild {
 
 /**
  * BusState:
- * @qom_allocated: Indicates whether the object was allocated by QOM.
- * @glib_allocated: Indicates whether the object was initialized in-place
- * yet is expected to be freed with g_free().
  */
 struct BusState {
     Object obj;
     DeviceState *parent;
     const char *name;
     int allow_hotplug;
-    bool qom_allocated;
-    bool glib_allocated;
     int max_index;
     QTAILQ_HEAD(ChildrenHead, BusChild) children;
     QLIST_ENTRY(BusState) sibling;
diff --git a/hw/qdev.c b/hw/qdev.c
index f43717b..788b4da 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -454,7 +454,6 @@ BusState *qbus_create(const char *typename, DeviceState *parent, const char *nam
     BusState *bus;
 
     bus = BUS(object_new(typename));
-    bus->qom_allocated = true;
 
     bus->parent = parent;
     bus->name = name ? g_strdup(name) : NULL;
@@ -465,14 +464,7 @@ BusState *qbus_create(const char *typename, DeviceState *parent, const char *nam
 
 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));
 }
 
 static char *bus_get_fw_dev_path(BusState *bus, DeviceState *dev)
diff --git a/hw/sysbus.c b/hw/sysbus.c
index 4969f06..ef8ffb6 100644
--- a/hw/sysbus.c
+++ b/hw/sysbus.c
@@ -274,7 +274,7 @@ static void main_system_bus_create(void)
     main_system_bus = g_malloc0(system_bus_info.instance_size);
     qbus_create_inplace(main_system_bus, TYPE_SYSTEM_BUS, NULL,
                         "main-system-bus");
-    main_system_bus->glib_allocated = true;
+    OBJECT(main_system_bus)->free = g_free;
     object_property_add_child(container_get(qdev_get_machine(),
                                             "/unattached"),
                               "sysbus", OBJECT(main_system_bus), NULL);
-- 
1.8.0

Re: [Qemu-devel] [PATCH 1.3 4/5] qdev: simplify (de)allocation of buses

[Andreas Färber]

Am 23.11.2012 09:47, schrieb Paolo Bonzini:
> All conditional deallocation can now be done with object_delete.
> Remove the @qom_allocated and @glib_allocated fields; replace the latter
> with a direct assignment of the @free function pointer.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

I am grateful for your suggestion to clean up this mess! However...

> ---
>  hw/pci.c       |  2 +-
>  hw/qdev-core.h |  5 -----
>  hw/qdev.c      | 10 +---------
>  hw/sysbus.c    |  2 +-
>  4 files changed, 3 insertions(+), 16 deletions(-)
> 
> diff --git a/hw/pci.c b/hw/pci.c
> index 9841e39..97a0cd7 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -301,9 +301,9 @@ PCIBus *pci_bus_new(DeviceState *parent, const char *name,
>      PCIBus *bus;
>  
>      bus = g_malloc0(sizeof(*bus));
> -    bus->qbus.glib_allocated = true;
>      pci_bus_new_inplace(bus, parent, name, address_space_mem,
>                          address_space_io, devfn_min);
> +    OBJECT(bus)->free = g_free;

Anthony asked not to use the macros like this, but in this case to have
Object *obj; to assign it after g_malloc0() and use obj->free = g_free;
here.

>      return bus;
>  }
>  
> diff --git a/hw/qdev-core.h b/hw/qdev-core.h
> index fce9e22..fff7f0f 100644
> --- a/hw/qdev-core.h
> +++ b/hw/qdev-core.h
> @@ -106,17 +106,12 @@ typedef struct BusChild {
>  
>  /**
>   * BusState:
> - * @qom_allocated: Indicates whether the object was allocated by QOM.
> - * @glib_allocated: Indicates whether the object was initialized in-place
> - * yet is expected to be freed with g_free().
>   */
>  struct BusState {
>      Object obj;
>      DeviceState *parent;
>      const char *name;
>      int allow_hotplug;
> -    bool qom_allocated;
> -    bool glib_allocated;
>      int max_index;
>      QTAILQ_HEAD(ChildrenHead, BusChild) children;
>      QLIST_ENTRY(BusState) sibling;
> diff --git a/hw/qdev.c b/hw/qdev.c
> index f43717b..788b4da 100644
> --- a/hw/qdev.c
> +++ b/hw/qdev.c
> @@ -454,7 +454,6 @@ BusState *qbus_create(const char *typename, DeviceState *parent, const char *nam
>      BusState *bus;
>  
>      bus = BUS(object_new(typename));
> -    bus->qom_allocated = true;
>  
>      bus->parent = parent;
>      bus->name = name ? g_strdup(name) : NULL;
> @@ -465,14 +464,7 @@ BusState *qbus_create(const char *typename, DeviceState *parent, const char *nam
>  
>  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));

In this case ("free") I have no objection to using "delete" for
simplification. ;)

>  }
>  
>  static char *bus_get_fw_dev_path(BusState *bus, DeviceState *dev)
> diff --git a/hw/sysbus.c b/hw/sysbus.c
> index 4969f06..ef8ffb6 100644
> --- a/hw/sysbus.c
> +++ b/hw/sysbus.c
> @@ -274,7 +274,7 @@ static void main_system_bus_create(void)
>      main_system_bus = g_malloc0(system_bus_info.instance_size);
>      qbus_create_inplace(main_system_bus, TYPE_SYSTEM_BUS, NULL,
>                          "main-system-bus");
> -    main_system_bus->glib_allocated = true;
> +    OBJECT(main_system_bus)->free = g_free;

Same here.

>      object_property_add_child(container_get(qdev_get_machine(),
>                                              "/unattached"),
>                                "sysbus", OBJECT(main_system_bus), NULL);
> 

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH 1.3 4/5] qdev: simplify (de)allocation of buses

[Paolo Bonzini]

Il 23/11/2012 18:07, Andreas Färber ha scritto:
>> >      bus = g_malloc0(sizeof(*bus));
>> > -    bus->qbus.glib_allocated = true;
>> >      pci_bus_new_inplace(bus, parent, name, address_space_mem,
>> >                          address_space_io, devfn_min);
>> > +    OBJECT(bus)->free = g_free;
> Anthony asked not to use the macros like this, but in this case to have
> Object *obj; to assign it after g_malloc0() and use obj->free = g_free;
> here.

All of the alternatives look ugly:

    /* malloc mismatch, ouch */
    obj = g_malloc0(sizeof(PCIBus));
    bus = PCI_BUS(obj);

    /* Cast to superclass? */
    bus = g_malloc0(sizeof(*bus));
    obj = OBJECT(bus);

    /* Huge path to access the member */
    bus->qbus.obj->free = g_free;

The right solution would be to have qbus_init:

    void qbus_init(BusState *bus,
                   DeviceState *parent, const char *name)
    {
        object_initialize(bus, typename);

        bus->parent = parent;
        bus->name = name ? g_strdup(name) : NULL;
        qbus_realize(bus);
    }

    void qbus_create_inplace(BusState *bus, const char *typename,
                             DeviceState *parent, const char *name)
    {
        object_initialize(bus, typename);
        qbus_init(bus, parent, name);
    }

and just use object_new + qbus_init.  But it is more invasive, I'd
prefer the least pleasant but smaller alternative now.

Paolo

[Qemu-devel] [PATCH 1.3 5/5] qom: make object_finalize static

[Paolo Bonzini]

It is not used anymore, and there is no need to make it public.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 include/qemu/object.h | 9 ---------
 qom/object.c          | 2 +-
 2 files changed, 1 insertion(+), 10 deletions(-)

diff --git a/include/qemu/object.h b/include/qemu/object.h
index 5ddcb4a..ed1f47f 100644
--- a/include/qemu/object.h
+++ b/include/qemu/object.h
@@ -505,15 +505,6 @@ void object_initialize_with_type(void *data, Type type);
 void object_initialize(void *obj, const char *typename);
 
 /**
- * object_finalize:
- * @obj: The object to finalize.
- *
- * This function destroys and object without freeing the memory associated with
- * it.
- */
-void object_finalize(void *obj);
-
-/**
  * object_dynamic_cast:
  * @obj: The object to cast.
  * @typename: The @typename to cast to.
diff --git a/qom/object.c b/qom/object.c
index f3e9517..70354fc 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -379,7 +379,7 @@ static void object_deinit(Object *obj, TypeImpl *type)
     }
 }
 
-void object_finalize(void *data)
+static void object_finalize(void *data)
 {
     Object *obj = data;
     TypeImpl *ti = obj->class->type;
-- 
1.8.0

Re: [Qemu-devel] [PATCH 1.3 5/5] qom: make object_finalize static

[Andreas Färber]

Am 23.11.2012 09:47, schrieb Paolo Bonzini:
> It is not used anymore, and there is no need to make it public.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

We have not yet started using the in-place mechanism much (i440fx and
prep_pci patches, not in master; also my tegra branch), so I would like
to keep this for symmetry with "initialize" vs. "new" and their
distinguished semantics. It is not a bugfix anyway, so not needed for 1.3.

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH 1.3 5/5] qom: make object_finalize static

[Paolo Bonzini]

Il 23/11/2012 18:12, Andreas Färber ha scritto:
> We have not yet started using the in-place mechanism much (i440fx and
> prep_pci patches, not in master; also my tegra branch), so I would like
> to keep this for symmetry with "initialize" vs. "new" and their
> distinguished semantics.

But you do not need it anymore.  When the last reference disappears,
everything is finalized.  So you just use ref/unref.  Delete is simply
unparent+unref, and applies just as well to objects that were
initialized in place.

The point of patch 3 is to get rid of the distinction between initialize
and new.  That distinction just shouldn't be there if you have reference
counting.

> It is not a bugfix anyway, so not needed for 1.3.

Fair enough.

Paolo

Re: [Qemu-devel] [PATCH 1.3 0/5] QOM/qdev lifetime fixes

[Anthony Liguori]

Paolo Bonzini <pbonzini@redhat.com> writes:

> These patches fix problems in the handling of freeing QOM/qdev
> objects.  Together, they fix hot-unplug of USB mass storage devices,
> which crashed with an assertion failure.
>
> I'm not 100% sure, but I think we were always leaking the scsi-disk in
> pre-QOM days.  Now we're freeing it properly, and the assertion proves it.
>
> However, I don't like particularly the assertion in object_delete.  Once
> we're sure we've fixed all bugs, we should remove it, because it prevents
> a fully correct tracking of references.
>
> In this case, for example, there is still one reference to the scsi-disk
> in the MSDState's scsi_dev member.  We don't have neither an object_ref
> nor an object_unref for it, so it happens to work.  If we had an
> object_ref, the matching object_unref would be in dc->exit.  But then
> we'd trip on the assertion failure again, because the SCSI bus is removed
> (thus calling qdev_free on the scsi-dev) before dc->exit is called.
>
> I have more patches to actually make the reference count of devices
> and buses fully correct, but they are even more scary than these, so
> they should wait for 1.4.
>

Applied. Thanks.

My patches didn't fix Peter's problem but yours do, I figured we'd take
your version in 1.3 and then for 1.4 I can attempt to rework them.

Regards,

Anthony Liguori

> Paolo Bonzini (5):
>   qom: fix refcount of non-heap-allocated objects
>   qdev: move bus removal to object_unparent
>   qom: make object_delete usable for statically-allocated objects
>   qdev: simplify (de)allocation of buses
>   qom: make object_finalize static
>
>  hw/qdev-core.h        |  5 -----
>  hw/qdev.c             | 26 ++++++++++++++------------
>  hw/pci.c              |  2 +-
>  hw/sysbus.c           |  2 +-
>  include/qemu/object.h | 29 ++++++++++++++++++++---------
>  qom/object.c          | 12 +++++++++---
>  6 files changed, 45 insertions(+), 31 deletions(-)
>
> -- 
> 1.8.0