[Qemu-devel] [PATCH 0/5] qom: path resolution, property aliases and more

[Qemu-devel] [PATCH 0/5] qom: path resolution, property aliases and more

[Paolo Bonzini]

This series builds on Stefan's object_property_add_alias() and exapnds it
to include:

a) aliases to links (needed for dataplane)

b) aliases to child<> properties (needed for c below)

c) aliases to objects (needed by Marcelo :))

Paolo

Marcelo Tosatti (1):
  mc146818rtc: add "rtc" link to "/machine"

Paolo Bonzini (3):
  qom: add a generic mechanism to resolve paths
  qom: allow creating an alias of a child<> property
  qom: allow creating an alias of an object

Stefan Hajnoczi (1):
  qom: add object_property_add_alias()

 hw/timer/mc146818rtc.c |   9 ++++
 include/qom/object.h   |  73 ++++++++++++++++++++++++++
 qom/object.c           | 135 ++++++++++++++++++++++++++++++++++++++++++-------
 3 files changed, 198 insertions(+), 19 deletions(-)

-- 
1.8.3.1

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

[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 <pbonzini@redhat.com>
---
 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

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 <pbonzini@redhat.com> 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 <pbonzini@redhat.com>

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

Otherwise:

Reviewed-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>

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

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 <pbonzini@redhat.com>
> ---
>  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

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

[Paolo Bonzini]

Il 17/06/2014 16:18, Andreas Färber ha scritto:
>>  /**
>> + * 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.

Good suggestion.
>> +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);
>
> 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).

The reason I went with "_full" is that the new argument is really needed 
only in a minority of cases.  There are ~50 occurrences right now, and I 
expect only a handful of them to need a ->resolve callback (and so far 
all of them would be in qom/object.c).

There are many examples in glib's GSource (g_timeout_add_full, 
g_main_context_invoke_full, etc.) or elsewhere in glib (g_format_size_full).

Since we do not have an ABI to follow, we could add arguments to the 
_full routine while keeping the shorthand version as is.

I can change the 50 occurrences if you want though.

Paolo

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

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

[Andreas Färber]

Am 17.06.2014 17:07, schrieb Paolo Bonzini:
> Il 17/06/2014 16:18, Andreas Färber ha scritto:
>>> +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);
>>
>> 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).
> 
> The reason I went with "_full" is that the new argument is really needed
> only in a minority of cases.  There are ~50 occurrences right now, and I
> expect only a handful of them to need a ->resolve callback (and so far
> all of them would be in qom/object.c).
> 
> There are many examples in glib's GSource (g_timeout_add_full,
> g_main_context_invoke_full, etc.) or elsewhere in glib
> (g_format_size_full).
> 
> Since we do not have an ABI to follow, we could add arguments to the
> _full routine while keeping the shorthand version as is.
> 
> I can change the 50 occurrences if you want though.

So what about my alternative suggestion of changing _add's void ->
ObjectProperty*? That would limit future updating to the struct itself
while still avoiding to touch the 50.

Andreas

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

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

[Paolo Bonzini]

Il 17/06/2014 17:19, Andreas Färber ha scritto:
> So what about my alternative suggestion of changing _add's void ->
> ObjectProperty*? That would limit future updating to the struct itself
> while still avoiding to touch the 50.

That's fine by me too.

Paolo

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

[Andreas Färber]

Am 17.06.2014 16:18, schrieb Andreas Färber:
> Am 11.06.2014 18:49, schrieb Paolo Bonzini:
>> diff --git a/qom/object.c b/qom/object.c
>> index e42b254..fcdd0da 100644
>> --- a/qom/object.c
>> +++ b/qom/object.c
[...]
>> @@ -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;

Forgot: White line preferred even for single-line statements for
consistency.

>> +    return *lprop->child;
>> +}
>> +
>>  static void object_release_link_property(Object *obj, const char *name,
>>                                           void *opaque)
>>  {

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

[Qemu-devel] [PATCH 2/5] qom: add object_property_add_alias()

[Paolo Bonzini]

From: Stefan Hajnoczi <stefanha@redhat.com>

Sometimes an object needs to present a property which is actually on
another object, or it needs to provide an alias name for an existing
property.

Examples:
  a.foo -> b.foo
  a.old_name -> a.new_name

The new object_property_add_alias() API allows objects to alias a
property on the same object or another object.  The source and target
names can be different.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 include/qom/object.h | 20 +++++++++++++++++
 qom/object.c         | 61 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 81 insertions(+)

diff --git a/include/qom/object.h b/include/qom/object.h
index f8ab845..9c4a5a4 100644
--- a/include/qom/object.h
+++ b/include/qom/object.h
@@ -1252,6 +1252,26 @@ void object_property_add_uint64_ptr(Object *obj, const char *name,
                                     const uint64_t *v, Error **Errp);
 
 /**
+ * object_property_add_alias:
+ * @obj: the object to add a property to
+ * @name: the name of the property
+ * @target_obj: the object to forward property access to
+ * @target_name: the name of the property on the forwarded object
+ * @errp: if an error occurs, a pointer to an area to store the error
+ *
+ * Add an alias for a property on an object.  This function will add a property
+ * of the same type as the forwarded property.
+ *
+ * The caller must ensure that <code>@target_obj</code> stays alive as long as
+ * this property exists.  In the case of a child object or an alias on the same
+ * object this will be the case.  For aliases to other objects the caller is
+ * responsible for taking a reference.
+ */
+void object_property_add_alias(Object *obj, const char *name,
+                               Object *target_obj, const char *target_name,
+                               Error **errp);
+
+/**
  * object_child_foreach:
  * @obj: the object whose children will be navigated
  * @fn: the iterator function to be called
diff --git a/qom/object.c b/qom/object.c
index fcdd0da..e146ae5 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -1532,6 +1532,67 @@ void object_property_add_uint64_ptr(Object *obj, const char *name,
                         NULL, NULL, (void *)v, errp);
 }
 
+typedef struct
+{
+    Object *target_obj;
+    const char *target_name;
+} AliasProperty;
+
+static void property_get_alias(Object *obj, struct Visitor *v, void *opaque,
+                               const char *name, Error **errp)
+{
+    AliasProperty *prop = opaque;
+
+    object_property_get(prop->target_obj, v, prop->target_name, errp);
+}
+
+static void property_set_alias(Object *obj, struct Visitor *v, void *opaque,
+                               const char *name, Error **errp)
+{
+    AliasProperty *prop = opaque;
+
+    object_property_set(prop->target_obj, v, prop->target_name, errp);
+}
+
+static Object *property_resolve_alias(Object *obj, void *opaque,
+                                      const gchar *part)
+{
+    AliasProperty *prop = opaque;
+
+    return object_resolve_path_component(prop->target_obj, prop->target_name);
+}
+
+static void property_release_alias(Object *obj, const char *name, void *opaque)
+{
+    AliasProperty *prop = opaque;
+
+    g_free(prop);
+}
+
+void object_property_add_alias(Object *obj, const char *name,
+                               Object *target_obj, const char *target_name,
+                               Error **errp)
+{
+    AliasProperty *prop;
+    ObjectProperty *target_prop;
+
+    target_prop = object_property_find(target_obj, target_name, errp);
+    if (!target_prop) {
+        return;
+    }
+
+    prop = g_malloc(sizeof(*prop));
+    prop->target_obj = target_obj;
+    prop->target_name = target_name;
+
+    object_property_add_full(obj, name, target_prop->type,
+                             property_get_alias,
+                             property_set_alias,
+                             property_resolve_alias,
+                             property_release_alias,
+                             prop, errp);
+}
+
 static void object_instance_init(Object *obj)
 {
     object_property_add_str(obj, "type", qdev_get_type, NULL, NULL);
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 2/5] qom: add object_property_add_alias()

[Andreas Färber]

Am 11.06.2014 18:49, schrieb Paolo Bonzini:
> From: Stefan Hajnoczi <stefanha@redhat.com>
> 
> Sometimes an object needs to present a property which is actually on
> another object, or it needs to provide an alias name for an existing
> property.
> 
> Examples:
>   a.foo -> b.foo
>   a.old_name -> a.new_name
> 
> The new object_property_add_alias() API allows objects to alias a
> property on the same object or another object.  The source and target
> names can be different.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> Reviewed-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  include/qom/object.h | 20 +++++++++++++++++
>  qom/object.c         | 61 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 81 insertions(+)
> 
> diff --git a/include/qom/object.h b/include/qom/object.h
> index f8ab845..9c4a5a4 100644
> --- a/include/qom/object.h
> +++ b/include/qom/object.h
> @@ -1252,6 +1252,26 @@ void object_property_add_uint64_ptr(Object *obj, const char *name,
>                                      const uint64_t *v, Error **Errp);
>  
>  /**
> + * object_property_add_alias:
> + * @obj: the object to add a property to
> + * @name: the name of the property
> + * @target_obj: the object to forward property access to
> + * @target_name: the name of the property on the forwarded object
> + * @errp: if an error occurs, a pointer to an area to store the error
> + *
> + * Add an alias for a property on an object.  This function will add a property
> + * of the same type as the forwarded property.
> + *
> + * The caller must ensure that <code>@target_obj</code> stays alive as long as
> + * this property exists.  In the case of a child object or an alias on the same
> + * object this will be the case.  For aliases to other objects the caller is
> + * responsible for taking a reference.
> + */
> +void object_property_add_alias(Object *obj, const char *name,
> +                               Object *target_obj, const char *target_name,
> +                               Error **errp);
> +
> +/**
>   * object_child_foreach:
>   * @obj: the object whose children will be navigated
>   * @fn: the iterator function to be called
> diff --git a/qom/object.c b/qom/object.c
> index fcdd0da..e146ae5 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -1532,6 +1532,67 @@ void object_property_add_uint64_ptr(Object *obj, const char *name,
>                          NULL, NULL, (void *)v, errp);
>  }
>  
> +typedef struct
> +{

Coding Style

> +    Object *target_obj;
> +    const char *target_name;
> +} AliasProperty;
> +
> +static void property_get_alias(Object *obj, struct Visitor *v, void *opaque,
> +                               const char *name, Error **errp)
> +{
> +    AliasProperty *prop = opaque;
> +
> +    object_property_get(prop->target_obj, v, prop->target_name, errp);
> +}
> +
> +static void property_set_alias(Object *obj, struct Visitor *v, void *opaque,
> +                               const char *name, Error **errp)
> +{
> +    AliasProperty *prop = opaque;
> +
> +    object_property_set(prop->target_obj, v, prop->target_name, errp);
> +}
> +
> +static Object *property_resolve_alias(Object *obj, void *opaque,
> +                                      const gchar *part)
> +{
> +    AliasProperty *prop = opaque;
> +
> +    return object_resolve_path_component(prop->target_obj, prop->target_name);
> +}
> +
> +static void property_release_alias(Object *obj, const char *name, void *opaque)
> +{
> +    AliasProperty *prop = opaque;
> +
> +    g_free(prop);
> +}
> +
> +void object_property_add_alias(Object *obj, const char *name,
> +                               Object *target_obj, const char *target_name,
> +                               Error **errp)
> +{
> +    AliasProperty *prop;
> +    ObjectProperty *target_prop;
> +
> +    target_prop = object_property_find(target_obj, target_name, errp);
> +    if (!target_prop) {
> +        return;
> +    }
> +
> +    prop = g_malloc(sizeof(*prop));
> +    prop->target_obj = target_obj;
> +    prop->target_name = target_name;
> +
> +    object_property_add_full(obj, name, target_prop->type,
> +                             property_get_alias,
> +                             property_set_alias,
> +                             property_resolve_alias,
> +                             property_release_alias,
> +                             prop, errp);
> +}
> +
>  static void object_instance_init(Object *obj)
>  {
>      object_property_add_str(obj, "type", qdev_get_type, NULL, NULL);

Otherwise looks good to me.

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

[Qemu-devel] [PATCH 3/5] qom: allow creating an alias of a child<> property

[Paolo Bonzini]

An object must have a single parent, so creating an alias of
a child<> property breaks assumptions about objects having
a single canonical path.  Fix this problem by turning these
aliases into links.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 qom/object.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/qom/object.c b/qom/object.c
index e146ae5..ddf781e 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -1575,22 +1575,31 @@ void object_property_add_alias(Object *obj, const char *name,
 {
     AliasProperty *prop;
     ObjectProperty *target_prop;
+    gchar *prop_type;
 
     target_prop = object_property_find(target_obj, target_name, errp);
     if (!target_prop) {
         return;
     }
 
+    if (object_property_is_child(target_prop)) {
+        prop_type = g_strdup_printf("link%s", target_prop->type + 5);
+    } else {
+        prop_type = g_strdup(target_prop->type);
+    }
+
     prop = g_malloc(sizeof(*prop));
     prop->target_obj = target_obj;
     prop->target_name = target_name;
 
-    object_property_add_full(obj, name, target_prop->type,
+    object_property_add_full(obj, name, prop_type,
                              property_get_alias,
                              property_set_alias,
                              property_resolve_alias,
                              property_release_alias,
                              prop, errp);
+
+    g_free(prop_type);
 }
 
 static void object_instance_init(Object *obj)
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 3/5] qom: allow creating an alias of a child<> property

[Peter Crosthwaite]

On Thu, Jun 12, 2014 at 2:49 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> An object must have a single parent, so creating an alias of
> a child<> property breaks assumptions about objects having
> a single canonical path.  Fix this problem by turning these
> aliases into links.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  qom/object.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/qom/object.c b/qom/object.c
> index e146ae5..ddf781e 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -1575,22 +1575,31 @@ void object_property_add_alias(Object *obj, const char *name,
>  {
>      AliasProperty *prop;
>      ObjectProperty *target_prop;
> +    gchar *prop_type;
>
>      target_prop = object_property_find(target_obj, target_name, errp);
>      if (!target_prop) {
>          return;
>      }
>
> +    if (object_property_is_child(target_prop)) {
> +        prop_type = g_strdup_printf("link%s", target_prop->type + 5);

strlen("child") ? Or some comment to explain the magic 5.

Otherwise:

Reviewed-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>

Longer term, should "child" and "link" be macrofied and these
hardcoded strlen's be fixed to avoid difficult developer traps if
anyone ever tries to change the literal strings?

Regards,
Peter


> +    } else {
> +        prop_type = g_strdup(target_prop->type);
> +    }
> +
>      prop = g_malloc(sizeof(*prop));
>      prop->target_obj = target_obj;
>      prop->target_name = target_name;
>
> -    object_property_add_full(obj, name, target_prop->type,
> +    object_property_add_full(obj, name, prop_type,
>                               property_get_alias,
>                               property_set_alias,
>                               property_resolve_alias,
>                               property_release_alias,
>                               prop, errp);
> +
> +    g_free(prop_type);
>  }
>
>  static void object_instance_init(Object *obj)
> --
> 1.8.3.1
>
>
>

Re: [Qemu-devel] [PATCH 3/5] qom: allow creating an alias of a child<> property

[Paolo Bonzini]

Il 17/06/2014 15:28, Peter Crosthwaite ha scritto:
>
> Longer term, should "child" and "link" be macrofied and these
> hardcoded strlen's be fixed to avoid difficult developer traps if
> anyone ever tries to change the literal strings?

Yes, good idea.

Paolo

Re: [Qemu-devel] [PATCH 3/5] qom: allow creating an alias of a child<> property

[Andreas Färber]

Am 17.06.2014 15:31, schrieb Paolo Bonzini:
> Il 17/06/2014 15:28, Peter Crosthwaite ha scritto:
>>
>> Longer term, should "child" and "link" be macrofied and these
>> hardcoded strlen's be fixed to avoid difficult developer traps if
>> anyone ever tries to change the literal strings?
> 
> Yes, good idea.

We do have static inline functions for that somewhere, probably in
object.c. We could move them to the header if needed elsewhere.

Andreas

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

Re: [Qemu-devel] [PATCH 3/5] qom: allow creating an alias of a child<> property

[Andreas Färber]

Am 17.06.2014 15:28, schrieb Peter Crosthwaite:
> On Thu, Jun 12, 2014 at 2:49 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> diff --git a/qom/object.c b/qom/object.c
>> index e146ae5..ddf781e 100644
>> --- a/qom/object.c
>> +++ b/qom/object.c
>> @@ -1575,22 +1575,31 @@ void object_property_add_alias(Object *obj, const char *name,
>>  {
>>      AliasProperty *prop;
>>      ObjectProperty *target_prop;
>> +    gchar *prop_type;
>>
>>      target_prop = object_property_find(target_obj, target_name, errp);
>>      if (!target_prop) {
>>          return;
>>      }
>>
>> +    if (object_property_is_child(target_prop)) {
>> +        prop_type = g_strdup_printf("link%s", target_prop->type + 5);
> 
> strlen("child") ?

+1

> Or some comment to explain the magic 5.
> 
> Otherwise:
> 
> Reviewed-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> 
> Longer term, should "child" and "link" be macrofied and these
> hardcoded strlen's be fixed to avoid difficult developer traps if
> anyone ever tries to change the literal strings?

I think my preference would be a helper function that returns the T from
link<T> (non-dup'ed), which could then here be used as "link<%s>". The
issue with a constant is that elsewhere 6 would be used.

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

Re: [Qemu-devel] [PATCH 3/5] qom: allow creating an alias of a child<> property

[Paolo Bonzini]

Il 17/06/2014 18:37, Andreas Färber ha scritto:
>> >
>> > Longer term, should "child" and "link" be macrofied and these
>> > hardcoded strlen's be fixed to avoid difficult developer traps if
>> > anyone ever tries to change the literal strings?
> I think my preference would be a helper function that returns the T from
> link<T> (non-dup'ed), which could then here be used as "link<%s>".

How can you return it non-dup'ed?  The trailing > gets in the way.

For now I just used strlen("child").

Paolo

Re: [Qemu-devel] [PATCH 3/5] qom: allow creating an alias of a child<> property

[Andreas Färber]

Am 17.06.2014 18:46, schrieb Paolo Bonzini:
> Il 17/06/2014 18:37, Andreas Färber ha scritto:
>>> >
>>> > Longer term, should "child" and "link" be macrofied and these
>>> > hardcoded strlen's be fixed to avoid difficult developer traps if
>>> > anyone ever tries to change the literal strings?
>> I think my preference would be a helper function that returns the T from
>> link<T> (non-dup'ed), which could then here be used as "link<%s>".
> 
> How can you return it non-dup'ed?  The trailing > gets in the way.

Crap, didn't think of >.

> For now I just used strlen("child").

Works for me.

Andreas

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

[Qemu-devel] [PATCH 4/5] qom: allow creating an alias of an object

[Paolo Bonzini]

Add a shorthand for creating an alias of a child<> property.  If you
pass a NULL target_name to object_property_add_alias, the function
will look up the child property that leads to target_obj, and create
an alias for that property.

This can be useful when an object wants to add a link to itself
at a well-known location.  For example, a real-time clock device might
add a link to itself at "/machine/rtc".  Such well-known locations can
then expose a standard set of properties that can be accessed via the
"qom-get" and "qom-set" commands.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 include/qom/object.h | 10 +++++++---
 qom/object.c         | 16 +++++++++++++---
 2 files changed, 20 insertions(+), 6 deletions(-)

diff --git a/include/qom/object.h b/include/qom/object.h
index 9c4a5a4..9cd0ffa 100644
--- a/include/qom/object.h
+++ b/include/qom/object.h
@@ -1256,11 +1256,15 @@ void object_property_add_uint64_ptr(Object *obj, const char *name,
  * @obj: the object to add a property to
  * @name: the name of the property
  * @target_obj: the object to forward property access to
- * @target_name: the name of the property on the forwarded object
+ * @target_name: the name of the property on the forwarded object, or
+ * #NULL to make an object alias.
  * @errp: if an error occurs, a pointer to an area to store the error
  *
- * Add an alias for a property on an object.  This function will add a property
- * of the same type as the forwarded property.
+ * Add an alias property on an object.  This function will add a property
+ * of the same type as the forwarded property or, if @target_name is #NULL,
+ * a link property that always resolves to @target_obj.  In fact, the case
+ * of a #NULL @target_obj actually creates an alias property that targets
+ * @target_obj's own child property.
  *
  * The caller must ensure that <code>@target_obj</code> stays alive as long as
  * this property exists.  In the case of a child object or an alias on the same
diff --git a/qom/object.c b/qom/object.c
index ddf781e..1e8e6af 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -1535,7 +1535,7 @@ void object_property_add_uint64_ptr(Object *obj, const char *name,
 typedef struct
 {
     Object *target_obj;
-    const char *target_name;
+    char *target_name;
 } AliasProperty;
 
 static void property_get_alias(Object *obj, struct Visitor *v, void *opaque,
@@ -1566,6 +1566,7 @@ static void property_release_alias(Object *obj, const char *name, void *opaque)
 {
     AliasProperty *prop = opaque;
 
+    g_free(prop->target_name);
     g_free(prop);
 }
 
@@ -1576,9 +1577,18 @@ void object_property_add_alias(Object *obj, const char *name,
     AliasProperty *prop;
     ObjectProperty *target_prop;
     gchar *prop_type;
+    gchar *the_target_name;
 
-    target_prop = object_property_find(target_obj, target_name, errp);
+    if (!target_name) {
+        the_target_name = object_get_canonical_path_component(target_obj);
+        target_obj = target_obj->parent;
+    } else {
+        the_target_name = g_strdup(target_name);
+    }
+
+    target_prop = object_property_find(target_obj, the_target_name, errp);
     if (!target_prop) {
+        g_free(the_target_name);
         return;
     }
 
@@ -1590,7 +1600,7 @@ void object_property_add_alias(Object *obj, const char *name,
 
     prop = g_malloc(sizeof(*prop));
     prop->target_obj = target_obj;
-    prop->target_name = target_name;
+    prop->target_name = the_target_name;
 
     object_property_add_full(obj, name, prop_type,
                              property_get_alias,
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 4/5] qom: allow creating an alias of an object

[Peter Crosthwaite]

On Thu, Jun 12, 2014 at 2:49 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Add a shorthand for creating an alias of a child<> property.  If you
> pass a NULL target_name to object_property_add_alias, the function
> will look up the child property that leads to target_obj, and create
> an alias for that property.
>
> This can be useful when an object wants to add a link to itself
> at a well-known location.  For example, a real-time clock device might
> add a link to itself at "/machine/rtc". Such well-known locations can
> then expose a standard set of properties that can be accessed via the
> "qom-get" and "qom-set" commands.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  include/qom/object.h | 10 +++++++---
>  qom/object.c         | 16 +++++++++++++---
>  2 files changed, 20 insertions(+), 6 deletions(-)
>
> diff --git a/include/qom/object.h b/include/qom/object.h
> index 9c4a5a4..9cd0ffa 100644
> --- a/include/qom/object.h
> +++ b/include/qom/object.h
> @@ -1256,11 +1256,15 @@ void object_property_add_uint64_ptr(Object *obj, const char *name,
>   * @obj: the object to add a property to
>   * @name: the name of the property
>   * @target_obj: the object to forward property access to
> - * @target_name: the name of the property on the forwarded object
> + * @target_name: the name of the property on the forwarded object, or
> + * #NULL to make an object alias.
>   * @errp: if an error occurs, a pointer to an area to store the error
>   *
> - * Add an alias for a property on an object.  This function will add a property
> - * of the same type as the forwarded property.
> + * Add an alias property on an object.  This function will add a property
> + * of the same type as the forwarded property or, if @target_name is #NULL,
> + * a link property that always resolves to @target_obj.  In fact, the case
> + * of a #NULL @target_obj actually creates an alias property that targets
> + * @target_obj's own child property.
>   *
>   * The caller must ensure that <code>@target_obj</code> stays alive as long as
>   * this property exists.  In the case of a child object or an alias on the same
> diff --git a/qom/object.c b/qom/object.c
> index ddf781e..1e8e6af 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -1535,7 +1535,7 @@ void object_property_add_uint64_ptr(Object *obj, const char *name,
>  typedef struct
>  {
>      Object *target_obj;
> -    const char *target_name;
> +    char *target_name;
>  } AliasProperty;
>
>  static void property_get_alias(Object *obj, struct Visitor *v, void *opaque,
> @@ -1566,6 +1566,7 @@ static void property_release_alias(Object *obj, const char *name, void *opaque)
>  {
>      AliasProperty *prop = opaque;
>
> +    g_free(prop->target_name);
>      g_free(prop);
>  }
>
> @@ -1576,9 +1577,18 @@ void object_property_add_alias(Object *obj, const char *name,
>      AliasProperty *prop;
>      ObjectProperty *target_prop;
>      gchar *prop_type;
> +    gchar *the_target_name;
>
> -    target_prop = object_property_find(target_obj, target_name, errp);
> +    if (!target_name) {
> +        the_target_name = object_get_canonical_path_component(target_obj);
> +        target_obj = target_obj->parent;

This semantic seems a little tricky. It also get the target's
canon-parent entangled in the process whereas your original
object_property_add_alias is more self contained:

http://lists.gnu.org/archive/html/qemu-devel/2014-06/msg01203.html

For instance - could you unparent then reparent an object and have
it's aliases survive? I think the best implementation is to simply
place your original alias logic here under if (!target_name) and
return.

Another possibility, is to add a self-reffing link (perhaps lazily)
from the object to itself ("." would be a good name in spirit with
canon paths looking like file paths) and then alias that.

Note that even though patch 3 prepares for this and my suggestion
would obsolete that, I think P3 still stands in it's own right.

Regards,
Peter

> +    } else {
> +        the_target_name = g_strdup(target_name);
> +    }
> +
> +    target_prop = object_property_find(target_obj, the_target_name, errp);
>      if (!target_prop) {
> +        g_free(the_target_name);
>          return;
>      }
>
> @@ -1590,7 +1600,7 @@ void object_property_add_alias(Object *obj, const char *name,
>
>      prop = g_malloc(sizeof(*prop));
>      prop->target_obj = target_obj;
> -    prop->target_name = target_name;
> +    prop->target_name = the_target_name;
>
>      object_property_add_full(obj, name, prop_type,
>                               property_get_alias,
> --
> 1.8.3.1
>
>
>

Re: [Qemu-devel] [PATCH 4/5] qom: allow creating an alias of an object

[Paolo Bonzini]

Il 17/06/2014 15:55, Peter Crosthwaite ha scritto:
> On Thu, Jun 12, 2014 at 2:49 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> Add a shorthand for creating an alias of a child<> property.  If you
>> pass a NULL target_name to object_property_add_alias, the function
>> will look up the child property that leads to target_obj, and create
>> an alias for that property.
>>
>> This can be useful when an object wants to add a link to itself
>> at a well-known location.  For example, a real-time clock device might
>> add a link to itself at "/machine/rtc". Such well-known locations can
>> then expose a standard set of properties that can be accessed via the
>> "qom-get" and "qom-set" commands.
>>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>>  include/qom/object.h | 10 +++++++---
>>  qom/object.c         | 16 +++++++++++++---
>>  2 files changed, 20 insertions(+), 6 deletions(-)
>>
>> diff --git a/include/qom/object.h b/include/qom/object.h
>> index 9c4a5a4..9cd0ffa 100644
>> --- a/include/qom/object.h
>> +++ b/include/qom/object.h
>> @@ -1256,11 +1256,15 @@ void object_property_add_uint64_ptr(Object *obj, const char *name,
>>   * @obj: the object to add a property to
>>   * @name: the name of the property
>>   * @target_obj: the object to forward property access to
>> - * @target_name: the name of the property on the forwarded object
>> + * @target_name: the name of the property on the forwarded object, or
>> + * #NULL to make an object alias.
>>   * @errp: if an error occurs, a pointer to an area to store the error
>>   *
>> - * Add an alias for a property on an object.  This function will add a property
>> - * of the same type as the forwarded property.
>> + * Add an alias property on an object.  This function will add a property
>> + * of the same type as the forwarded property or, if @target_name is #NULL,
>> + * a link property that always resolves to @target_obj.  In fact, the case
>> + * of a #NULL @target_obj actually creates an alias property that targets
>> + * @target_obj's own child property.
>>   *
>>   * The caller must ensure that <code>@target_obj</code> stays alive as long as
>>   * this property exists.  In the case of a child object or an alias on the same
>> diff --git a/qom/object.c b/qom/object.c
>> index ddf781e..1e8e6af 100644
>> --- a/qom/object.c
>> +++ b/qom/object.c
>> @@ -1535,7 +1535,7 @@ void object_property_add_uint64_ptr(Object *obj, const char *name,
>>  typedef struct
>>  {
>>      Object *target_obj;
>> -    const char *target_name;
>> +    char *target_name;
>>  } AliasProperty;
>>
>>  static void property_get_alias(Object *obj, struct Visitor *v, void *opaque,
>> @@ -1566,6 +1566,7 @@ static void property_release_alias(Object *obj, const char *name, void *opaque)
>>  {
>>      AliasProperty *prop = opaque;
>>
>> +    g_free(prop->target_name);
>>      g_free(prop);
>>  }
>>
>> @@ -1576,9 +1577,18 @@ void object_property_add_alias(Object *obj, const char *name,
>>      AliasProperty *prop;
>>      ObjectProperty *target_prop;
>>      gchar *prop_type;
>> +    gchar *the_target_name;
>>
>> -    target_prop = object_property_find(target_obj, target_name, errp);
>> +    if (!target_name) {
>> +        the_target_name = object_get_canonical_path_component(target_obj);
>> +        target_obj = target_obj->parent;
>
> This semantic seems a little tricky. It also get the target's
> canon-parent entangled in the process whereas your original
> object_property_add_alias is more self contained:
>
> http://lists.gnu.org/archive/html/qemu-devel/2014-06/msg01203.html
>
> For instance - could you unparent then reparent an object and have
> it's aliases survive? I think the best implementation is to simply
> place your original alias logic here under if (!target_name) and
> return.

Ok, so that would mean special casing target_prop == NULL.

Paolo

Re: [Qemu-devel] [PATCH 4/5] qom: allow creating an alias of an object

[Paolo Bonzini]

Il 17/06/2014 16:06, Paolo Bonzini ha scritto:
>>
>> For instance - could you unparent then reparent an object and have
>> it's aliases survive? I think the best implementation is to simply
>> place your original alias logic here under if (!target_name) and
>> return.
>
> Ok, so that would mean special casing target_prop == NULL.

Let's wait for Andreas's opinion.

Paolo

Re: [Qemu-devel] [PATCH 4/5] qom: allow creating an alias of an object

[Peter Crosthwaite]

On Wed, Jun 18, 2014 at 12:06 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 17/06/2014 15:55, Peter Crosthwaite ha scritto:
>
>> On Thu, Jun 12, 2014 at 2:49 AM, Paolo Bonzini <pbonzini@redhat.com>
>> wrote:
>>>
>>> Add a shorthand for creating an alias of a child<> property.  If you
>>> pass a NULL target_name to object_property_add_alias, the function
>>> will look up the child property that leads to target_obj, and create
>>> an alias for that property.
>>>
>>> This can be useful when an object wants to add a link to itself
>>> at a well-known location.  For example, a real-time clock device might
>>> add a link to itself at "/machine/rtc". Such well-known locations can
>>> then expose a standard set of properties that can be accessed via the
>>> "qom-get" and "qom-set" commands.
>>>
>>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>>> ---
>>>  include/qom/object.h | 10 +++++++---
>>>  qom/object.c         | 16 +++++++++++++---
>>>  2 files changed, 20 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/include/qom/object.h b/include/qom/object.h
>>> index 9c4a5a4..9cd0ffa 100644
>>> --- a/include/qom/object.h
>>> +++ b/include/qom/object.h
>>> @@ -1256,11 +1256,15 @@ void object_property_add_uint64_ptr(Object *obj,
>>> const char *name,
>>>   * @obj: the object to add a property to
>>>   * @name: the name of the property
>>>   * @target_obj: the object to forward property access to
>>> - * @target_name: the name of the property on the forwarded object
>>> + * @target_name: the name of the property on the forwarded object, or
>>> + * #NULL to make an object alias.
>>>   * @errp: if an error occurs, a pointer to an area to store the error
>>>   *
>>> - * Add an alias for a property on an object.  This function will add a
>>> property
>>> - * of the same type as the forwarded property.
>>> + * Add an alias property on an object.  This function will add a
>>> property
>>> + * of the same type as the forwarded property or, if @target_name is
>>> #NULL,
>>> + * a link property that always resolves to @target_obj.  In fact, the
>>> case
>>> + * of a #NULL @target_obj actually creates an alias property that
>>> targets
>>> + * @target_obj's own child property.
>>>   *
>>>   * The caller must ensure that <code>@target_obj</code> stays alive as
>>> long as
>>>   * this property exists.  In the case of a child object or an alias on
>>> the same
>>> diff --git a/qom/object.c b/qom/object.c
>>> index ddf781e..1e8e6af 100644
>>> --- a/qom/object.c
>>> +++ b/qom/object.c
>>> @@ -1535,7 +1535,7 @@ void object_property_add_uint64_ptr(Object *obj,
>>> const char *name,
>>>  typedef struct
>>>  {
>>>      Object *target_obj;
>>> -    const char *target_name;
>>> +    char *target_name;
>>>  } AliasProperty;
>>>
>>>  static void property_get_alias(Object *obj, struct Visitor *v, void
>>> *opaque,
>>> @@ -1566,6 +1566,7 @@ static void property_release_alias(Object *obj,
>>> const char *name, void *opaque)
>>>  {
>>>      AliasProperty *prop = opaque;
>>>
>>> +    g_free(prop->target_name);
>>>      g_free(prop);
>>>  }
>>>
>>> @@ -1576,9 +1577,18 @@ void object_property_add_alias(Object *obj, const
>>> char *name,
>>>      AliasProperty *prop;
>>>      ObjectProperty *target_prop;
>>>      gchar *prop_type;
>>> +    gchar *the_target_name;
>>>
>>> -    target_prop = object_property_find(target_obj, target_name, errp);
>>> +    if (!target_name) {
>>> +        the_target_name =
>>> object_get_canonical_path_component(target_obj);
>>> +        target_obj = target_obj->parent;
>>
>>
>> This semantic seems a little tricky. It also get the target's
>> canon-parent entangled in the process whereas your original
>> object_property_add_alias is more self contained:
>>
>> http://lists.gnu.org/archive/html/qemu-devel/2014-06/msg01203.html
>>
>> For instance - could you unparent then reparent an object and have
>> it's aliases survive? I think the best implementation is to simply
>> place your original alias logic here under if (!target_name) and
>> return.
>
>
> Ok, so that would mean special casing target_prop == NULL.
>

target_name == NULL is the special case I think? It is already
something of a special case in this patch with a child to parent
traversal for who is getting a property aliased.

Regards,
Peter

> Paolo
>

Re: [Qemu-devel] [PATCH 4/5] qom: allow creating an alias of an object

[Paolo Bonzini]

Il 17/06/2014 16:16, Peter Crosthwaite ha scritto:
>> > Ok, so that would mean special casing target_prop == NULL.
>> >
> target_name == NULL is the special case I think? It is already
> something of a special case in this patch with a child to parent
> traversal for who is getting a property aliased.

Yeah, but not in the getter/setter/resolver callbacks.  In the end a 
completely different set of functions would be used, which is why I went 
for aliasing the parent's child property.

Paolo

[Qemu-devel] [PATCH 5/5] mc146818rtc: add "rtc" link to "/machine"

[Paolo Bonzini]

From: Marcelo Tosatti <mtosatti@redhat.com>

Add a link to rtc under /machine providing a stable
location for management apps to query "date" field.

{"execute":"qom-get","arguments":{"path":"/machine/rtc","property":"date"} }

Suggested by Paolo Bonzini.

Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: mtosatti@redhat.com <mtosatti@redhat.com>
---
 hw/timer/mc146818rtc.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/hw/timer/mc146818rtc.c b/hw/timer/mc146818rtc.c
index df54546..8c706e1 100644
--- a/hw/timer/mc146818rtc.c
+++ b/hw/timer/mc146818rtc.c
@@ -893,6 +893,9 @@ static void rtc_realizefn(DeviceState *dev, Error **errp)
 
     object_property_add(OBJECT(s), "date", "struct tm",
                         rtc_get_date, NULL, NULL, s, NULL);
+
+    object_property_add_alias(qdev_get_machine(), "rtc",
+                              OBJECT(s), NULL, &error_abort);
 }
 
 ISADevice *rtc_init(ISABus *bus, int base_year, qemu_irq intercept_irq)
@@ -932,11 +935,17 @@ static void rtc_class_initfn(ObjectClass *klass, void *data)
     dc->cannot_instantiate_with_device_add_yet = true;
 }
 
+static void rtc_finalize(Object *obj)
+{
+    object_property_del(qdev_get_machine(), "rtc", NULL);
+}
+
 static const TypeInfo mc146818rtc_info = {
     .name          = TYPE_MC146818_RTC,
     .parent        = TYPE_ISA_DEVICE,
     .instance_size = sizeof(RTCState),
     .class_init    = rtc_class_initfn,
+    .instance_finalize = rtc_finalize,
 };
 
 static void mc146818rtc_register_types(void)
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 5/5] mc146818rtc: add "rtc" link to "/machine"

[Peter Crosthwaite]

On Thu, Jun 12, 2014 at 2:49 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> From: Marcelo Tosatti <mtosatti@redhat.com>
>
> Add a link to rtc under /machine providing a stable
> location for management apps to query "date" field.
>
> {"execute":"qom-get","arguments":{"path":"/machine/rtc","property":"date"} }
>
> Suggested by Paolo Bonzini.
>
> Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: mtosatti@redhat.com <mtosatti@redhat.com>
> ---
>  hw/timer/mc146818rtc.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/hw/timer/mc146818rtc.c b/hw/timer/mc146818rtc.c
> index df54546..8c706e1 100644
> --- a/hw/timer/mc146818rtc.c
> +++ b/hw/timer/mc146818rtc.c
> @@ -893,6 +893,9 @@ static void rtc_realizefn(DeviceState *dev, Error **errp)
>
>      object_property_add(OBJECT(s), "date", "struct tm",
>                          rtc_get_date, NULL, NULL, s, NULL);
> +
> +    object_property_add_alias(qdev_get_machine(), "rtc",
> +                              OBJECT(s), NULL, &error_abort);

This will fail if anyone wants to add two such devices to a machine
model. It seems a bit board specific to assume that this device is
only valid as a singleton. Perhaps s/&error_abort/local_err/ and
raising a warning explaining that only the first RTC in the system
gets the alias?

The other options is arrayification using your (hot off the press)
"[*]" proposal:

+    object_property_add_alias(qdev_get_machine(), "rtc[*]",
+                              OBJECT(s), NULL, &error_abort);

Regards,
Peter

>  }
>
>  ISADevice *rtc_init(ISABus *bus, int base_year, qemu_irq intercept_irq)
> @@ -932,11 +935,17 @@ static void rtc_class_initfn(ObjectClass *klass, void *data)
>      dc->cannot_instantiate_with_device_add_yet = true;
>  }
>
> +static void rtc_finalize(Object *obj)
> +{
> +    object_property_del(qdev_get_machine(), "rtc", NULL);
> +}
> +
>  static const TypeInfo mc146818rtc_info = {
>      .name          = TYPE_MC146818_RTC,
>      .parent        = TYPE_ISA_DEVICE,
>      .instance_size = sizeof(RTCState),
>      .class_init    = rtc_class_initfn,
> +    .instance_finalize = rtc_finalize,
>  };
>
>  static void mc146818rtc_register_types(void)
> --
> 1.8.3.1
>
>

Re: [Qemu-devel] [PATCH 5/5] mc146818rtc: add "rtc" link to "/machine"

[Paolo Bonzini]

Il 17/06/2014 16:09, Peter Crosthwaite ha scritto:
>> > +
>> > +    object_property_add_alias(qdev_get_machine(), "rtc",
>> > +                              OBJECT(s), NULL, &error_abort);
> This will fail if anyone wants to add two such devices to a machine
> model. It seems a bit board specific to assume that this device is
> only valid as a singleton. Perhaps s/&error_abort/local_err/ and
> raising a warning explaining that only the first RTC in the system
> gets the alias?

&error_warn? :)  But then, /machine/rtc.tm is the only supported 
interface, and it should be the same for all RTC devices so perhaps a 
NULL error pointer is enough.

> The other options is arrayification using your (hot off the press)
> "[*]" proposal:
>
> +    object_property_add_alias(qdev_get_machine(), "rtc[*]",
> +                              OBJECT(s), NULL, &error_abort);

This could make sense, but would leave /machine/rtc[0] in the common 
case.  I prefer raising a warning.

Paolo

Re: [Qemu-devel] [PATCH 5/5] mc146818rtc: add "rtc" link to "/machine"

[Peter Crosthwaite]

On Wed, Jun 18, 2014 at 12:12 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 17/06/2014 16:09, Peter Crosthwaite ha scritto:
>
>>> > +
>>> > +    object_property_add_alias(qdev_get_machine(), "rtc",
>>> > +                              OBJECT(s), NULL, &error_abort);
>>
>> This will fail if anyone wants to add two such devices to a machine
>> model. It seems a bit board specific to assume that this device is
>> only valid as a singleton. Perhaps s/&error_abort/local_err/ and
>> raising a warning explaining that only the first RTC in the system
>> gets the alias?
>
>
> &error_warn? :)  But then, /machine/rtc.tm is the only supported interface,

I have thought about &error_warn in the past, but thought the better
of it. Warnings need user digestable message and I can't think of a
sane implementation where you populate a message for error framework
to raise while maintaining the needed errp == NULL || *errp == NULL
semantic at the same time.

> and it should be the same for all RTC devices so perhaps a NULL error
> pointer is enough.
>

NULL Works for me. But other than source verbosity is there any reason
to not catch the error?:

object_property_add_alias(qdev_get_machine(), "rtc",
                              OBJECT(s), NULL, &local_err);
if (local_err) {
    error_report("WARNING: Multiple RTCs in system, not aliasing %s",
object_get_canonical_path(OBJECT(s)));
   /* NOTE: no exit(1) */
}

