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

["Andreas Färber" <afaerber@suse.de>]

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?

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;

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg