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

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

[Michael Tokarev]

While dealing with USB issues today I noticed that
usb_del monitor command is broken, attempting to
delete any usb device immediately results in assertion
failure:

(qemu) usb_del 0.1
ERROR:qom/object.c:408:object_delete: assertion failed: (obj->ref == 0)
Aborted


I bisected this issue to commit:

commit da57febfed7bad11be79f047b59719c38abd0712
Author: Paolo Bonzini <pbonzini@redhat.com>
Date:   Tue Mar 27 18:38:47 2012 +0200

    qdev: give all devices a canonical path

    A strong limitation of QOM right now is that unconverted ports
    (e.g. all...) do not give a canonical path to devices that are
    part of the board.  This in turn makes it impossible to replace
    PROP_PTR with a QOM link for example.

    Reviewed-by: Anthony Liguori <aliguori@us.ibm.com>
    Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
    Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>


The problem is still present in current qemu/master git.

I'm not sure what to do with this.

See also http://lists.gnu.org/archive/html/qemu-devel/2012-07/msg00199.html --
Peter Maydell reoprted this very issue a while back but no one replied.

Thanks,

/mjt

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

[Michael Tokarev]

On 08.08.2012 16:18, Michael Tokarev wrote:
> While dealing with USB issues today I noticed that
> usb_del monitor command is broken, attempting to
> delete any usb device immediately results in assertion
> failure:
> 
> (qemu) usb_del 0.1
> ERROR:qom/object.c:408:object_delete: assertion failed: (obj->ref == 0)
> Aborted
> 
> 
> I bisected this issue to commit:
> 
> commit da57febfed7bad11be79f047b59719c38abd0712
> Author: Paolo Bonzini <pbonzini@redhat.com>
> Date:   Tue Mar 27 18:38:47 2012 +0200
> 
>     qdev: give all devices a canonical path
> 
>     A strong limitation of QOM right now is that unconverted ports
>     (e.g. all...) do not give a canonical path to devices that are
>     part of the board.  This in turn makes it impossible to replace
>     PROP_PTR with a QOM link for example.
> 
>     Reviewed-by: Anthony Liguori <aliguori@us.ibm.com>
>     Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>     Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> 
> 
> The problem is still present in current qemu/master git.
> 
> I'm not sure what to do with this.

Well.  This commit adds this code:

@@ -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.  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?

Thanks,

/mjt

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

[Paolo Bonzini]

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

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

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

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

[Paolo Bonzini]

Il 08/08/2012 14:48, Andreas Färber ha scritto:
> 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?

object_property_del_all should take care of it for embedded objects:
- the outermost struct is deleted via object_delete
- it is unparented and unrefed, so refcount goes to 0 and finalize is called
- finalize calls object_property_del_all
- object_finalize_child_property calls unref on the nested object, so
refcount goes to 0 and finalize is called.  Things can then proceed
recursively.

It would be more complicated (and would cause memory leaks) if you got
nested objects that were allocated.  To account for this, I understood
you would need something like the following:

- you add a "void (*release)(void *obj)" member to Object.
- object_new sets the release member to g_free
- object_delete does not call g_free anymore
- object_finalize calls the release member if it is not NULL

Do you have time to implement it?

Paolo

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

[Michael Tokarev]

On 08.08.2012 16:39, Paolo Bonzini wrote:
> 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?

I must clarify this.

I'm not trying to blame Paolo for the wrong patch which
"broke" things (it exposed an old bug in other codepath).
I'm just saying that the patch description appears to be
"too innocent" -- yes, now each device has a path, or,
in other words, a NAME, but this patch ALSO changes
refcounting, and THIS is what I'm referring to above --
it isn't only gives a name, but also links "unowned"
objects to a bus.  To me it is a wrong (too innocent)
description.  I browsed comits trying to find which
change might have caused it, and provided ther was
a mention of "references" or "connecting" in this patch
description somewhere, I'd found this change much faster,
without a painful (*) bisection.

Maybe it is just me who does not know this code area
well enough, so things aren't as obvious for me as for
others, I dunno, but to me the description -- the only
thing I'm trying to "blame" Paolo for here -- might be
quite a bit better.

(*) painful because this bisection come in the process
of another bisection dealing with another usb issue,
which come to yet another bug in usb handling somewhere
(giving another assertion).

>> 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.

This sounds ecouraging -- "alot of other similar bugs".. :(

Something similar should be applied to 1.1-stable.  FWIW, some
changes are not needed there, like this:
> --- 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;
>      }

