Re: [Qemu-devel] commit da57febfed "qdev: give all devices a canonical path" broke usb_del

[Paolo Bonzini <pbonzini@redhat.com>]

Il 08/08/2012 14:22, Michael Tokarev ha scritto:
> @@ -152,6 +152,16 @@ int qdev_init(DeviceState *dev)
> +
> +    if (!OBJECT(dev)->parent) {
> +        static int unattached_count = 0;
> +        gchar *name = g_strdup_printf("device[%d]", unattached_count++);
> +
> +        object_property_add_child(container_get("/unattached"), name,
> +                                  OBJECT(dev), NULL);
> +        g_free(name);
> +    }
> 
> ie, there's now extra call to object_property_add_child(),
> which does object_ref().  So no doubt there's no a new
> assertion failure: (obj->ref == 0).
> 
> Once again, patch description does not reflect what is
> actually done by the patch.

Huh? The add_child ensures that there is a canonical path.

> Can we please stop this
> practice of accepting patches with desrciption not
> reflecting reality?
> 
> Back to the point: should there be a call to object_unref()
> somewhere?

There should be a call to object_unparent() somewhere actually.
We've been peppering the code with them for a few months now while
waiting for "the" solution, but I fail to see what the solution
could be other than the patch below (something similar has probably
been proposed already).  BTW there are probably a lot of other similar
bugs somewhere.

Paolo

-------------------- 8< -----------------
>From 6d1ea139acf85184fa721654bcc68a4544a536ca Mon Sep 17 00:00:00 2001
From: Paolo Bonzini <pbonzini@redhat.com>
Date: Wed, 8 Aug 2012 14:33:02 +0200
Subject: [PATCH] qom: object_delete should unparent the object first

object_deinit is only called when the reference count goes to zero,
and yet tries to do an object_unparent.  Now, object_unparent
either does nothing or it will decrease the reference count.
Because we know the reference count is zero, the object_unparent
call in object_deinit is useless.

Instead, we need to disconnect the object from its parent just
before we remove the last reference apart from the parent's.  This
happens in object_delete.  Once we do this, all calls to
object_unparent peppered through QEMU can go away.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/acpi_piix4.c   | 1 -
 hw/qdev.c         | 2 --
 hw/shpc.c         | 1 -
 hw/xen_platform.c | 3 ---
 qom/object.c      | 3 +--
 5 file modificati, 1 inserzione(+), 9 rimozioni(-)

diff --git a/hw/acpi_piix4.c b/hw/acpi_piix4.c
index 0aace60..72d6e5c 100644
--- a/hw/acpi_piix4.c
+++ b/hw/acpi_piix4.c
@@ -305,7 +305,6 @@ static void acpi_piix_eject_slot(PIIX4PMState *s, unsigned slots)
             if (pc->no_hotplug) {
                 slot_free = false;
             } else {
-                object_unparent(OBJECT(dev));
                 qdev_free(qdev);
             }
         }
diff --git a/hw/qdev.c b/hw/qdev.c
index b5b74b9..b5a52ac 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -159,7 +159,6 @@ int qdev_init(DeviceState *dev)
 
     rc = dc->init(dev);
     if (rc < 0) {
-        object_unparent(OBJECT(dev));
         qdev_free(dev);
         return rc;
     }
@@ -243,7 +242,6 @@ void qbus_reset_all_fn(void *opaque)
 int qdev_simple_unplug_cb(DeviceState *dev)
 {
     /* just zap it */
-    object_unparent(OBJECT(dev));
     qdev_free(dev);
     return 0;
 }
diff --git a/hw/shpc.c b/hw/shpc.c
index 6b9884d..a5baf24 100644
--- a/hw/shpc.c
+++ b/hw/shpc.c
@@ -253,7 +253,6 @@ static void shpc_free_devices_in_slot(SHPCDevice *shpc, int slot)
          ++devfn) {
         PCIDevice *affected_dev = shpc->sec_bus->devices[devfn];
         if (affected_dev) {
-            object_unparent(OBJECT(affected_dev));
             qdev_free(&affected_dev->qdev);
         }
     }
diff --git a/hw/xen_platform.c b/hw/xen_platform.c
index c1fe984..0d6c2ff 100644
--- a/hw/xen_platform.c
+++ b/hw/xen_platform.c
@@ -87,9 +87,6 @@ static void unplug_nic(PCIBus *b, PCIDevice *d, void *o)
 {
     if (pci_get_word(d->config + PCI_CLASS_DEVICE) ==
             PCI_CLASS_NETWORK_ETHERNET) {
-        /* Until qdev_free includes a call to object_unparent, we call it here
-         */
-        object_unparent(&d->qdev.parent_obj);
         qdev_free(&d->qdev);
     }
 }
diff --git a/qom/object.c b/qom/object.c
index 00bb3b0..3ccd744 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -366,8 +366,6 @@ static void object_deinit(Object *obj, TypeImpl *type)
     if (type_has_parent(type)) {
         object_deinit(obj, type_get_parent(type));
     }
-
-    object_unparent(obj);
 }
 
 void object_finalize(void *data)
@@ -402,8 +402,9 @@ Object *object_new(const char *typename)
 
 void object_delete(Object *obj)
 {
+    object_unparent(obj);
+    g_assert(obj->ref == 1);
     object_unref(obj);
-    g_assert(obj->ref == 0);
     g_free(obj);
 }
 
-- 
1.7.11.2