[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] commit da57febfed "qdev: give all devices a canonical p
From: |
Anthony Liguori |
Subject: |
Re: [Qemu-devel] commit da57febfed "qdev: give all devices a canonical path" broke usb_del |
Date: |
Tue, 21 Aug 2012 14:53:34 -0500 |
User-agent: |
Notmuch/0.13.2+93~ged93d79 (http://notmuchmail.org) Emacs/23.3.1 (x86_64-pc-linux-gnu) |
Paolo Bonzini <address@hidden> writes:
> Il 20/08/2012 19:58, Anthony Liguori ha scritto:
>> Michael Tokarev <address@hidden> 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 <address@hidden>
>>> 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 <address@hidden>
>>> Signed-off-by: Michael Tokarev <address@hidden>
>>>
>>> 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
- [Qemu-devel] commit da57febfed "qdev: give all devices a canonical path" broke usb_del, Michael Tokarev, 2012/08/08
- Re: [Qemu-devel] commit da57febfed "qdev: give all devices a canonical path" broke usb_del, Michael Tokarev, 2012/08/08
- Re: [Qemu-devel] commit da57febfed "qdev: give all devices a canonical path" broke usb_del, Paolo Bonzini, 2012/08/08
- Re: [Qemu-devel] commit da57febfed "qdev: give all devices a canonical path" broke usb_del, Michael Tokarev, 2012/08/08
- Re: [Qemu-devel] commit da57febfed "qdev: give all devices a canonical path" broke usb_del, Michael Tokarev, 2012/08/08
- Re: [Qemu-devel] commit da57febfed "qdev: give all devices a canonical path" broke usb_del, Michael Tokarev, 2012/08/08
- Re: [Qemu-devel] commit da57febfed "qdev: give all devices a canonical path" broke usb_del, Anthony Liguori, 2012/08/20
- Re: [Qemu-devel] commit da57febfed "qdev: give all devices a canonical path" broke usb_del, Paolo Bonzini, 2012/08/21
- Re: [Qemu-devel] commit da57febfed "qdev: give all devices a canonical path" broke usb_del,
Anthony Liguori <=