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

[Igor Mammedov <imammedo@redhat.com>]

On Fri, 28 Feb 2014 18:15:55 +0100
Andreas Färber <afaerber@suse.de> 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.
> 
> 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?
there is another user for this, I need similar routine for memdev object.

alternatively it seems that Object that has parent is always name so we could
add 'name' property to Object and replace with it device's 'id'

> 
> In any way I would've liked to get CC'ed on this QOM API proposal please.
> 
> > +
> > +/**
> >   * object_get_canonical_path:
> >   *
> >   * Returns: The canonical path for a object.  This is the path within the
> > diff --git a/qom/object.c b/qom/object.c
> > index 660859c..0cdc319 100644
> > --- a/qom/object.c
> > +++ b/qom/object.c
> > @@ -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.
> 
> Regards,
> Andreas
> 
> > +
> > +    QTAILQ_FOREACH(prop, &obj->parent->properties, node) {
> > +        if (!object_property_is_child(prop)) {
> > +            continue;
> > +        }
> > +
> > +        if (prop->opaque == obj) {
> > +            return g_strdup(prop->name);
> > +        }
> > +    }
> > +
> > +    /* obj had a parent but was not a child, should never happen */
> > +    g_assert_not_reached();
> > +    return NULL;
> > +}
> > +
> >  gchar *object_get_canonical_path(Object *obj)
> >  {
> >      Object *root = object_get_root();
> > -    char *newpath = NULL, *path = NULL;
> > +    char *newpath, *path = NULL;
> >  
> >      while (obj != root) {
> > -        ObjectProperty *prop = NULL;
> > -
> > -        g_assert(obj->parent != NULL);
> > -
> > -        QTAILQ_FOREACH(prop, &obj->parent->properties, node) {
> > -            if (!object_property_is_child(prop)) {
> > -                continue;
> > -            }
> > +        char *component = object_get_canonical_basename(obj);
> >  
> > -            if (prop->opaque == obj) {
> > -                if (path) {
> > -                    newpath = g_strdup_printf("%s/%s", prop->name, path);
> > -                    g_free(path);
> > -                    path = newpath;
> > -                } else {
> > -                    path = g_strdup(prop->name);
> > -                }
> > -                break;
> > -            }
> > +        if (path) {
> > +            newpath = g_strdup_printf("%s/%s", component, path);
> > +            g_free(component);
> > +            g_free(path);
> > +            path = newpath;
> > +        } else {
> > +            path = component;
> >          }
> >  
> > -        g_assert(prop != NULL);
> > -
> >          obj = obj->parent;
> >      }
> >  
> > -    newpath = g_strdup_printf("/%s", path);
> > +    newpath = g_strdup_printf("/%s", path ? path : "");
> >      g_free(path);
> >  
> >      return newpath;
>