and this:

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

the lines being removed does not exist in 1.1-stable.

I can guess this is due to previous attempts to fix
similar issues in other places.

Thank you!

/mjt

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

[Michael Tokarev]

[-- Attachment #1: Type: text/plain, Size: 432 bytes --]

On 08.08.2012 17:09, Michael Tokarev wrote:
[]
> Something similar should be applied to 1.1-stable.  FWIW, some
> changes are not needed there.

Cherry-pick to stable-1.1 removes the two unneeded hunks.
This is what I plan to include into debian package.  It
fixes the original usb_del issue, and I didn't find new
regressions so far - tried a few device_del and similar.

Should it go to qemu/stable-1.1 as well?

Thank you!

/mjt

[-- Attachment #2: qom:-object_delete-should-unparent-the-object-first.diff --]
[-- Type: text/x-patch, Size: 2688 bytes --]

Author: Paolo Bonzini <pbonzini@redhat.com>
Date:   Wed Aug 8 14:39:11 2012 +0200
Bug-Debian: http://bugs.debian.org/684282
Comment: cherry-picked from qemu/master to stable-1.1 (mjt)

    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>
    Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>

diff --git a/hw/acpi_piix4.c b/hw/acpi_piix4.c
index 0345490..585da4e 100644
--- a/hw/acpi_piix4.c
+++ b/hw/acpi_piix4.c
@@ -299,7 +299,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 6a8f6bd..9bb1c6b 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -240,7 +240,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/xen_platform.c b/hw/xen_platform.c
index 0214f37..84221df 100644
--- a/hw/xen_platform.c
+++ b/hw/xen_platform.c
@@ -87,9 +87,6 @@ static void unplug_nic(PCIBus *b, PCIDevice *d)
 {
     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 6f839ad..58dd886 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -347,8 +347,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)
@@ -385,8 +383,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);
 }
 

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

[Michael Tokarev]

On 08.08.2012 18:42, Michael Tokarev wrote:
> Should it go to qemu/stable-1.1 as well?

qemu/stable-1.1 also includes f63e60327b8e239ae97fa71060940ca20a8bf38e.
FWIW.

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

[Anthony Liguori]

Michael Tokarev <mjt@tls.msk.ru> writes:

> On 08.08.2012 17:09, Michael Tokarev wrote:
> []
>> Something similar should be applied to 1.1-stable.  FWIW, some
>> changes are not needed there.
>
> Cherry-pick to stable-1.1 removes the two unneeded hunks.
> This is what I plan to include into debian package.  It
> fixes the original usb_del issue, and I didn't find new
> regressions so far - tried a few device_del and similar.
>
> Should it go to qemu/stable-1.1 as well?
>
> Thank you!
>
> /mjtAuthor: Paolo Bonzini <pbonzini@redhat.com>
> Date:   Wed Aug 8 14:39:11 2012 +0200
> Bug-Debian: http://bugs.debian.org/684282
> Comment: cherry-picked from qemu/master to stable-1.1 (mjt)
>
>     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>
>     Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
>
> diff --git a/hw/acpi_piix4.c b/hw/acpi_piix4.c
> index 0345490..585da4e 100644
> --- a/hw/acpi_piix4.c
> +++ b/hw/acpi_piix4.c
> @@ -299,7 +299,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 6a8f6bd..9bb1c6b 100644
> --- a/hw/qdev.c
> +++ b/hw/qdev.c
> @@ -240,7 +240,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/xen_platform.c b/hw/xen_platform.c
> index 0214f37..84221df 100644
> --- a/hw/xen_platform.c
> +++ b/hw/xen_platform.c
> @@ -87,9 +87,6 @@ static void unplug_nic(PCIBus *b, PCIDevice *d)
>  {
>      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 6f839ad..58dd886 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -347,8 +347,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)
> @@ -385,8 +383,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);
>  }

This won't work with composition.  object_delete() is never called for
child<> objects.

Regards,

Anthony Liguori


>  

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

[Paolo Bonzini]