Regards,
Peter

>
>> The other options is arrayification using your (hot off the press)
>> "[*]" proposal:
>>
>> +    object_property_add_alias(qdev_get_machine(), "rtc[*]",
>> +                              OBJECT(s), NULL, &error_abort);
>
>
> This could make sense, but would leave /machine/rtc[0] in the common case.
> I prefer raising a warning.
>
> Paolo
>

Re: [Qemu-devel] [PATCH 5/5] mc146818rtc: add "rtc" link to "/machine"

[Paolo Bonzini]

Il 17/06/2014 16:25, Peter Crosthwaite ha scritto:
>> > &error_warn? :)  But then, /machine/rtc.tm is the only supported interface,
> I have thought about &error_warn in the past, but thought the better
> of it. Warnings need user digestable message and I can't think of a
> sane implementation where you populate a message for error framework
> to raise while maintaining the needed errp == NULL || *errp == NULL
> semantic at the same time.
>
>> > and it should be the same for all RTC devices so perhaps a NULL error
>> > pointer is enough.
>> >
> NULL Works for me. But other than source verbosity is there any reason
> to not catch the error?:

See above: it doesn't matter _which_ RTC gets the magic alias.  For a 
proper implementation, they should all be the same.

Paolo

Re: [Qemu-devel] [PATCH 5/5] mc146818rtc: add "rtc" link to "/machine"

