From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:59674) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WKPYR-0006Cl-Fy for qemu-devel@nongnu.org; Mon, 03 Mar 2014 04:53:05 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WKPYK-0004Ea-IU for qemu-devel@nongnu.org; Mon, 03 Mar 2014 04:52:59 -0500 Received: from mx1.redhat.com ([209.132.183.28]:45615) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WKPYK-0004EC-AC for qemu-devel@nongnu.org; Mon, 03 Mar 2014 04:52:52 -0500 Date: Mon, 3 Mar 2014 10:52:47 +0100 From: Stefan Hajnoczi Message-ID: <20140303095247.GF4780@stefanha-thinkpad.redhat.com> References: <1393600696-24118-1-git-send-email-stefanha@redhat.com> <1393600696-24118-2-git-send-email-stefanha@redhat.com> <5310C44B.1050302@suse.de> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: <5310C44B.1050302@suse.de> Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v5 1/7] object: add object_get_canonical_basename() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Andreas =?iso-8859-1?Q?F=E4rber?= Cc: Kevin Wolf , Fam Zheng , qemu-devel@nongnu.org, Michael Roth , Igor Mammedov , Paolo Bonzini On Fri, Feb 28, 2014 at 06:15:55PM +0100, Andreas F=E4rber wrote: > Am 28.02.2014 16:18, schrieb Stefan Hajnoczi: > > It is often useful to find an object's child property name. Also use > > this new function to simplify the implementation of > > object_get_canonical_path(). > >=20 > > Signed-off-by: Stefan Hajnoczi > > --- > > include/qom/object.h | 8 ++++++++ > > qom/object.c | 53 ++++++++++++++++++++++++++++++------------= ---------- > > 2 files changed, 39 insertions(+), 22 deletions(-) > >=20 > > diff --git a/include/qom/object.h b/include/qom/object.h > > index 9c7c361..8c6db7c 100644 > > --- a/include/qom/object.h > > +++ b/include/qom/object.h > > @@ -974,6 +974,14 @@ const char *object_property_get_type(Object *obj= , const char *name, > > Object *object_get_root(void); > > =20 > > /** > > + * object_get_canonical_basename: > > + * > > + * Returns: The final component in the object's canonical path. The= canonical > > + * path is the path within the composition tree starting from the ro= ot. > > + */ > > +gchar *object_get_canonical_basename(Object *obj); >=20 > I find this name very confusing. There is no such thing as a canonical > base name, ..._canonical_path_component would make its purpose much mor= e > obvious. If it's confusing then we need to choose a different name. I will rename it to object_get_canonical_path_component() like you suggested. That said, I still think the name I chose makes sense. We already have object_get_canonical_path() and there are POSIX dirname(3)/basename(3) APIs which split paths. So this is basically basename(object_get_canonical_path()), hence object_get_canonical_basename(). And it is canonical because, while there may be link properties with different names that point to this object, only this property name comes from the canonical path. > An underlying issue here probably is that Anthony didn't want a public > API to access the Object::parent. But the only other user is > iothread_get_id() in 4/7, so we might just loop over its known path > prefix there to discover the right child<>. On the other hand, couldn't > device IDs in /machine/peripheral benefit from this today, too? Looping over properties of a well-known object is an alternative but also requires a change: int object_child_foreach(Object *obj, int (*fn)(Object *child, void *opaque), void *opaque); Notice this API does not provide the name of the child to fn()! So either way, we need to extend the QOM API. > In any way I would've liked to get CC'ed on this QOM API proposal pleas= e. Will do in the future. > > @@ -1102,39 +1102,48 @@ void object_property_add_link(Object *obj, co= nst char *name, > > g_free(full_type); > > } > > =20 > > +gchar *object_get_canonical_basename(Object *obj) > > +{ > > + ObjectProperty *prop =3D NULL; > > + > > + g_assert(obj->parent !=3D NULL); >=20 > It might make sense to assert obj first? Accessing ->parent would not b= e > much different from ->parent->properties otherwise. But applies to the > original code as well and the movement looks OK. Will fix. Stefan