[v5,1/7] object: add object_get_canonical_basename()

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(-)

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;

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;
>

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