Il 20/08/2012 19:58, Anthony Liguori ha scritto:
> Michael Tokarev <mjt@tls.msk.ru> writes:
> 
>> On 08.08.2012 17:09, Michael Tokarev wrote:
>> []
>>> Something similar should be applied to 1.1-stable.  FWIW, some
>>> changes are not needed there.
>>
>> Cherry-pick to stable-1.1 removes the two unneeded hunks.
>> This is what I plan to include into debian package.  It
>> fixes the original usb_del issue, and I didn't find new
>> regressions so far - tried a few device_del and similar.
>>
>> Should it go to qemu/stable-1.1 as well?
>>
>> Thank you!
>>
>> /mjtAuthor: Paolo Bonzini <pbonzini@redhat.com>
>> Date:   Wed Aug 8 14:39:11 2012 +0200
>> Bug-Debian: http://bugs.debian.org/684282
>> Comment: cherry-picked from qemu/master to stable-1.1 (mjt)
>>
>>     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>
>>     Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
>>
>> diff --git a/hw/acpi_piix4.c b/hw/acpi_piix4.c
>> index 0345490..585da4e 100644
>> --- a/hw/acpi_piix4.c
>> +++ b/hw/acpi_piix4.c
>> @@ -299,7 +299,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 6a8f6bd..9bb1c6b 100644
>> --- a/hw/qdev.c
>> +++ b/hw/qdev.c
>> @@ -240,7 +240,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/xen_platform.c b/hw/xen_platform.c
>> index 0214f37..84221df 100644
>> --- a/hw/xen_platform.c
>> +++ b/hw/xen_platform.c
>> @@ -87,9 +87,6 @@ static void unplug_nic(PCIBus *b, PCIDevice *d)
>>  {
>>      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 6f839ad..58dd886 100644
>> --- a/qom/object.c
>> +++ b/qom/object.c
>> @@ -347,8 +347,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)
>> @@ -385,8 +383,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);
>>  }
> 
> This won't work with composition.  object_delete() is never called for
> child<> objects.

For non-heap-allocated children, their last ref will go away when the
parent's child<> property is eliminated.  This will remove the last
reference and call object_finalize (which will take care of multiple
levels of compositions).

The same holds for heap-allocated children, but indeed you will leak the
memory for the object because object_delete is not called.  However this
is already the case, the patch is not introducing a regression.

Paolo

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

[Anthony Liguori]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Il 20/08/2012 19:58, Anthony Liguori ha scritto:
>> Michael Tokarev <mjt@tls.msk.ru> writes:
>> 
>>> On 08.08.2012 17:09, Michael Tokarev wrote:
>>> []
>>>> Something similar should be applied to 1.1-stable.  FWIW, some
>>>> changes are not needed there.
>>>
>>> Cherry-pick to stable-1.1 removes the two unneeded hunks.
>>> This is what I plan to include into debian package.  It
>>> fixes the original usb_del issue, and I didn't find new
>>> regressions so far - tried a few device_del and similar.
>>>
>>> Should it go to qemu/stable-1.1 as well?
>>>
>>> Thank you!
>>>
>>> /mjtAuthor: Paolo Bonzini <pbonzini@redhat.com>
>>> Date:   Wed Aug 8 14:39:11 2012 +0200
>>> Bug-Debian: http://bugs.debian.org/684282
>>> Comment: cherry-picked from qemu/master to stable-1.1 (mjt)
>>>
>>>     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>
>>>     Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
>>>
>>> diff --git a/hw/acpi_piix4.c b/hw/acpi_piix4.c
>>> index 0345490..585da4e 100644
>>> --- a/hw/acpi_piix4.c
>>> +++ b/hw/acpi_piix4.c
>>> @@ -299,7 +299,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 6a8f6bd..9bb1c6b 100644
>>> --- a/hw/qdev.c
>>> +++ b/hw/qdev.c
>>> @@ -240,7 +240,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/xen_platform.c b/hw/xen_platform.c
>>> index 0214f37..84221df 100644
>>> --- a/hw/xen_platform.c
>>> +++ b/hw/xen_platform.c
>>> @@ -87,9 +87,6 @@ static void unplug_nic(PCIBus *b, PCIDevice *d)
>>>  {
>>>      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 6f839ad..58dd886 100644
>>> --- a/qom/object.c
>>> +++ b/qom/object.c
>>> @@ -347,8 +347,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)
>>> @@ -385,8 +383,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);
>>>  }
>> 
>> This won't work with composition.  object_delete() is never called for
>> child<> objects.
>
> For non-heap-allocated children, their last ref will go away when the
> parent's child<> property is eliminated.  This will remove the last
> reference and call object_finalize (which will take care of multiple
> levels of compositions).
>
> The same holds for heap-allocated children, but indeed you will leak the
> memory for the object because object_delete is not called.  However this
> is already the case, the patch is not introducing a regression.

Ok, can you submit as a top level patch and I'll apply it for 1.2?

Regards,

Anthony Liguori

>
> Paolo