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

[Peter Crosthwaite]

On Thu, Jun 12, 2014 at 2:49 AM, Paolo Bonzini <address@hidden> wrote:
> 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>

I commented them before, but there are two 80 char lines.

Otherwise:

Reviewed-by: Peter Crosthwaite <address@hidden>

> ---
>  include/qom/object.h | 49 ++++++++++++++++++++++++++++++++++++++++++++++
>  qom/object.c         | 55 
> ++++++++++++++++++++++++++++++++++------------------
>  2 files changed, 85 insertions(+), 19 deletions(-)
>
> 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".
> + */
> +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
> 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);
> -}
> -
>  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;
>      }
> --
> 1.8.3.1
>
>
>