From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:53742) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Sz5gU-0007bB-Ir for qemu-devel@nongnu.org; Wed, 08 Aug 2012 08:48:28 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Sz5gK-00019s-Fk for qemu-devel@nongnu.org; Wed, 08 Aug 2012 08:48:22 -0400 Received: from cantor2.suse.de ([195.135.220.15]:58952 helo=mx2.suse.de) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Sz5gK-00019W-51 for qemu-devel@nongnu.org; Wed, 08 Aug 2012 08:48:12 -0400 Message-ID: <50226006.6050402@suse.de> Date: Wed, 08 Aug 2012 14:48:06 +0200 From: =?UTF-8?B?QW5kcmVhcyBGw6RyYmVy?= MIME-Version: 1.0 References: <50225901.7040101@msgid.tls.msk.ru> <50225A21.6070208@msgid.tls.msk.ru> <50225DEF.1060206@redhat.com> In-Reply-To: <50225DEF.1060206@redhat.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable 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 , Anthony Liguori , Michael Tokarev , qemu-devel 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? >=20 > 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. >=20 > Paolo >=20 > -------------------- 8< ----------------- > From 6d1ea139acf85184fa721654bcc68a4544a536ca Mon Sep 17 00:00:00 2001 > From: Paolo Bonzini > Date: Wed, 8 Aug 2012 14:33:02 +0200 > Subject: [PATCH] qom: object_delete should unparent the object first >=20 > 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. >=20 > 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. >=20 > Signed-off-by: Paolo Bonzini > --- > 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(-) >=20 > 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, u= nsigned slots) > if (pc->no_hotplug) { > slot_free =3D 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) > =20 > rc =3D 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 *s= hpc, int slot) > ++devfn) { > PCIDevice *affected_dev =3D 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) =3D=3D > 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 *ty= pe) > if (type_has_parent(type)) { > object_deinit(obj, type_get_parent(type)); > } > - > - object_unparent(obj); > } > =20 > void object_finalize(void *data) > @@ -402,8 +402,9 @@ Object *object_new(const char *typename) > =20 > void object_delete(Object *obj) > { > + object_unparent(obj); > + g_assert(obj->ref =3D=3D 1); > object_unref(obj); > - g_assert(obj->ref =3D=3D 0); > g_free(obj); > } > =20 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 --=20 SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 N=C3=BCrnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imend=C3=B6rffer; HRB 16746 AG N=C3=BC= rnberg