Add object_property_get_child().

   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.

+ */

include/qemu/object.h +4 -0

+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

qom/object.c +9 -0

index 2de6eaf..61e775b 100644
@@ -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);

qom/object.c +25 -0

+        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. */

qom/object.c +20 -0

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

static void object_property_del_child(Object *obj, Object *child, Error
**errp)

 {
     ObjectProperty *prop;

     QTAILQ_FOREACH(prop, &obj->properties, node) {

qom/object.c +22 -1

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

qom/object.c +4 -1

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

qom/object.c +1 -2

@@ -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) {

qom/object.c +3 -1

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

qom/object.c +2 -10

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

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.

Quoted 31 lines

+ */

include/qemu/object.h +4 -0

+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

qom/object.c +11 -0

index 2de6eaf..61e775b 100644
@@ -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, ...)
+{

qom/object.c static void object_property_del_all(Object *obj) · line 317

Quoted 7 lines

qom/object.c +7 -0

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

Quoted 52 lines

qom/object.c +25 -0

+        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. */

qom/object.c +25 -0

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

qom/object.c static void object_property_del_all(Object *obj) · line 373

Quoted 5 lines

qom/object.c +5 -0

+        }
+        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.

Quoted 112 lines

static void object_property_del_child(Object *obj, Object *child, Error
**errp)

  {
      ObjectProperty *prop;

      QTAILQ_FOREACH(prop,&obj->properties, node) {

qom/object.c +22 -1

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

qom/object.c +4 -1

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

qom/object.c +1 -2

@@ -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) {

qom/object.c +3 -1

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

qom/object.c +2 -10

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

qom/object.c +0 -0

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

qom/object.c static Object *object_resolve_partial_path(Object · line 1107

Quoted 8 lines

qom/object.c +1 -1

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

Paolo

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 <[email protected]>

Am 19.02.2012 17:04, schrieb Alexander Barabash:

Quoted 4 lines

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

but does it in a more straightforward way.

Quoted 5 lines · Alexander Barabash

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

Quoted 1 line

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

Add object_property_get_child().

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

Am 19.02.2012 17:04, schrieb Alexander Barabash:

Quoted 2 lines

...

Signed-off-by Alexander Barabash<[email protected]>

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

qom/object.c static void object_property_del_all(Object *obj) · line 349

Quoted 8 lines

+/* Go from LINK_PROPERTY_TYPE_PREFIX FOO LINK_PROPERTY_TYPE_SUFFIX to FOO. */

qom/object.c +6 -0

+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