[Andreas Färber]

Am 17.06.2014 17:01, schrieb Paolo Bonzini:
> Il 17/06/2014 16:25, Peter Crosthwaite ha scritto:
>>> > &error_warn? :)  But then, /machine/rtc.tm is the only supported
>>> interface,
>> I have thought about &error_warn in the past, but thought the better
>> of it. Warnings need user digestable message and I can't think of a
>> sane implementation where you populate a message for error framework
>> to raise while maintaining the needed errp == NULL || *errp == NULL
>> semantic at the same time.
>>
>>> > and it should be the same for all RTC devices so perhaps a NULL error
>>> > pointer is enough.
>>> >
>> NULL Works for me. But other than source verbosity is there any reason
>> to not catch the error?:
> 
> See above: it doesn't matter _which_ RTC gets the magic alias.  For a
> proper implementation, they should all be the same.

Doesn't this "alias" potentially change the canonical path since the new
property's type is going to be child<foo>?

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

Re: [Qemu-devel] [PATCH 5/5] mc146818rtc: add "rtc" link to "/machine"

[Paolo Bonzini]

Il 17/06/2014 18:55, Andreas Färber ha scritto:
>> > See above: it doesn't matter _which_ RTC gets the magic alias.  For a
>> > proper implementation, they should all be the same.
> Doesn't this "alias" potentially change the canonical path since the new
> property's type is going to be child<foo>?

