Patchwork [v2] Revised: Add object_property_get_child().

login
register
mail settings
Submitter Alexander Barabash
Date Feb. 19, 2012, 4:04 p.m.
Message ID <4F411D76.9020105@mentor.com>
Download mbox | patch
Permalink /patch/142061/
State New
Headers show

Comments

Alexander Barabash - Feb. 19, 2012, 4:04 p.m.
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>

-        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;
          }
Andreas Färber - Feb. 19, 2012, 5:14 p.m.
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
Peter Maydell - Feb. 19, 2012, 6:30 p.m.
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
Alexander Barabash - Feb. 20, 2012, 11:14 a.m.
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.
Alexander Barabash - Feb. 20, 2012, 3:06 p.m.
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

Patch

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