diff mbox

commit da57febfed "qdev: give all devices a canonical path" broke usb_del

Message ID 50227AB9.7010206@msgid.tls.msk.ru
State New
Headers show

Commit Message

Michael Tokarev Aug. 8, 2012, 2:42 p.m. UTC 
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

Comments

Michael Tokarev Aug. 8, 2012, 4:08 p.m. UTC | #1 
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.
Anthony Liguori Aug. 20, 2012, 5:58 p.m. UTC | #2 
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


>
Paolo Bonzini Aug. 21, 2012, 7:06 a.m. UTC | #3 
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
Anthony Liguori Aug. 21, 2012, 7:53 p.m. UTC | #4 
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
diff mbox

Patch

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