No, patch 3 takes care of this.  The alias property type's is going to 
be link<foo>.

Paolo

Re: [Qemu-devel] [PATCH 5/5] mc146818rtc: add "rtc" link to "/machine"

[Andreas Färber]

Am 11.06.2014 18:49, schrieb Paolo Bonzini:
> From: Marcelo Tosatti <mtosatti@redhat.com>
> 
> Add a link to rtc under /machine providing a stable
> location for management apps to query "date" field.
> 
> {"execute":"qom-get","arguments":{"path":"/machine/rtc","property":"date"} }
> 
> Suggested by Paolo Bonzini.
> 
> Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: mtosatti@redhat.com <mtosatti@redhat.com>

Do those management apps want a generic location for each machine or
just for PC? I know several ARM boards that have different ones on SoC
and on PMIC (I2C) for instance. In that case it may be safer to
implement an interface and to search for that? Or at least give the
"rtc" node a more generic property type for ABI stability? I.e. you only
want "date" here, not any other properties of the mc146818rtc.

> ---
>  hw/timer/mc146818rtc.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/hw/timer/mc146818rtc.c b/hw/timer/mc146818rtc.c
> index df54546..8c706e1 100644
> --- a/hw/timer/mc146818rtc.c
> +++ b/hw/timer/mc146818rtc.c
> @@ -893,6 +893,9 @@ static void rtc_realizefn(DeviceState *dev, Error **errp)
>  
>      object_property_add(OBJECT(s), "date", "struct tm",
>                          rtc_get_date, NULL, NULL, s, NULL);
> +
> +    object_property_add_alias(qdev_get_machine(), "rtc",
> +                              OBJECT(s), NULL, &error_abort);

