On Fri, Feb 28, 2014 at 06:15:55PM +0100, Andreas Färber 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().
> >
> > Signed-off-by: Stefan Hajnoczi <stefa...@redhat.com>
> > ---
> > include/qom/object.h | 8 ++++++++
> > qom/object.c | 53
> > ++++++++++++++++++++++++++++++----------------------
> > 2 files changed, 39 insertions(+), 22 deletions(-)
> >
> > 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);
> >
> > /**
> > + * 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 root.
> > + */
> > +gchar *object_get_canonical_basename(Object *obj);
>
> I find this name very confusing. There is no such thing as a canonical
> base name, ..._canonical_path_component would make its purpose much more
> 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 please.
Will do in the future.
> > @@ -1102,39 +1102,48 @@ void object_property_add_link(Object *obj, const
> > char *name,
> > g_free(full_type);
> > }
> >
> > +gchar *object_get_canonical_basename(Object *obj)
> > +{
> > + ObjectProperty *prop = NULL;
> > +
> > + g_assert(obj->parent != NULL);
>
> It might make sense to assert obj first? Accessing ->parent would not be
> much different from ->parent->properties otherwise. But applies to the
> original code as well and the movement looks OK.
Will fix.
Stefan
