Re: [Qemu-devel] [PATCH v5 1/7] object: add object_get_canonical_basename()

[Stefan Hajnoczi <stefanha@redhat.com>]

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 <stefanha@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