From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:35706) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UHbbF-0001U8-0f for qemu-devel@nongnu.org; Mon, 18 Mar 2013 11:03:51 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UHbbA-000683-1o for qemu-devel@nongnu.org; Mon, 18 Mar 2013 11:03:44 -0400 Received: from mx1.redhat.com ([209.132.183.28]:36083) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UHbb9-00067X-QC for qemu-devel@nongnu.org; Mon, 18 Mar 2013 11:03:39 -0400 Message-ID: <51472CC0.8010706@redhat.com> Date: Mon, 18 Mar 2013 16:03:28 +0100 From: Paolo Bonzini MIME-Version: 1.0 References: <50e744fbae4b08dc4ec33d5d44acc83da7170391.1363264726.git.mst@redhat.com> <87zjy0946n.fsf@codemonkey.ws> In-Reply-To: <87zjy0946n.fsf@codemonkey.ws> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v8 2/3] qom: pass original path to unparent method List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Anthony Liguori Cc: Kevin Wolf , Eduardo Habkost , "Michael S. Tsirkin" , libvir-list@redhat.com, Stefan Hajnoczi , Markus Armbruster , qemu-devel@nongnu.org, Gerd Hoffmann , Luiz Capitulino , Andreas =?us-ascii?B?PT91dGYtOD9RP0Y9QzM9QTRyYmVy Pz0=?= Il 18/03/2013 15:24, Anthony Liguori ha scritto: > "Michael S. Tsirkin" writes: > >> We need to know the original path since unparenting loses this state. >> >> Signed-off-by: Michael S. Tsirkin >> --- >> hw/qdev.c | 4 ++-- >> include/qom/object.h | 3 ++- >> qom/object.c | 4 +++- >> 3 files changed, 7 insertions(+), 4 deletions(-) >> >> diff --git a/hw/qdev.c b/hw/qdev.c >> index 741af96..64546cf 100644 >> --- a/hw/qdev.c >> +++ b/hw/qdev.c >> @@ -436,7 +436,7 @@ static void qbus_realize(BusState *bus, DeviceState *parent, const char *name) >> } >> } >> >> -static void bus_unparent(Object *obj) >> +static void bus_unparent(Object *obj, const char *path) >> { >> BusState *bus = BUS(obj); >> BusChild *kid; >> @@ -756,7 +756,7 @@ static void device_class_base_init(ObjectClass *class, void *data) >> klass->props = NULL; >> } >> >> -static void device_unparent(Object *obj) >> +static void device_unparent(Object *obj, const char *path) >> { >> DeviceState *dev = DEVICE(obj); >> DeviceClass *dc = DEVICE_GET_CLASS(dev); >> diff --git a/include/qom/object.h b/include/qom/object.h >> index cf094e7..f0790d4 100644 >> --- a/include/qom/object.h >> +++ b/include/qom/object.h >> @@ -330,11 +330,12 @@ typedef struct ObjectProperty >> /** >> * ObjectUnparent: >> * @obj: the object that is being removed from the composition tree >> + * @path: canonical path that object had if any >> * >> * Called when an object is being removed from the QOM composition tree. >> * The function should remove any backlinks from children objects to @obj. >> */ >> -typedef void (ObjectUnparent)(Object *obj); >> +typedef void (ObjectUnparent)(Object *obj, const char *path); >> >> /** >> * ObjectFree: >> diff --git a/qom/object.c b/qom/object.c >> index 3d638ff..21c9da4 100644 >> --- a/qom/object.c >> +++ b/qom/object.c >> @@ -362,14 +362,16 @@ static void object_property_del_child(Object *obj, Object *child, Error **errp) >> >> void object_unparent(Object *obj) >> { >> + gchar *path = object_get_canonical_path(obj); >> object_ref(obj); >> if (obj->parent) { >> object_property_del_child(obj->parent, obj, NULL); >> } >> if (obj->class->unparent) { >> - (obj->class->unparent)(obj); >> + (obj->class->unparent)(obj, path); >> } > > I think you should actually just move this call above > if (obj->parent) { object_parent_del_child(...); }. > > There's no harm AFAICT in doing this and it seems more logical to me to > have destruction flow start with the subclass and move up to the base > class. > > This avoids needing a hack like this because the object is still in a > reasonable state when unparent is called. > > Paolo, do you see anything wrong with this? I looked at the commit you > added this in and it doesn't look like it would be a problem. Yes, seems okay. Especially if you think of object_property_del_child as the base class's implementation of unparent. Paolo