Irrespective of the error_abort discussion, realize feels too late for
setting up such a property. My suggestion would be to do it in PC
machine init instead - or if not feasible, here in instance_init with
NULL errp.

>  }
>  
>  ISADevice *rtc_init(ISABus *bus, int base_year, qemu_irq intercept_irq)
> @@ -932,11 +935,17 @@ static void rtc_class_initfn(ObjectClass *klass, void *data)
>      dc->cannot_instantiate_with_device_add_yet = true;
>  }
>  
> +static void rtc_finalize(Object *obj)
> +{
> +    object_property_del(qdev_get_machine(), "rtc", NULL);

In Peter's multiple-RTC scenario this means the first RTC unparented(?)
will remove the property, not necessarily the RTC the property points
to. Not really relevant in practice, just pointing out that it's not
just about adding.

Do we need to clean it up at all? And why not just use an
"old-fashioned" link<> property?

Regards,
Andreas

> +}
> +
>  static const TypeInfo mc146818rtc_info = {
>      .name          = TYPE_MC146818_RTC,
>      .parent        = TYPE_ISA_DEVICE,
>      .instance_size = sizeof(RTCState),
>      .class_init    = rtc_class_initfn,
> +    .instance_finalize = rtc_finalize,
>  };
>  
>  static void mc146818rtc_register_types(void)

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

