From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:36288) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1T3uWZ-0000Jc-7d for qemu-devel@nongnu.org; Tue, 21 Aug 2012 15:54:04 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1T3uWS-0005Uo-1H for qemu-devel@nongnu.org; Tue, 21 Aug 2012 15:54:03 -0400 Received: from e5.ny.us.ibm.com ([32.97.182.145]:40460) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1T3uWR-0005Ti-TY for qemu-devel@nongnu.org; Tue, 21 Aug 2012 15:53:55 -0400 Received: from /spool/local by e5.ny.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Tue, 21 Aug 2012 15:53:46 -0400 Received: from d01relay04.pok.ibm.com (d01relay04.pok.ibm.com [9.56.227.236]) by d01dlp03.pok.ibm.com (Postfix) with ESMTP id 87B73C90043 for ; Tue, 21 Aug 2012 15:53:43 -0400 (EDT) Received: from d03av02.boulder.ibm.com (d03av02.boulder.ibm.com [9.17.195.168]) by d01relay04.pok.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id q7LJrgls137536 for ; Tue, 21 Aug 2012 15:53:42 -0400 Received: from d03av02.boulder.ibm.com (loopback [127.0.0.1]) by d03av02.boulder.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id q7LJrcgr004486 for ; Tue, 21 Aug 2012 13:53:38 -0600 From: Anthony Liguori In-Reply-To: <5033337F.50109@redhat.com> References: <50225901.7040101@msgid.tls.msk.ru> <50225A21.6070208@msgid.tls.msk.ru> <50225DEF.1060206@redhat.com> <50226512.1090705@msgid.tls.msk.ru> <50227AB9.7010206@msgid.tls.msk.ru> <87y5l91m3t.fsf@codemonkey.ws> <5033337F.50109@redhat.com> Date: Tue, 21 Aug 2012 14:53:34 -0500 Message-ID: <87d32k10ox.fsf@codemonkey.ws> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Subject: Re: [Qemu-devel] commit da57febfed "qdev: give all devices a canonical path" broke usb_del List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini Cc: Peter Maydell , Michael Tokarev , qemu-devel Paolo Bonzini writes: > Il 20/08/2012 19:58, Anthony Liguori ha scritto: >> Michael Tokarev 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 >>> 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 >>> Signed-off-by: Michael Tokarev >>> >>> 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