From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:54226) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UHb9p-0005yx-Uu for qemu-devel@nongnu.org; Mon, 18 Mar 2013 10:35:32 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UHb9m-0004GT-SH for qemu-devel@nongnu.org; Mon, 18 Mar 2013 10:35:25 -0400 Received: from mx1.redhat.com ([209.132.183.28]:16864) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UHb9m-0004GG-K2 for qemu-devel@nongnu.org; Mon, 18 Mar 2013 10:35:22 -0400 Date: Mon, 18 Mar 2013 16:35:51 +0200 From: "Michael S. Tsirkin" Message-ID: <20130318143551.GA13513@redhat.com> References: <50e744fbae4b08dc4ec33d5d44acc83da7170391.1363264726.git.mst@redhat.com> <87zjy0946n.fsf@codemonkey.ws> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <87zjy0946n.fsf@codemonkey.ws> 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 , libvir-list@redhat.com, Stefan Hajnoczi , qemu-devel@nongnu.org, Markus Armbruster , Gerd Hoffmann , Paolo Bonzini , Luiz Capitulino , Andreas =?us-ascii?B?PT91dGYtOD9RP0Y9QzM9QTRyYmVyPz0=?= On Mon, Mar 18, 2013 at 09:24:16AM -0500, Anthony Liguori wrote: > "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. At Paolo's request children are intentionally reported before parents, shouldn't this apply? > > 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. > > Regards, > > Anthony Liguori Hmm I already put this on my branch (and sent a pull request). I guess I could back it out, though it will create minor problems if someone is basing on my tree. Cleanup in a separate patch? > > object_unref(obj); > > + g_free(path); > > } > > > > static void object_deinit(Object *obj, TypeImpl *type) > > -- > > MST