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

[Andreas Färber]

Am 08.08.2012 14:39, schrieb Paolo Bonzini:
> Il 08/08/2012 14:22, Michael Tokarev ha scritto:
>> [...] 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 <address@hidden>
> 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 <address@hidden>
> ---
>  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);
>  }
>  

Adding object_unparent() to object_delete() looks okay to me, but we
should not forget about the upcoming i440fx and prep_pci use cases where
we want to embed children in the parent's struct, so that
object_delete() will never be called on it. Thus object_unparent() would
need to remain present in the deinit path, no?

Andreas

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