Re: [Qemu-devel] [PATCH 5/5] mc146818rtc: add "rtc" link to "/machine"

[Paolo Bonzini]

Il 17/06/2014 19:09, Andreas Färber ha scritto:
> Do those management apps want a generic location for each machine or
> just for PC?

For machines that implement the RTC_CHANGE event.

> I know several ARM boards that have different ones on SoC
> and on PMIC (I2C) for instance. In that case it may be safer to
> implement an interface and to search for that?

Possibly, but I'm not sure how management can know which types have an 
interface.

If there are many clocks, they all *should* be based on rtc_clock and 
hopefully:

- either only one sends RTC_CHANGE events (right now only the PC's RTC 
sends the event)

- or the OS sets them to the same value when it modifies them, so they 
should have the same date property (but if not, one is as good as any 
other for the purpose of reconstructing lost RTC_CHANGE events).

> Or at least give the
> "rtc" node a more generic property type for ABI stability? I.e. you only
> want "date" here, not any other properties of the mc146818rtc.

The idea is to make a few selected properties "well-known" and link to 
them via aliases, in this case "date" from "/machine/rtc" is the 
well-known property.

Paolo

Re: [Qemu-devel] [PATCH 5/5] mc146818rtc: add "rtc" link to "/machine"

[Andreas Färber]

