[Qemu-devel] [PATCH] Add object_property_get_child().

[Qemu-devel] [PATCH] Add object_property_get_child().

[Alexander Barabash]

     Add object_property_get_child().

     Adding a direct accessor to a child property.

     In the existing implementation, object_property_get() must be used,
     with with a visitor, implementing the 'type_str' callback,
     receiving the child's canonical path.

     In the new implementation, the child is returned directly.
     Link properties are resolved in the same way, as they are
     when an absolute path is resolved.

     Errors relevant to the operation, QERR_OBJECT_PROPERTY_NOT_FOUND
     and QERR_OBJECT_PROPERTY_INVALID_TYPE were added.

     Also, in the new implementation, some common sense refactoring was done
     in the file 'qom/object.c' in the code extracting child and link 
properties.

     Signed-off-by: Alexander Barabash <alexander_barabash@mentor.com>

diff --git a/include/qemu/object.h b/include/qemu/object.h
index e7e32fe..190f422 100644
--- a/include/qemu/object.h
+++ b/include/qemu/object.h
@@ -711,6 +711,21 @@ int64_t object_property_get_int(Object *obj, const 
char *name,
                                  struct Error **errp);

  /**
+ * object_property_get_child:
+ * @obj: the object
+ * @name: the name of the property
+ * @errp: returns an error if this function fails
+ *
+ * Returns: if this a child property, the value of the property, or NULL if
+ * an error occurs (including when the property value is not a child 
property).
+ *
+ * Result's reference count does not change.
+ * Therefore, he caller is responsible for referencing the result.
+ */
+Object *object_property_get_child(Object *obj, const char *name,
+                                  struct Error **errp);
+
+/**
   * object_property_set:
   * @obj: the object
   * @v: the visitor that will be used to write the property value.  
This should
diff --git a/qerror.h b/qerror.h
index e26c635..45e4468 100644
--- a/qerror.h
+++ b/qerror.h
@@ -178,6 +178,12 @@ QError *qobject_to_qerror(const QObject *obj);
  #define QERR_NOT_SUPPORTED \
      "{ 'class': 'NotSupported', 'data': {} }"

+#define QERR_OBJECT_PROPERTY_NOT_FOUND \
+    "{ 'class': 'ObjectPropertyNotFound', 'data': { 'object': %s, 
'property': %s } }"
+
+#define QERR_OBJECT_PROPERTY_INVALID_TYPE \
+    "{ 'class': 'ObjectPropertyInvalidType', 'data': { 'object': %s, 
'property': %s, 'expected_type': %s } }"
+
  #define QERR_OPEN_FILE_FAILED \
      "{ 'class': 'OpenFileFailed', 'data': { 'filename': %s } }"

diff --git a/qom/object.c b/qom/object.c
index 2de6eaf..61e775b 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -297,12 +297,86 @@ static void object_property_del_all(Object *obj)
      }
  }

+/*
+ * To ensure correct format checking,
+ * this function should be used only via REPORT_OBJECT_ERROR() macro.
+ *
+ * The first argument after 'obj' should be of type 'const char *'.
+ * It is ignored, and replaced by the canonical path of 'obj'.
+ */
+static void report_object_error(Error **errp, const char *fmt, Object 
*obj, ...)
+    GCC_FMT_ATTR(2, 4);
+static void report_object_error(Error **errp, const char *fmt, Object 
*obj, ...)
+{
+    gchar *path;
+    va_list ap;
+
+    if (errp != NULL) {
+        path = object_get_canonical_path(obj);
+        va_start(ap, obj);
+        va_arg(ap, const char *); /* Ignore the dummy string. */
+        error_set(errp, fmt, path, &ap);
+        va_end(ap);
+        g_free(path);
+    }
+}
+#define REPORT_OBJECT_ERROR(errp, fmt, obj, ...)                 \
+    do {                                                         \
+        report_object_error(errp, fmt, obj, "", ## __VA_ARGS__); \
+    } while (0)
+
+#define CHILD_PROPERTY_TYPE_PREFIX "child<"
+#define CHILD_PROPERTY_TYPE_SUFFIX ">"
+#define LINK_PROPERTY_TYPE_PREFIX "link<"
+#define LINK_PROPERTY_TYPE_SUFFIX ">"
+
+static bool object_property_is_child(ObjectProperty *prop)
+{
+    return (strstart(prop->type, CHILD_PROPERTY_TYPE_PREFIX, NULL) != 0);
+}
+
+static bool object_property_is_link(ObjectProperty *prop)
+{
+    return (strstart(prop->type, LINK_PROPERTY_TYPE_PREFIX, NULL) != 0);
+}
+
+/* Go from LINK_PROPERTY_TYPE_PREFIX FOO LINK_PROPERTY_TYPE_SUFFIX to 
FOO.  */
+static gchar *link_type_to_type(const gchar *type)
+{
+    return g_strndup(&type[sizeof(LINK_PROPERTY_TYPE_PREFIX) - 1],
+                     strlen(type)
+                     - (sizeof(LINK_PROPERTY_TYPE_PREFIX) - 1)
+                     - (sizeof(LINK_PROPERTY_TYPE_SUFFIX) - 1));
+}
+
+static Object *object_property_extract_child(ObjectProperty *prop,
+                                             bool *p_is_invalid_type)
+{
+    if (p_is_invalid_type != NULL) {
+        *p_is_invalid_type = false;
+    }
+    if (object_property_is_child(prop)) {
+        return (Object *)prop->opaque;
+    } else if (object_property_is_link(prop)) {
+        if (prop->opaque != NULL) {
+            return *(Object **)prop->opaque;
+        } else {
+            return NULL;
+        }
+    } else {
+        if (p_is_invalid_type != NULL) {
+            *p_is_invalid_type = true;
+        }
+        return NULL;
+    }
+}
+
  static void object_property_del_child(Object *obj, Object *child, 
Error **errp)
  {
      ObjectProperty *prop;

      QTAILQ_FOREACH(prop, &obj->properties, node) {
-        if (!strstart(prop->type, "child<", NULL)) {
+        if (!object_property_is_child(prop)) {
              continue;
          }

@@ -799,6 +873,27 @@ Object *object_get_root(void)
      return root;
  }

+Object *object_property_get_child(Object *obj, const char *name,
+                                  struct Error **errp) {
+    Object *result = NULL;
+    ObjectProperty *prop = object_property_find(obj, name);
+    bool is_invalid_type;
+
+    if (prop == NULL) {
+        REPORT_OBJECT_ERROR(errp, QERR_OBJECT_PROPERTY_NOT_FOUND, obj, 
name);
+        return NULL;
+    }
+
+    result = object_property_extract_child(prop, &is_invalid_type);
+    if (is_invalid_type) {
+        REPORT_OBJECT_ERROR(errp, QERR_OBJECT_PROPERTY_INVALID_TYPE,
+                            obj, name, "child");
+        return NULL;
+    }
+
+    return result;
+}
+
  static void object_get_child_property(Object *obj, Visitor *v, void 
*opaque,
                                        const char *name, Error **errp)
  {
@@ -829,7 +924,10 @@ void object_property_add_child(Object *obj, const 
char *name,
       */
      assert(!object_is_type(obj, type_interface));

-    type = g_strdup_printf("child<%s>", 
object_get_typename(OBJECT(child)));
+    type = g_strdup_printf(CHILD_PROPERTY_TYPE_PREFIX
+                           "%s"
+                           CHILD_PROPERTY_TYPE_SUFFIX,
+                           object_get_typename(OBJECT(child)));

      object_property_add(obj, name, type, object_get_child_property,
                          NULL, object_finalize_child_property, child, 
errp);
@@ -878,8 +976,7 @@ static void object_set_link_property(Object *obj, 
Visitor *v, void *opaque,
      if (strcmp(path, "") != 0) {
          Object *target;

-        /* Go from link<FOO> to FOO.  */
-        target_type = g_strndup(&type[5], strlen(type) - 6);
+        target_type = link_type_to_type(type);
          target = object_resolve_path_type(path, target_type, &ambiguous);

          if (ambiguous) {
@@ -907,7 +1004,9 @@ void object_property_add_link(Object *obj, const 
char *name,
  {
      gchar *full_type;

-    full_type = g_strdup_printf("link<%s>", type);
+    full_type = g_strdup_printf(LINK_PROPERTY_TYPE_PREFIX
+                                "%s"
+                                LINK_PROPERTY_TYPE_SUFFIX, type);

      object_property_add(obj, name, full_type,
                          object_get_link_property,
@@ -932,7 +1031,7 @@ gchar *object_get_canonical_path(Object *obj)
          g_assert(obj->parent != NULL);

          QTAILQ_FOREACH(prop, &obj->parent->properties, node) {
-            if (!strstart(prop->type, "child<", NULL)) {
+            if (!object_property_is_child(prop)) {
                  continue;
              }

@@ -980,15 +1079,7 @@ static Object *object_resolve_abs_path(Object *parent,
          return NULL;
      }

-    child = NULL;
-    if (strstart(prop->type, "link<", NULL)) {
-        Object **pchild = prop->opaque;
-        if (*pchild) {
-            child = *pchild;
-        }
-    } else if (strstart(prop->type, "child<", NULL)) {
-        child = prop->opaque;
-    }
+    child = object_property_extract_child(prop, NULL);

      if (!child) {
          return NULL;
@@ -1010,7 +1101,7 @@ static Object *object_resolve_partial_path(Object 
*parent,
      QTAILQ_FOREACH(prop, &parent->properties, node) {
          Object *found;

-        if (!strstart(prop->type, "child<", NULL)) {
+        if (!object_property_is_child(prop)) {
              continue;
          }

Re: [Qemu-devel] [PATCH] Add object_property_get_child().

[Peter Maydell]

On 16 February 2012 17:39, Alexander Barabash
<alexander_barabash@mentor.com> wrote:
>
>    Add object_property_get_child().
>
>    Adding a direct accessor to a child property.
>
>    In the existing implementation, object_property_get() must be used,
>    with with a visitor, implementing the 'type_str' callback,
>    receiving the child's canonical path.
>
>    In the new implementation, the child is returned directly.
>    Link properties are resolved in the same way, as they are
>    when an absolute path is resolved.
>
>    Errors relevant to the operation, QERR_OBJECT_PROPERTY_NOT_FOUND
>    and QERR_OBJECT_PROPERTY_INVALID_TYPE were added.

Not trivial (look at the length of it!).

>    Also, in the new implementation, some common sense refactoring was done
>    in the file 'qom/object.c' in the code extracting child and link
> properties.

If you find yourself writing a commit message with a paragraph
starting "Also" this almost always means you should split it
into more than one patch.

-- PMM

Re: [Qemu-devel] [PATCH] Add object_property_get_child().

[Paolo Bonzini]

Besides the non-triviality of the patch, how is this different from
object_property_get_link?  Perhaps we should just rename that one to
object_property_get_obj, or add a function that is just a synonym.

> + */
> +Object *object_property_get_child(Object *obj, const char *name,
> +                                  struct Error **errp);
> +
> +/**
>   * object_property_set:
>   * @obj: the object
>   * @v: the visitor that will be used to write the property value.  This
> should
> diff --git a/qom/object.c b/qom/object.c
> index 2de6eaf..61e775b 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -297,12 +297,86 @@ static void object_property_del_all(Object *obj)
>      }
>  }
> 
> +/*
> + * To ensure correct format checking,
> + * this function should be used only via REPORT_OBJECT_ERROR() macro.
> + *
> + * The first argument after 'obj' should be of type 'const char *'.
> + * It is ignored, and replaced by the canonical path of 'obj'.
> + */
> +static void report_object_error(Error **errp, const char *fmt, Object
> *obj, ...)
> +    GCC_FMT_ATTR(2, 4);
> +static void report_object_error(Error **errp, const char *fmt, Object
> *obj, ...)
> +{
> +    gchar *path;
> +    va_list ap;
> +
> +    if (errp != NULL) {
> +        path = object_get_canonical_path(obj);
> +        va_start(ap, obj);
> +        va_arg(ap, const char *); /* Ignore the dummy string. */

Why do you need it at all?

> +        error_set(errp, fmt, path, &ap);
> +        va_end(ap);
> +        g_free(path);
> +    }
> +}
> +#define REPORT_OBJECT_ERROR(errp, fmt, obj, ...)                 \
> +    do {                                                         \
> +        report_object_error(errp, fmt, obj, "", ## __VA_ARGS__); \
> +    } while (0)
> +#define CHILD_PROPERTY_TYPE_PREFIX "child<"
> +#define CHILD_PROPERTY_TYPE_SUFFIX ">"
> +#define LINK_PROPERTY_TYPE_PREFIX "link<"
> +#define LINK_PROPERTY_TYPE_SUFFIX ">"
> +
> +static bool object_property_is_child(ObjectProperty *prop)
> +{
> +    return (strstart(prop->type, CHILD_PROPERTY_TYPE_PREFIX, NULL) != 0);
> +}
> +
> +static bool object_property_is_link(ObjectProperty *prop)
> +{
> +    return (strstart(prop->type, LINK_PROPERTY_TYPE_PREFIX, NULL) != 0);
> +}
> +
> +/* Go from LINK_PROPERTY_TYPE_PREFIX FOO LINK_PROPERTY_TYPE_SUFFIX to
> FOO.  */
> +static gchar *link_type_to_type(const gchar *type)
> +{
> +    return g_strndup(&type[sizeof(LINK_PROPERTY_TYPE_PREFIX) - 1],
> +                     strlen(type)
> +                     - (sizeof(LINK_PROPERTY_TYPE_PREFIX) - 1)
> +                     - (sizeof(LINK_PROPERTY_TYPE_SUFFIX) - 1));
> +}
> +
> +static Object *object_property_extract_child(ObjectProperty *prop,
> +                                             bool *p_is_invalid_type)
> +{
> +    if (p_is_invalid_type != NULL) {
> +        *p_is_invalid_type = false;
> +    }
> +    if (object_property_is_child(prop)) {
> +        return (Object *)prop->opaque;
> +    } else if (object_property_is_link(prop)) {
> +        if (prop->opaque != NULL) {
> +            return *(Object **)prop->opaque;
> +        } else {
> +            return NULL;
> +        }
> +    } else {
> +        if (p_is_invalid_type != NULL) {
> +            *p_is_invalid_type = true;
> +        }
> +        return NULL;
> +    }
> +}
> +

object_property_get_child should never be on a fast path.  We should
avoid as much as possible peeking in the opaque.  Instead, you should
just use a visitor like object_property_get_link does.

Everything starting here should be a separate patch.  But if you remove
object_property_extract_child, I doubt it makes as much sense to
refactor this.

>  static void object_property_del_child(Object *obj, Object *child, Error
> **errp)
>  {
>      ObjectProperty *prop;
> 
>      QTAILQ_FOREACH(prop, &obj->properties, node) {
> -        if (!strstart(prop->type, "child<", NULL)) {
> +        if (!object_property_is_child(prop)) {
>              continue;
>          }
> 
> @@ -799,6 +873,27 @@ Object *object_get_root(void)
>      return root;
>  }
> 
> +Object *object_property_get_child(Object *obj, const char *name,
> +                                  struct Error **errp) {
> +    Object *result = NULL;
> +    ObjectProperty *prop = object_property_find(obj, name);
> +    bool is_invalid_type;
> +
> +    if (prop == NULL) {
> +        REPORT_OBJECT_ERROR(errp, QERR_OBJECT_PROPERTY_NOT_FOUND, obj,
> name);
> +        return NULL;
> +    }
> +
> +    result = object_property_extract_child(prop, &is_invalid_type);
> +    if (is_invalid_type) {
> +        REPORT_OBJECT_ERROR(errp, QERR_OBJECT_PROPERTY_INVALID_TYPE,
> +                            obj, name, "child");
> +        return NULL;
> +    }
> +
> +    return result;
> +}
> +
>  static void object_get_child_property(Object *obj, Visitor *v, void
> *opaque,
>                                        const char *name, Error **errp)
>  {
> @@ -829,7 +924,10 @@ void object_property_add_child(Object *obj, const
> char *name,
>       */
>      assert(!object_is_type(obj, type_interface));
> 
> -    type = g_strdup_printf("child<%s>",
> object_get_typename(OBJECT(child)));
> +    type = g_strdup_printf(CHILD_PROPERTY_TYPE_PREFIX
> +                           "%s"
> +                           CHILD_PROPERTY_TYPE_SUFFIX,
> +                           object_get_typename(OBJECT(child)));
> 
>      object_property_add(obj, name, type, object_get_child_property,
>                          NULL, object_finalize_child_property, child,
> errp);
> @@ -878,8 +976,7 @@ static void object_set_link_property(Object *obj,
> Visitor *v, void *opaque,
>      if (strcmp(path, "") != 0) {
>          Object *target;
> 
> -        /* Go from link<FOO> to FOO.  */
> -        target_type = g_strndup(&type[5], strlen(type) - 6);
> +        target_type = link_type_to_type(type);
>          target = object_resolve_path_type(path, target_type, &ambiguous);
> 
>          if (ambiguous) {
> @@ -907,7 +1004,9 @@ void object_property_add_link(Object *obj, const
> char *name,
>  {
>      gchar *full_type;
> 
> -    full_type = g_strdup_printf("link<%s>", type);
> +    full_type = g_strdup_printf(LINK_PROPERTY_TYPE_PREFIX
> +                                "%s"
> +                                LINK_PROPERTY_TYPE_SUFFIX, type);
> 
>      object_property_add(obj, name, full_type,
>                          object_get_link_property,
> @@ -932,7 +1031,7 @@ gchar *object_get_canonical_path(Object *obj)
>          g_assert(obj->parent != NULL);
> 
>          QTAILQ_FOREACH(prop, &obj->parent->properties, node) {
> -            if (!strstart(prop->type, "child<", NULL)) {
> +            if (!object_property_is_child(prop)) {
>                  continue;
>              }
> 
> @@ -980,15 +1079,7 @@ static Object *object_resolve_abs_path(Object
> *parent,
>          return NULL;
>      }
> 
> -    child = NULL;
> -    if (strstart(prop->type, "link<", NULL)) {
> -        Object **pchild = prop->opaque;
> -        if (*pchild) {
> -            child = *pchild;
> -        }
> -    } else if (strstart(prop->type, "child<", NULL)) {
> -        child = prop->opaque;
> -    }
> +    child = object_property_extract_child(prop, NULL);
> 
>      if (!child) {
>          return NULL;
> @@ -1010,7 +1101,7 @@ static Object *object_resolve_partial_path(Object
> *parent,
>      QTAILQ_FOREACH(prop, &parent->properties, node) {
>          Object *found;
> 
> -        if (!strstart(prop->type, "child<", NULL)) {
> +        if (!object_property_is_child(prop)) {
>              continue;
>          }
> 
> 
> 
> 

Paolo

Re: [Qemu-devel] [PATCH] Add object_property_get_child().

[Alexander Barabash]

On 02/17/2012 12:17 PM, Paolo Bonzini wrote:
> Besides the non-triviality of the patch, how is this different from
> object_property_get_link?  Perhaps we should just rename that one to
> object_property_get_obj, or add a function that is just a synonym.
As for the patch's (non-)triviality, I just wanted to check what counts 
as trivial here.
I agree that was a bit over-reaching.

The proposed object_property_get_child() may return either
the direct child with the specified name in the composition tree,
or the value of the link with the specified name, as
object_property_get_link() indeed does.

It has exactly the same semantics as:

Object *object_property_get_child(Object *obj, const char *name,
                                   struct Error **errp)
{
     Object * result;
     bool ambigious;
     gchar *child_path;

     /* If name contains a '/' in it, report an error and return NULL. */

     child_path = g_strdup_printf("%s/%s", 
object_get_canonical_path(obj), name);
     result = object_resolve_path(child_path, &ambigious);
     g_free(child_path);
     if (result == NULL) {
        /* Report an error. */
        return NULL;
     }
     if (ambigious) {
        /* Report an error. */
        return NULL;
     }
     return result;
}

but does it in a more straightforward way.

>
>> + */
>> +Object *object_property_get_child(Object *obj, const char *name,
>> +                                  struct Error **errp);
>> +
>> +/**
>>    * object_property_set:
>>    * @obj: the object
>>    * @v: the visitor that will be used to write the property value.  This
>> should
>> diff --git a/qom/object.c b/qom/object.c
>> index 2de6eaf..61e775b 100644
>> --- a/qom/object.c
>> +++ b/qom/object.c
>> @@ -297,12 +297,86 @@ static void object_property_del_all(Object *obj)
>>       }
>>   }
>>
>> +/*
>> + * To ensure correct format checking,
>> + * this function should be used only via REPORT_OBJECT_ERROR() macro.
>> + *
>> + * The first argument after 'obj' should be of type 'const char *'.
>> + * It is ignored, and replaced by the canonical path of 'obj'.
>> + */
>> +static void report_object_error(Error **errp, const char *fmt, Object
>> *obj, ...)
>> +    GCC_FMT_ATTR(2, 4);
>> +static void report_object_error(Error **errp, const char *fmt, Object
>> *obj, ...)
>> +{
>> +    gchar *path;
>> +    va_list ap;
>> +
>> +    if (errp != NULL) {
>> +        path = object_get_canonical_path(obj);
>> +        va_start(ap, obj);
>> +        va_arg(ap, const char *); /* Ignore the dummy string. */
> Why do you need it at all?

I think this is needed to avoid replicating code and have an easy tool 
to report
object's path in errors.

>
>> +        error_set(errp, fmt, path,&ap);
>> +        va_end(ap);
>> +        g_free(path);
>> +    }
>> +}
>> +#define REPORT_OBJECT_ERROR(errp, fmt, obj, ...)                 \
>> +    do {                                                         \
>> +        report_object_error(errp, fmt, obj, "", ## __VA_ARGS__); \
>> +    } while (0)
>> +#define CHILD_PROPERTY_TYPE_PREFIX "child<"
>> +#define CHILD_PROPERTY_TYPE_SUFFIX ">"
>> +#define LINK_PROPERTY_TYPE_PREFIX "link<"
>> +#define LINK_PROPERTY_TYPE_SUFFIX ">"
>> +
>> +static bool object_property_is_child(ObjectProperty *prop)
>> +{
>> +    return (strstart(prop->type, CHILD_PROPERTY_TYPE_PREFIX, NULL) != 0);
>> +}
>> +
>> +static bool object_property_is_link(ObjectProperty *prop)
>> +{
>> +    return (strstart(prop->type, LINK_PROPERTY_TYPE_PREFIX, NULL) != 0);
>> +}
>> +
>> +/* Go from LINK_PROPERTY_TYPE_PREFIX FOO LINK_PROPERTY_TYPE_SUFFIX to
>> FOO.  */
>> +static gchar *link_type_to_type(const gchar *type)
>> +{
>> +    return g_strndup(&type[sizeof(LINK_PROPERTY_TYPE_PREFIX) - 1],
>> +                     strlen(type)
>> +                     - (sizeof(LINK_PROPERTY_TYPE_PREFIX) - 1)
>> +                     - (sizeof(LINK_PROPERTY_TYPE_SUFFIX) - 1));
>> +}
>> +
>> +static Object *object_property_extract_child(ObjectProperty *prop,
>> +                                             bool *p_is_invalid_type)
>> +{
>> +    if (p_is_invalid_type != NULL) {
>> +        *p_is_invalid_type = false;
>> +    }
>> +    if (object_property_is_child(prop)) {
>> +        return (Object *)prop->opaque;
>> +    } else if (object_property_is_link(prop)) {
>> +        if (prop->opaque != NULL) {
>> +            return *(Object **)prop->opaque;
>> +        } else {
>> +            return NULL;
>> +        }
>> +    } else {
>> +        if (p_is_invalid_type != NULL) {
>> +            *p_is_invalid_type = true;
>> +        }
>> +        return NULL;
>> +    }
>> +}
>> +
> object_property_get_child should never be on a fast path.  We should
> avoid as much as possible peeking in the opaque.  Instead, you should
> just use a visitor like object_property_get_link does.

I am not sure what do you mean by "should never be on a fast path".
Anyway, I did not add any peeking in the opaque. I just moved the code
from object_resolve_abs_path() into a subroutine. Fairly enough, we could
avoid looking into the link property's opaque by calling 
object_property_get_link()
instead. I'll prepare a revision of the patch to this end.


>
> Everything starting here should be a separate patch.  But if you remove
> object_property_extract_child, I doubt it makes as much sense to
> refactor this.

Obviously, the refactoring was needed to add code. If we don't,
no refactoring is required.

>
>>   static void object_property_del_child(Object *obj, Object *child, Error
>> **errp)
>>   {
>>       ObjectProperty *prop;
>>
>>       QTAILQ_FOREACH(prop,&obj->properties, node) {
>> -        if (!strstart(prop->type, "child<", NULL)) {
>> +        if (!object_property_is_child(prop)) {
>>               continue;
>>           }
>>
>> @@ -799,6 +873,27 @@ Object *object_get_root(void)
>>       return root;
>>   }
>>
>> +Object *object_property_get_child(Object *obj, const char *name,
>> +                                  struct Error **errp) {
>> +    Object *result = NULL;
>> +    ObjectProperty *prop = object_property_find(obj, name);
>> +    bool is_invalid_type;
>> +
>> +    if (prop == NULL) {
>> +        REPORT_OBJECT_ERROR(errp, QERR_OBJECT_PROPERTY_NOT_FOUND, obj,
>> name);
>> +        return NULL;
>> +    }
>> +
>> +    result = object_property_extract_child(prop,&is_invalid_type);
>> +    if (is_invalid_type) {
>> +        REPORT_OBJECT_ERROR(errp, QERR_OBJECT_PROPERTY_INVALID_TYPE,
>> +                            obj, name, "child");
>> +        return NULL;
>> +    }
>> +
>> +    return result;
>> +}
>> +
>>   static void object_get_child_property(Object *obj, Visitor *v, void
>> *opaque,
>>                                         const char *name, Error **errp)
>>   {
>> @@ -829,7 +924,10 @@ void object_property_add_child(Object *obj, const
>> char *name,
>>        */
>>       assert(!object_is_type(obj, type_interface));
>>
>> -    type = g_strdup_printf("child<%s>",
>> object_get_typename(OBJECT(child)));
>> +    type = g_strdup_printf(CHILD_PROPERTY_TYPE_PREFIX
>> +                           "%s"
>> +                           CHILD_PROPERTY_TYPE_SUFFIX,
>> +                           object_get_typename(OBJECT(child)));
>>
>>       object_property_add(obj, name, type, object_get_child_property,
>>                           NULL, object_finalize_child_property, child,
>> errp);
>> @@ -878,8 +976,7 @@ static void object_set_link_property(Object *obj,
>> Visitor *v, void *opaque,
>>       if (strcmp(path, "") != 0) {
>>           Object *target;
>>
>> -        /* Go from link<FOO>  to FOO.  */
>> -        target_type = g_strndup(&type[5], strlen(type) - 6);
>> +        target_type = link_type_to_type(type);
>>           target = object_resolve_path_type(path, target_type,&ambiguous);
>>
>>           if (ambiguous) {
>> @@ -907,7 +1004,9 @@ void object_property_add_link(Object *obj, const
>> char *name,
>>   {
>>       gchar *full_type;
>>
>> -    full_type = g_strdup_printf("link<%s>", type);
>> +    full_type = g_strdup_printf(LINK_PROPERTY_TYPE_PREFIX
>> +                                "%s"
>> +                                LINK_PROPERTY_TYPE_SUFFIX, type);
>>
>>       object_property_add(obj, name, full_type,
>>                           object_get_link_property,
>> @@ -932,7 +1031,7 @@ gchar *object_get_canonical_path(Object *obj)
>>           g_assert(obj->parent != NULL);
>>
>>           QTAILQ_FOREACH(prop,&obj->parent->properties, node) {
>> -            if (!strstart(prop->type, "child<", NULL)) {
>> +            if (!object_property_is_child(prop)) {
>>                   continue;
>>               }
>>
>> @@ -980,15 +1079,7 @@ static Object *object_resolve_abs_path(Object
>> *parent,
>>           return NULL;
>>       }
>>
>> -    child = NULL;
>> -    if (strstart(prop->type, "link<", NULL)) {
>> -        Object **pchild = prop->opaque;
>> -        if (*pchild) {
>> -            child = *pchild;
>> -        }
>> -    } else if (strstart(prop->type, "child<", NULL)) {
>> -        child = prop->opaque;
>> -    }
>> +    child = object_property_extract_child(prop, NULL);
>>
>>       if (!child) {
>>           return NULL;
>> @@ -1010,7 +1101,7 @@ static Object *object_resolve_partial_path(Object
>> *parent,
>>       QTAILQ_FOREACH(prop,&parent->properties, node) {
>>           Object *found;
>>
>> -        if (!strstart(prop->type, "child<", NULL)) {
>> +        if (!object_property_is_child(prop)) {
>>               continue;
>>           }
>>
>>
>>
>>
> Paolo
Alex

[Qemu-devel] [PATCH v2] Revised: Add object_property_get_child().

[Alexander Barabash]

     Add object_property_get_child().

     Adding a direct accessor to a child property.

     In the existing implementation, object_property_get() must be used,
     with with a visitor, implementing the 'type_str' callback,
     receiving the child's canonical path.

     In the new implementation, the child is returned directly.
     For link properties, object_property_get_link() is used
     to resolve the link.

     Also, in the new implementation, object_property_get_child() is used
     as a subroutine of object_resolve_abs_path(). This changes the way
     object_resolve_abs_path() operates, moving away from directly peeking
     the property's 'opaque' field to using object_property_get_link().

     Thus, in the mew implementation link properties are resolved in the 
same way,
     as they are when an absolute path is resolved.

     Errors relevant to the operation, QERR_OBJECT_PROPERTY_NOT_FOUND
     and QERR_OBJECT_PROPERTY_INVALID_TYPE were added.

     Also, in the new implementation, some common sense refactoring was done
     in the file 'qom/object.c' in the code extracting child and link 
properties.

     Signed-off-by: Alexander Barabash <alexander_barabash@mentor.com>

diff --git a/include/qemu/object.h b/include/qemu/object.h
index ba2409d..001521d 100644
--- a/include/qemu/object.h
+++ b/include/qemu/object.h
@@ -711,6 +711,21 @@ int64_t object_property_get_int(Object *obj, const 
char *name,
                                  struct Error **errp);

  /**
+ * object_property_get_child:
+ * @obj: the object
+ * @name: the name of the property
+ * @errp: returns an error if this function fails
+ *
+ * Returns: if this a child property, the value of the property, or NULL if
+ * an error occurs (including when the property value is not a child 
property).
+ *
+ * Result's reference count does not change.
+ * Therefore, he caller is responsible for referencing the result.
+ */
+Object *object_property_get_child(Object *obj, const char *name,
+                                  struct Error **errp);
+
+/**
   * object_property_set:
   * @obj: the object
   * @v: the visitor that will be used to write the property value.  
This should
diff --git a/qerror.h b/qerror.h
index e26c635..45e4468 100644
--- a/qerror.h
+++ b/qerror.h
@@ -178,6 +178,12 @@ QError *qobject_to_qerror(const QObject *obj);
  #define QERR_NOT_SUPPORTED \
      "{ 'class': 'NotSupported', 'data': {} }"

+#define QERR_OBJECT_PROPERTY_NOT_FOUND \
+    "{ 'class': 'ObjectPropertyNotFound', 'data': { 'object': %s, 
'property': %s } }"
+
+#define QERR_OBJECT_PROPERTY_INVALID_TYPE \
+    "{ 'class': 'ObjectPropertyInvalidType', 'data': { 'object': %s, 
'property': %s, 'expected_type': %s } }"
+
  #define QERR_OPEN_FILE_FAILED \
      "{ 'class': 'OpenFileFailed', 'data': { 'filename': %s } }"

diff --git a/qom/object.c b/qom/object.c
index 2de6eaf..ddd19e1 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -297,12 +297,64 @@ static void object_property_del_all(Object *obj)
      }
  }

+/*
+ * To ensure correct format checking,
+ * this function should be used only via REPORT_OBJECT_ERROR() macro.
+ *
+ * The first argument after 'obj' should be of type 'const char *'.
+ * It is ignored, and replaced by the canonical path of 'obj'.
+ */
+static void report_object_error(Error **errp, const char *fmt, Object 
*obj, ...)
+    GCC_FMT_ATTR(2, 4);
+static void report_object_error(Error **errp, const char *fmt, Object 
*obj, ...)
+{
+    gchar *path;
+    va_list ap;
+
+    if (errp != NULL) {
+        path = object_get_canonical_path(obj);
+        va_start(ap, obj);
+        va_arg(ap, const char *); /* Ignore the dummy string. */
+        error_set(errp, fmt, path, &ap);
+        va_end(ap);
+        g_free(path);
+    }
+}
+#define REPORT_OBJECT_ERROR(errp, fmt, obj, ...)                 \
+    do {                                                         \
+        report_object_error(errp, fmt, obj, "", ## __VA_ARGS__); \
+    } while (0)
+
+#define CHILD_PROPERTY_TYPE_PREFIX "child<"
+#define CHILD_PROPERTY_TYPE_SUFFIX ">"
+#define LINK_PROPERTY_TYPE_PREFIX "link<"
+#define LINK_PROPERTY_TYPE_SUFFIX ">"
+
+static bool object_property_is_child(ObjectProperty *prop)
+{
+    return (strstart(prop->type, CHILD_PROPERTY_TYPE_PREFIX, NULL) != 0);
+}
+
+static bool object_property_is_link(ObjectProperty *prop)
+{
+    return (strstart(prop->type, LINK_PROPERTY_TYPE_PREFIX, NULL) != 0);
+}
+
+/* Go from LINK_PROPERTY_TYPE_PREFIX FOO LINK_PROPERTY_TYPE_SUFFIX to 
FOO.  */
+static gchar *link_type_to_type(const gchar *type)
+{
+    return g_strndup(&type[sizeof(LINK_PROPERTY_TYPE_PREFIX) - 1],
+                     strlen(type)
+                     - (sizeof(LINK_PROPERTY_TYPE_PREFIX) - 1)
+                     - (sizeof(LINK_PROPERTY_TYPE_SUFFIX) - 1));
+}
+
  static void object_property_del_child(Object *obj, Object *child, 
Error **errp)
  {
      ObjectProperty *prop;

      QTAILQ_FOREACH(prop, &obj->properties, node) {
-        if (!strstart(prop->type, "child<", NULL)) {
+        if (!object_property_is_child(prop)) {
              continue;
          }

@@ -799,6 +851,29 @@ Object *object_get_root(void)
      return root;
  }

+Object *object_property_get_child(Object *obj, const char *name,
+                                  struct Error **errp) {
+    Object *result;
+    ObjectProperty *prop = object_property_find(obj, name);
+
+    if (prop == NULL) {
+        REPORT_OBJECT_ERROR(errp, QERR_OBJECT_PROPERTY_NOT_FOUND, obj, 
name);
+        return NULL;
+    }
+
+    if (object_property_is_child(prop)) {
+        result = (Object *)prop->opaque;
+    } else if (object_property_is_link(prop)) {
+        result = object_property_get_link(obj, name, errp);
+    } else {
+        result = NULL;
+        REPORT_OBJECT_ERROR(errp, QERR_OBJECT_PROPERTY_INVALID_TYPE,
+                            obj, name, "child/link");
+    }
+
+    return result;
+}
+
  static void object_get_child_property(Object *obj, Visitor *v, void 
*opaque,
                                        const char *name, Error **errp)
  {
@@ -829,7 +904,10 @@ void object_property_add_child(Object *obj, const 
char *name,
       */
      assert(!object_is_type(obj, type_interface));

-    type = g_strdup_printf("child<%s>", 
object_get_typename(OBJECT(child)));
+    type = g_strdup_printf(CHILD_PROPERTY_TYPE_PREFIX
+                           "%s"
+                           CHILD_PROPERTY_TYPE_SUFFIX,
+                           object_get_typename(OBJECT(child)));

      object_property_add(obj, name, type, object_get_child_property,
                          NULL, object_finalize_child_property, child, 
errp);
@@ -878,8 +956,7 @@ static void object_set_link_property(Object *obj, 
Visitor *v, void *opaque,
      if (strcmp(path, "") != 0) {
          Object *target;

-        /* Go from link<FOO> to FOO.  */
-        target_type = g_strndup(&type[5], strlen(type) - 6);
+        target_type = link_type_to_type(type);
          target = object_resolve_path_type(path, target_type, &ambiguous);

          if (ambiguous) {
@@ -907,7 +984,9 @@ void object_property_add_link(Object *obj, const 
char *name,
  {
      gchar *full_type;

-    full_type = g_strdup_printf("link<%s>", type);
+    full_type = g_strdup_printf(LINK_PROPERTY_TYPE_PREFIX
+                                "%s"
+                                LINK_PROPERTY_TYPE_SUFFIX, type);

      object_property_add(obj, name, full_type,
                          object_get_link_property,
@@ -932,7 +1011,7 @@ gchar *object_get_canonical_path(Object *obj)
          g_assert(obj->parent != NULL);

          QTAILQ_FOREACH(prop, &obj->parent->properties, node) {
-            if (!strstart(prop->type, "child<", NULL)) {
+            if (!object_property_is_child(prop)) {
                  continue;
              }

@@ -964,7 +1043,6 @@ static Object *object_resolve_abs_path(Object *parent,
                                            const char *typename,
                                            int index)
  {
-    ObjectProperty *prop;
      Object *child;

      if (parts[index] == NULL) {
@@ -975,20 +1053,7 @@ static Object *object_resolve_abs_path(Object *parent,
          return object_resolve_abs_path(parent, parts, typename, index 
+ 1);
      }

-    prop = object_property_find(parent, parts[index]);
-    if (prop == NULL) {
-        return NULL;
-    }
-
-    child = NULL;
-    if (strstart(prop->type, "link<", NULL)) {
-        Object **pchild = prop->opaque;
-        if (*pchild) {
-            child = *pchild;
-        }
-    } else if (strstart(prop->type, "child<", NULL)) {
-        child = prop->opaque;
-    }
+    child = object_property_get_child(parent, parts[index], NULL);

      if (!child) {
          return NULL;
@@ -1010,7 +1075,7 @@ static Object *object_resolve_partial_path(Object 
*parent,
      QTAILQ_FOREACH(prop, &parent->properties, node) {
          Object *found;

-        if (!strstart(prop->type, "child<", NULL)) {
+        if (!object_property_is_child(prop)) {
              continue;
          }

Re: [Qemu-devel] [PATCH v2] Revised: Add object_property_get_child().

[Andreas Färber]

Am 19.02.2012 17:04, schrieb Alexander Barabash:
> 
>     Add object_property_get_child().
> 
>     Adding a direct accessor to a child property.
> 
>     In the existing implementation, object_property_get() must be used,
>     with with a visitor, implementing the 'type_str' callback,
>     receiving the child's canonical path.
> 
>     In the new implementation, the child is returned directly.
>     For link properties, object_property_get_link() is used
>     to resolve the link.
> 
>     Also, in the new implementation, object_property_get_child() is used
>     as a subroutine of object_resolve_abs_path(). This changes the way
>     object_resolve_abs_path() operates, moving away from directly peeking
>     the property's 'opaque' field to using object_property_get_link().
> 
>     Thus, in the mew implementation link properties are resolved in the
> same way,
>     as they are when an absolute path is resolved.
> 
>     Errors relevant to the operation, QERR_OBJECT_PROPERTY_NOT_FOUND
>     and QERR_OBJECT_PROPERTY_INVALID_TYPE were added.
> 
>     Also, in the new implementation, some common sense refactoring was done
>     in the file 'qom/object.c' in the code extracting child and link
> properties.
> 
>     Signed-off-by: Alexander Barabash <alexander_barabash@mentor.com>

Please use git-send-email to submit your patches: The commit message is
unnecessarily indented and the first line is duplicated. Instead of
"Revised: " (which v2 already indicates) the subject should mention the
topic, here "qom: ".

http://wiki.qemu.org/Contribute/SubmitAPatch

Your patch is doing too many things at once. Please split it up into a
series of easily digestable and bisectable patches, e.g.:
* ..._PREFIX/SUFFIX introduction and use in _add_child/link,
* ..._is_child/_is_link introduction and use in
_property_del_child/_get_canonical_path/_resolve_partial_path,
* link_type_to_type() and use in _set_link_property,
* REPORT_OBJECT_ERROR(),
* object_property_get_child().

> +/* Go from LINK_PROPERTY_TYPE_PREFIX FOO LINK_PROPERTY_TYPE_SUFFIX to
> FOO.  */
> +static gchar *link_type_to_type(const gchar *type)
> +{
> +    return g_strndup(&type[sizeof(LINK_PROPERTY_TYPE_PREFIX) - 1],
> +                     strlen(type)
> +                     - (sizeof(LINK_PROPERTY_TYPE_PREFIX) - 1)
> +                     - (sizeof(LINK_PROPERTY_TYPE_SUFFIX) - 1));

Any reason not to use strlen()? I don't think this is a hot path, and
repeated sizeof() - 1 does not read so nice. The alternative would be to
hardcode the offsets.

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 v2] Revised: Add object_property_get_child().

[Peter Maydell]

On 19 February 2012 17:14, Andreas Färber <afaerber@suse.de> wrote:
> Am 19.02.2012 17:04, schrieb Alexander Barabash:
>> +    return g_strndup(&type[sizeof(LINK_PROPERTY_TYPE_PREFIX) - 1],
>> +                     strlen(type)
>> +                     - (sizeof(LINK_PROPERTY_TYPE_PREFIX) - 1)
>> +                     - (sizeof(LINK_PROPERTY_TYPE_SUFFIX) - 1));
>
> Any reason not to use strlen()? I don't think this is a hot path, and
> repeated sizeof() - 1 does not read so nice.

gcc will optimise away strlen("constant string") at compile time anyway,
so there really is no need to avoid it.

-- PMM

Re: [Qemu-devel] [PATCH v2] Revised: Add object_property_get_child().

[Alexander Barabash]

On 02/19/2012 07:14 PM, Andreas Färber wrote:
> Am 19.02.2012 17:04, schrieb Alexander Barabash:
>> ...
>>      Signed-off-by: Alexander Barabash<alexander_barabash@mentor.com>
> Please use git-send-email to submit your patches: The commit message is
> unnecessarily indented and the first line is duplicated. Instead of
> "Revised: " (which v2 already indicates) the subject should mention the
> topic, here "qom: ".
>
> http://wiki.qemu.org/Contribute/SubmitAPatch

Thanks for the advice.

>
> Your patch is doing too many things at once. Please split it up into a
> series of easily digestable and bisectable patches, e.g.:
> * ..._PREFIX/SUFFIX introduction and use in _add_child/link,
> * ..._is_child/_is_link introduction and use in
> _property_del_child/_get_canonical_path/_resolve_partial_path,
> * link_type_to_type() and use in _set_link_property,
> * REPORT_OBJECT_ERROR(),
> * object_property_get_child().

OK

>> +/* Go from LINK_PROPERTY_TYPE_PREFIX FOO LINK_PROPERTY_TYPE_SUFFIX to
>> FOO.  */
>> +static gchar *link_type_to_type(const gchar *type)
>> +{
>> +    return g_strndup(&type[sizeof(LINK_PROPERTY_TYPE_PREFIX) - 1],
>> +                     strlen(type)
>> +                     - (sizeof(LINK_PROPERTY_TYPE_PREFIX) - 1)
>> +                     - (sizeof(LINK_PROPERTY_TYPE_SUFFIX) - 1));
> Any reason not to use strlen()? I don't think this is a hot path, and
> repeated sizeof() - 1 does not read so nice. The alternative would be to
> hardcode the offsets.

I replaced "5" (i.e. the length "link<") with 
"sizeof(LINK_PROPERTY_TYPE_PREFIX) - 1".
If we assume that "strlen("link<") is always optimized away,
then certainly using strlen is equivalent and looks better.

>
> Andreas
>
Alex

Re: [Qemu-devel] [PATCH v2] Revised: Add object_property_get_child().

[Alexander Barabash]

On 02/19/2012 06:04 PM, Alexander Barabash wrote:
>     Add object_property_get_child().
Please disregard this patch. object_property_get_link() works for this 
purpose.

Re: [Qemu-devel] [PATCH] Add object_property_get_child().

[Paolo Bonzini]

On 02/19/2012 01:36 PM, Alexander Barabash wrote:
> 
> 
> The proposed object_property_get_child() may return either
> the direct child with the specified name in the composition tree,
> or the value of the link with the specified name, as
> object_property_get_link() indeed does.

Have you actually checked that object_property_get_link() doesn't work
for you?  It seems to me that your patch does the same as the one-liner:

#define object_property_get_child object_property_get_link

Now _this_ would be good for qemu-trivial. ;)

> but does it in a more straightforward way. 

I disagree that it is more straightforward.  It is more *direct*
perhaps, in that it doesn't go through visitors,
object_get_child_property and object_resolve_path, but it also violates
the encapsulation that properties provide quite heavily.

Peeking at the opaque values of properties should be done as sparingly
as possible.  Ideally once, since no QOM property code should be in a
fast path.  In this case, it is done in object_resolve_abs_path, and
that should be enough.

Paolo

Re: [Qemu-devel] [PATCH] Add object_property_get_child().

[Alexander Barabash]

On 02/20/2012 11:11 AM, Paolo Bonzini wrote:
> On 02/19/2012 01:36 PM, Alexander Barabash wrote:
>>
>> The proposed object_property_get_child() may return either
>> the direct child with the specified name in the composition tree,
>> or the value of the link with the specified name, as
>> object_property_get_link() indeed does.
> Have you actually checked that object_property_get_link() doesn't work
> for you?  It seems to me that your patch does the same as the one-liner:
>
> #define object_property_get_child object_property_get_link
>
> Now _this_ would be good for qemu-trivial. ;)

You are right. Please disregard my v2 patch.

I would only suggest that the documentation of object_property_add_child()
be amended to reflect the fact that the child object may be retrieved using
object_property_get_link().

>> but does it in a more straightforward way.
> I disagree that it is more straightforward.  It is more *direct*
> perhaps, in that it doesn't go through visitors,
> object_get_child_property and object_resolve_path, but it also violates
> the encapsulation that properties provide quite heavily.
>
> Peeking at the opaque values of properties should be done as sparingly
> as possible.  Ideally once, since no QOM property code should be in a
> fast path.  In this case, it is done in object_resolve_abs_path, and
> that should be enough.
>
> Paolo
Alex

Re: [Qemu-devel] [PATCH] Add object_property_get_child().

[Paolo Bonzini]

On 02/20/2012 12:13 PM, Alexander Barabash wrote:
> I would only suggest that the documentation of object_property_add_child()
> be amended to reflect the fact that the child object may be retrieved using
> object_property_get_link().

Sure, feel free to post a patch for that or to add a synonym with
#define.  Alternatively, amending the documentation of
object_property_get_link or even renaming it would also be fine, I guess.

Paolo