Re: [Qemu-devel] [PATCH 1/5] qom: add a generic mechanism to resolve paths

[Andreas Färber]

Am 11.06.2014 18:49, schrieb Paolo Bonzini:
> It may be desirable to have custom link<> properties that do more
> than just store an object.  Even the addition of a "check"
> function is not enough if setting the link has side effects
> or if a non-standard reference counting is preferrable.
> 
> Avoid the assumption that the opaque field of a link<> is a
> LinkProperty struct, by adding a generic "resolve" callback
> to ObjectProperty.  This fixes aliases of link properties.
> 
> Signed-off-by: Paolo Bonzini <address@hidden>
> ---
>  include/qom/object.h | 49 ++++++++++++++++++++++++++++++++++++++++++++++
>  qom/object.c         | 55 
> ++++++++++++++++++++++++++++++++++------------------
>  2 files changed, 85 insertions(+), 19 deletions(-)

After reading through multiple times, this does seem to make sense... ;)

> diff --git a/include/qom/object.h b/include/qom/object.h
> index a641dcd..f8ab845 100644
> --- a/include/qom/object.h
> +++ b/include/qom/object.h
> @@ -304,6 +304,23 @@ typedef void (ObjectPropertyAccessor)(Object *obj,
>                                        Error **errp);
>  
>  /**
> + * ObjectPropertyResolve:
> + * @obj: the object that owns the property
> + * @opaque: the opaque registered with the property
> + * @part: the name of the property
> + *
> + * If @path is the path that led to @obj, the function should
> + * return the Object corresponding to "@path/@part".  If #NULL
> + * is returned, "@path/@part" is not a valid object path.
> + *
> + * The returned object can also be used as a starting point
> + * to resolve a relative path starting with "@part".

Minor: I would propose something like:

 * Resolves the #Object corresponding to property @part.
 *
 * The returned object can also be used as a starting point
 * to resolve a relative path starting with "@part".
 *
 * Returns: If @path is the path that led to @obj, the function
 * returns the #Object corresponding to "@path/@part".
 * If "@path/@part" is not a valid object path, it returns #NULL.

I.e. inverting the two sentences to fit into a gtk-doc "Returns:"
section, and either Object -> #Object or object.

> + */
> +typedef Object *(ObjectPropertyResolve)(Object *obj,
> +                                        void *opaque,
> +                                        const char *part);
> +
> +/**
>   * ObjectPropertyRelease:
>   * @obj: the object that owns the property
>   * @name: the name of the property
> @@ -321,6 +338,7 @@ typedef struct ObjectProperty
>      gchar *type;
>      ObjectPropertyAccessor *get;
>      ObjectPropertyAccessor *set;
> +    ObjectPropertyResolve *resolve;
>      ObjectPropertyRelease *release;
>      void *opaque;
>  
> @@ -769,6 +787,37 @@ void object_ref(Object *obj);
>  void object_unref(Object *obj);
>  
>  /**
> + * object_property_add_full:
> + * @obj: the object to add a property to
> + * @name: the name of the property.  This can contain any character except 
> for
> + *  a forward slash.  In general, you should use hyphens '-' instead of
> + *  underscores '_' when naming properties.
> + * @type: the type name of the property.  This namespace is pretty loosely
> + *   defined.  Sub namespaces are constructed by using a prefix and then
> + *   to angle brackets.  For instance, the type 'virtio-net-pci' in the
> + *   'link' namespace would be 'link<virtio-net-pci>'.
> + * @get: The getter to be called to read a property.  If this is NULL, then
> + *   the property cannot be read.
> + * @set: the setter to be called to write a property.  If this is NULL,
> + *   then the property cannot be written.
> + * @resolve: called when the property name is used as part of an object
> + *   path.  This is meant for cases when you want to have custom link
> + *   properties.  If it is NULL, the property name cannot be used as part
> + *   of a valid object path.
> + * @release: called when the property is removed from the object.  This is
> + *   meant to allow a property to free its opaque upon object
> + *   destruction.  This may be NULL.
> + * @opaque: an opaque pointer to pass to the callbacks for the property
> + * @errp: returns an error if this function fails
> + */
> +void object_property_add_full(Object *obj, const char *name, const char 
> *type,
> +                              ObjectPropertyAccessor *get,
> +                              ObjectPropertyAccessor *set,
> +                              ObjectPropertyResolve *resolve,
> +                              ObjectPropertyRelease *release,
> +                              void *opaque, Error **errp);
> +
> +/**
>   * object_property_add:
>   * @obj: the object to add a property to
>   * @name: the name of the property.  This can contain any character except 
> for

I'm a bit concerned about the duplication going on here, e.g. of the
forbidden characters. I think we should either just add the new argument
to object_property_add() and add NULL arguments at old call sites as
done before, or we should (to avoid future _really_full,
_really_really_full versions) return the resulting ObjectProperty * for
modification by the caller (in this case: ->resolve = foo).

> diff --git a/qom/object.c b/qom/object.c
> index e42b254..fcdd0da 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -355,11 +355,6 @@ static inline bool 
> object_property_is_child(ObjectProperty *prop)
>      return strstart(prop->type, "child<", NULL);
>  }
>  
> -static inline bool object_property_is_link(ObjectProperty *prop)
> -{
> -    return strstart(prop->type, "link<", NULL);
> -}
> -

(Scratch my earlier comment re macros, this is the function I meant.)

>  static void object_property_del_all(Object *obj)
>  {
>      while (!QTAILQ_EMPTY(&obj->properties)) {
> @@ -727,9 +722,10 @@ void object_unref(Object *obj)
>      }
>  }
>  
> -void object_property_add(Object *obj, const char *name, const char *type,
> +void object_property_add_full(Object *obj, const char *name, const char 
> *type,
>                           ObjectPropertyAccessor *get,
>                           ObjectPropertyAccessor *set,
> +                         ObjectPropertyResolve *resolve,
>                           ObjectPropertyRelease *release,
>                           void *opaque, Error **errp)
>  {
> @@ -751,12 +747,23 @@ void object_property_add(Object *obj, const char *name, 
> const char *type,
>  
>      prop->get = get;
>      prop->set = set;
> +    prop->resolve = resolve;
>      prop->release = release;
>      prop->opaque = opaque;
>  
>      QTAILQ_INSERT_TAIL(&obj->properties, prop, node);
>  }
>  
> +void object_property_add(Object *obj, const char *name, const char *type,
> +                         ObjectPropertyAccessor *get,
> +                         ObjectPropertyAccessor *set,
> +                         ObjectPropertyRelease *release,
> +                         void *opaque, Error **errp)
> +{
> +    object_property_add_full(obj, name, type, get, set, NULL, release,
> +                             opaque, errp);
> +}
> +
>  ObjectProperty *object_property_find(Object *obj, const char *name,
>                                       Error **errp)
>  {
> @@ -993,6 +1000,11 @@ static void object_get_child_property(Object *obj, 
> Visitor *v, void *opaque,
>      g_free(path);
>  }
>  
> +static Object *object_resolve_child_property(Object *parent, void *opaque, 
> const gchar *part)
> +{
> +    return opaque;
> +}
> +
>  static void object_finalize_child_property(Object *obj, const char *name,
>                                             void *opaque)
>  {
> @@ -1009,8 +1021,9 @@ void object_property_add_child(Object *obj, const char 
> *name,
>  
>      type = g_strdup_printf("child<%s>", object_get_typename(OBJECT(child)));
>  
> -    object_property_add(obj, name, type, object_get_child_property, NULL,
> -                        object_finalize_child_property, child, &local_err);
> +    object_property_add_full(obj, name, type, object_get_child_property, 
> NULL,
> +                             object_resolve_child_property,
> +                             object_finalize_child_property, child, 
> &local_err);
>      if (local_err) {
>          error_propagate(errp, local_err);
>          goto out;
> @@ -1128,6 +1141,12 @@ static void object_set_link_property(Object *obj, 
> Visitor *v, void *opaque,
>      }
>  }
>  
> +static Object *object_resolve_link_property(Object *parent, void *opaque, 
> const gchar *part)
> +{
> +    LinkProperty *lprop = opaque;
> +    return *lprop->child;
> +}
> +
>  static void object_release_link_property(Object *obj, const char *name,
>                                           void *opaque)
>  {
> @@ -1156,12 +1175,13 @@ void object_property_add_link(Object *obj, const char 
> *name,
>  
>      full_type = g_strdup_printf("link<%s>", type);
>  
> -    object_property_add(obj, name, full_type,
> -                        object_get_link_property,
> -                        check ? object_set_link_property : NULL,
> -                        object_release_link_property,
> -                        prop,
> -                        &local_err);
> +    object_property_add_full(obj, name, full_type,
> +                             object_get_link_property,
> +                             check ? object_set_link_property : NULL,
> +                             object_resolve_link_property,
> +                             object_release_link_property,
> +                             prop,
> +                             &local_err);
>      if (local_err) {
>          error_propagate(errp, local_err);
>          g_free(prop);
> @@ -1225,11 +1245,8 @@ Object *object_resolve_path_component(Object *parent, 
> const gchar *part)
>          return NULL;
>      }
>  
> -    if (object_property_is_link(prop)) {
> -        LinkProperty *lprop = prop->opaque;
> -        return *lprop->child;
> -    } else if (object_property_is_child(prop)) {
> -        return prop->opaque;
> +    if (prop->resolve) {
> +        return prop->resolve(parent, prop->opaque, part);
>      } else {
>          return NULL;
>      }

The _add vs. _add_full issue aside, the implementation looks good.

Regards,
Andreas

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