Am 17.06.2014 19:30, schrieb Paolo Bonzini:
> Il 17/06/2014 19:09, Andreas Färber ha scritto:
>> Or at least give the
>> "rtc" node a more generic property type for ABI stability? I.e. you only
>> want "date" here, not any other properties of the mc146818rtc.
> 
> The idea is to make a few selected properties "well-known" and link to
> them via aliases, in this case "date" from "/machine/rtc" is the
> well-known property.

Now I'm confused. Then why not just alias "date" onto /machine?

Andreas

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

Re: [Qemu-devel] [PATCH 5/5] mc146818rtc: add "rtc" link to "/machine"

[Paolo Bonzini]

Il 17/06/2014 19:38, Andreas Färber ha scritto:
> Am 17.06.2014 19:30, schrieb Paolo Bonzini:
>> Il 17/06/2014 19:09, Andreas Färber ha scritto:
>>> Or at least give the
>>> "rtc" node a more generic property type for ABI stability? I.e. you only
>>> want "date" here, not any other properties of the mc146818rtc.
>>
>> The idea is to make a few selected properties "well-known" and link to
>> them via aliases, in this case "date" from "/machine/rtc" is the
>> well-known property.
>
> Now I'm confused. Then why not just alias "date" onto /machine?

Because we hadn't thought of it? :)  If that's fine for you, all the 
better.  So rtc.date would be aliased as "/machine.rtc_date".  Works for me.

Paolo