diff mbox

[4/7] qom: add object_new_propv / object_new_proplist constructors

Message ID 1430404944-7322-5-git-send-email-berrange@redhat.com
State New
Headers show

Commit Message

Daniel P. Berrangé April 30, 2015, 2:42 p.m. UTC
It is reasonably common to want to create an object, set a
number of properties, register it in the hierarchy and then
mark it as complete (if a user creatable type). This requires
quite a lot of error prone, verbose, boilerplate code to achieve.

The object_new_propv / object_new_proplist constructors will
simplify this task by performing all required steps in one go,
accepting the property names/values as variadic args.

Usage would be:

   Error *err = NULL;
   Object *obj;
   obj = object_new_propv(TYPE_MEMORY_BACKEND_FILE,
                          "/objects",
                          "hostmem0",
                          &err,
                          "share", "yes",
                          "mem-path", "/dev/shm/somefile",
                          "prealloc", "yes",
                          "size", "1048576",
                          NULL);

Note all property values are passed in string form and will
be parsed into their required data types.

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 include/qom/object.h       |  64 +++++++++++++++
 qom/object.c               |  67 ++++++++++++++++
 tests/.gitignore           |   1 +
 tests/Makefile             |   5 +-
 tests/check-qom-proplist.c | 190 +++++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 326 insertions(+), 1 deletion(-)
 create mode 100644 tests/check-qom-proplist.c

Comments

Paolo Bonzini April 30, 2015, 2:53 p.m. UTC | #1
On 30/04/2015 16:42, Daniel P. Berrange wrote:
> + * object_new_propv:
> + * @typename:  The name of the type of the object to instantiate.
> + * @path: the object path to register under
> + * @id: The unique ID of the object
> + * @errp: pointer to error object
> + * @...: list of property names and values
> + *
> + * This function with initialize a new object using heap allocated memory.
> + * The returned object has a reference count of 1, and will be freed when
> + * the last reference is dropped.
> + *
> + * The @id parameter will be used to register the object in the /objects
> + * hierarchy.
> + *
> + * The variadic parameters are a list of pairs of (propname, propvalue)
> + * strings. The propname of NULL indicates the end of the property
> + * list. If the object implements the user creatable interface, the
> + * object will be marked complete once all the properties have been
> + * processed.
> + *
> + *   Error *err = NULL;
> + *   Object *obj;
> + *
> + *   obj = object_new_propv(TYPE_MEMORY_BACKEND_FILE,
> + *                          "/objects",

Only one thing I missed in my previous review---I'd rather make the path
an object, you can easily get it through container_get("/path").

I can fix this when committing the patches.

Paolo
Daniel P. Berrangé April 30, 2015, 2:54 p.m. UTC | #2
On Thu, Apr 30, 2015 at 04:53:27PM +0200, Paolo Bonzini wrote:
> 
> 
> On 30/04/2015 16:42, Daniel P. Berrange wrote:
> > + * object_new_propv:
> > + * @typename:  The name of the type of the object to instantiate.
> > + * @path: the object path to register under
> > + * @id: The unique ID of the object
> > + * @errp: pointer to error object
> > + * @...: list of property names and values
> > + *
> > + * This function with initialize a new object using heap allocated memory.
> > + * The returned object has a reference count of 1, and will be freed when
> > + * the last reference is dropped.
> > + *
> > + * The @id parameter will be used to register the object in the /objects
> > + * hierarchy.
> > + *
> > + * The variadic parameters are a list of pairs of (propname, propvalue)
> > + * strings. The propname of NULL indicates the end of the property
> > + * list. If the object implements the user creatable interface, the
> > + * object will be marked complete once all the properties have been
> > + * processed.
> > + *
> > + *   Error *err = NULL;
> > + *   Object *obj;
> > + *
> > + *   obj = object_new_propv(TYPE_MEMORY_BACKEND_FILE,
> > + *                          "/objects",
> 
> Only one thing I missed in my previous review---I'd rather make the path
> an object, you can easily get it through container_get("/path").
> 
> I can fix this when committing the patches.

Oh, i misunderstood - I thought you only wanted the path. I'm fine with
the API either way.

Regards,
Daniel
Paolo Bonzini April 30, 2015, 3 p.m. UTC | #3
On 30/04/2015 16:42, Daniel P. Berrange wrote:
> +    propname = va_arg(vargs, char *);
> +    while (propname != NULL) {
> +        const char *value = va_arg(vargs, char *);
> +
> +        g_assert(value != NULL);
> +        object_property_parse(obj, value, propname, errp);
> +        if (*errp) {
> +            goto error;
> +        }
> +        propname = va_arg(vargs, char *);
> +    }
> +
> +    object_property_add_child(container_get(object_get_root(), path),
> +                              id, obj, errp);
> +    if (*errp) {
> +        goto error;
> +    }
> +
> +    if (object_dynamic_cast(obj, TYPE_USER_CREATABLE)) {
> +        user_creatable_complete(obj, errp);
> +        if (*errp) {
> +            object_property_del(container_get(object_get_root(), path),
> +                                id, &error_abort);

Easier:

    object_unparent(obj);

> +            goto error;
> +        }
> +    }
> +
> +    return obj;

And another one: do we want to unref the object here, since we've added
a stable reference via object_property_add_child?

Do you prefer me too apply patches 1-2-3-5-6-7, minus the testcases, or
do you simply want to resubmit?

Paolo
Daniel P. Berrangé April 30, 2015, 3:02 p.m. UTC | #4
On Thu, Apr 30, 2015 at 05:00:28PM +0200, Paolo Bonzini wrote:
> 
> 
> On 30/04/2015 16:42, Daniel P. Berrange wrote:
> > +    propname = va_arg(vargs, char *);
> > +    while (propname != NULL) {
> > +        const char *value = va_arg(vargs, char *);
> > +
> > +        g_assert(value != NULL);
> > +        object_property_parse(obj, value, propname, errp);
> > +        if (*errp) {
> > +            goto error;
> > +        }
> > +        propname = va_arg(vargs, char *);
> > +    }
> > +
> > +    object_property_add_child(container_get(object_get_root(), path),
> > +                              id, obj, errp);
> > +    if (*errp) {
> > +        goto error;
> > +    }
> > +
> > +    if (object_dynamic_cast(obj, TYPE_USER_CREATABLE)) {
> > +        user_creatable_complete(obj, errp);
> > +        if (*errp) {
> > +            object_property_del(container_get(object_get_root(), path),
> > +                                id, &error_abort);
> 
> Easier:
> 
>     object_unparent(obj);
> 
> > +            goto error;
> > +        }
> > +    }
> > +
> > +    return obj;
> 
> And another one: do we want to unref the object here, since we've added
> a stable reference via object_property_add_child?
> 
> Do you prefer me too apply patches 1-2-3-5-6-7, minus the testcases, or
> do you simply want to resubmit?

I'll resubmit in a short while.

Regards,
Daniel
diff mbox

Patch

diff --git a/include/qom/object.h b/include/qom/object.h
index d2d7748..6365b91 100644
--- a/include/qom/object.h
+++ b/include/qom/object.h
@@ -607,6 +607,70 @@  Object *object_new(const char *typename);
 Object *object_new_with_type(Type type);
 
 /**
+ * object_new_propv:
+ * @typename:  The name of the type of the object to instantiate.
+ * @path: the object path to register under
+ * @id: The unique ID of the object
+ * @errp: pointer to error object
+ * @...: list of property names and values
+ *
+ * This function with initialize a new object using heap allocated memory.
+ * The returned object has a reference count of 1, and will be freed when
+ * the last reference is dropped.
+ *
+ * The @id parameter will be used to register the object in the /objects
+ * hierarchy.
+ *
+ * The variadic parameters are a list of pairs of (propname, propvalue)
+ * strings. The propname of NULL indicates the end of the property
+ * list. If the object implements the user creatable interface, the
+ * object will be marked complete once all the properties have been
+ * processed.
+ *
+ *   Error *err = NULL;
+ *   Object *obj;
+ *
+ *   obj = object_new_propv(TYPE_MEMORY_BACKEND_FILE,
+ *                          "/objects",
+ *                          "hostmem0",
+ *                          &err,
+ *                          "share", "yes",
+ *                          "mem-path", "/dev/shm/somefile",
+ *                          "prealloc", "yes",
+ *                          "size", "1048576",
+ *                          NULL);
+ *
+ *   if (!obj) {
+ *     g_printerr("Cannot create memory backend: %s\n",
+ *                error_get_pretty(err));
+ *   }
+ *
+ * Returns: The newly allocated, instantiated & initialized object.
+ */
+Object *object_new_propv(const char *typename,
+                         const char *path,
+                         const char *id,
+                         Error **errp,
+                         ...)
+    __attribute__((sentinel));
+
+/**
+ * object_new_proplist:
+ * @typename:  The name of the type of the object to instantiate.
+ * @path: the object path to register under
+ * @id: The unique ID of the object
+ * @errp: pointer to error object
+ * @vargs: list of property names and values
+ *
+ * See object_new_propv for documentation.
+ */
+Object *object_new_proplist(const char *typename,
+                            const char *path,
+                            const char *id,
+                            Error **errp,
+                            va_list vargs);
+
+/**
  * object_initialize_with_type:
  * @data: A pointer to the memory to be used for the object.
  * @size: The maximum size available at @data for the object.
diff --git a/qom/object.c b/qom/object.c
index b8dff43..8927261 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -11,6 +11,7 @@ 
  */
 
 #include "qom/object.h"
+#include "qom/object_interfaces.h"
 #include "qemu-common.h"
 #include "qapi/visitor.h"
 #include "qapi-visit.h"
@@ -439,6 +440,72 @@  Object *object_new(const char *typename)
     return object_new_with_type(ti);
 }
 
+Object *object_new_propv(const char *typename,
+                         const char *path,
+                         const char *id,
+                         Error **errp,
+                         ...)
+{
+    va_list vargs;
+    Object *obj;
+
+    va_start(vargs, errp);
+    obj = object_new_proplist(typename, path, id, errp, vargs);
+    va_end(vargs);
+
+    return obj;
+}
+
+Object *object_new_proplist(const char *typename,
+                            const char *path,
+                            const char *id,
+                            Error **errp,
+                            va_list vargs)
+{
+    Object *obj;
+    const char *propname;
+
+    obj = object_new(typename);
+
+    if (object_class_is_abstract(object_get_class(obj))) {
+        error_setg(errp, "object type '%s' is abstract", typename);
+        goto error;
+    }
+
+    propname = va_arg(vargs, char *);
+    while (propname != NULL) {
+        const char *value = va_arg(vargs, char *);
+
+        g_assert(value != NULL);
+        object_property_parse(obj, value, propname, errp);
+        if (*errp) {
+            goto error;
+        }
+        propname = va_arg(vargs, char *);
+    }
+
+    object_property_add_child(container_get(object_get_root(), path),
+                              id, obj, errp);
+    if (*errp) {
+        goto error;
+    }
+
+    if (object_dynamic_cast(obj, TYPE_USER_CREATABLE)) {
+        user_creatable_complete(obj, errp);
+        if (*errp) {
+            object_property_del(container_get(object_get_root(), path),
+                                id, &error_abort);
+            goto error;
+        }
+    }
+
+    return obj;
+
+ error:
+    object_unref(obj);
+    return NULL;
+}
+
 Object *object_dynamic_cast(Object *obj, const char *typename)
 {
     if (obj && object_class_dynamic_cast(object_get_class(obj), typename)) {
diff --git a/tests/.gitignore b/tests/.gitignore
index 0dcb618..dc813c2 100644
--- a/tests/.gitignore
+++ b/tests/.gitignore
@@ -5,6 +5,7 @@  check-qjson
 check-qlist
 check-qstring
 check-qom-interface
+check-qom-proplist
 rcutorture
 test-aio
 test-bitops
diff --git a/tests/Makefile b/tests/Makefile
index 309e869..e0a831c 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -68,6 +68,8 @@  check-unit-y += tests/test-bitops$(EXESUF)
 check-unit-$(CONFIG_HAS_GLIB_SUBPROCESS_TESTS) += tests/test-qdev-global-props$(EXESUF)
 check-unit-y += tests/check-qom-interface$(EXESUF)
 gcov-files-check-qom-interface-y = qom/object.c
+check-unit-y += tests/check-qom-proplist$(EXESUF)
+gcov-files-check-qom-proplist-y = qom/object.c
 check-unit-y += tests/test-qemu-opts$(EXESUF)
 gcov-files-test-qemu-opts-y = qom/test-qemu-opts.c
 check-unit-y += tests/test-write-threshold$(EXESUF)
@@ -240,7 +242,7 @@  test-qapi-obj-y = tests/test-qapi-visit.o tests/test-qapi-types.o \
 
 $(test-obj-y): QEMU_INCLUDES += -Itests
 QEMU_CFLAGS += -I$(SRC_PATH)/tests
-qom-core-obj = qom/object.o qom/qom-qobject.o qom/container.o
+qom-core-obj = qom/object.o qom/qom-qobject.o qom/container.o qom/object_interfaces.o
 
 tests/check-qint$(EXESUF): tests/check-qint.o libqemuutil.a
 tests/check-qstring$(EXESUF): tests/check-qstring.o libqemuutil.a
@@ -249,6 +251,7 @@  tests/check-qlist$(EXESUF): tests/check-qlist.o libqemuutil.a
 tests/check-qfloat$(EXESUF): tests/check-qfloat.o libqemuutil.a
 tests/check-qjson$(EXESUF): tests/check-qjson.o libqemuutil.a libqemustub.a
 tests/check-qom-interface$(EXESUF): tests/check-qom-interface.o $(qom-core-obj) libqemuutil.a libqemustub.a
+tests/check-qom-proplist$(EXESUF): tests/check-qom-proplist.o $(qom-core-obj) libqemuutil.a libqemustub.a
 tests/test-coroutine$(EXESUF): tests/test-coroutine.o $(block-obj-y) libqemuutil.a libqemustub.a
 tests/test-aio$(EXESUF): tests/test-aio.o $(block-obj-y) libqemuutil.a libqemustub.a
 tests/test-rfifolock$(EXESUF): tests/test-rfifolock.o libqemuutil.a libqemustub.a
diff --git a/tests/check-qom-proplist.c b/tests/check-qom-proplist.c
new file mode 100644
index 0000000..9a02e4d
--- /dev/null
+++ b/tests/check-qom-proplist.c
@@ -0,0 +1,190 @@ 
+/*
+ * Copyright (C) 2015 Red Hat, Inc.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library.  If not, see
+ * <http://www.gnu.org/licenses/>.
+ *
+ * Author: Daniel P. Berrange <berrange@redhat.com>
+ */
+
+#include <glib.h>
+
+#include "qom/object.h"
+#include "qemu/module.h"
+
+
+#define TYPE_DUMMY "qemu:dummy"
+
+typedef struct DummyObject DummyObject;
+typedef struct DummyObjectClass DummyObjectClass;
+
+#define DUMMY_OBJECT(obj)                               \
+    OBJECT_CHECK(DummyObject, (obj), TYPE_DUMMY)
+
+struct DummyObject {
+    Object parent;
+
+    bool bv;
+    char *sv;
+};
+
+struct DummyObjectClass {
+    ObjectClass parent;
+};
+
+
+static void dummy_set_bv(Object *obj,
+                         bool value,
+                         Error **errp)
+{
+    DummyObject *dobj = DUMMY_OBJECT(obj);
+
+    dobj->bv = value;
+}
+
+static bool dummy_get_bv(Object *obj,
+                         Error **errp)
+{
+    DummyObject *dobj = DUMMY_OBJECT(obj);
+
+    return dobj->bv;
+}
+
+
+static void dummy_set_sv(Object *obj,
+                         const char *value,
+                         Error **errp)
+{
+    DummyObject *dobj = DUMMY_OBJECT(obj);
+
+    g_free(dobj->sv);
+    dobj->sv = g_strdup(value);
+}
+
+static char *dummy_get_sv(Object *obj,
+                          Error **errp)
+{
+    DummyObject *dobj = DUMMY_OBJECT(obj);
+
+    return g_strdup(dobj->sv);
+}
+
+
+static void dummy_init(Object *obj)
+{
+    object_property_add_bool(obj, "bv",
+                             dummy_get_bv,
+                             dummy_set_bv,
+                             NULL);
+    object_property_add_str(obj, "sv",
+                            dummy_get_sv,
+                            dummy_set_sv,
+                            NULL);
+}
+
+static void dummy_finalize(Object *obj)
+{
+    DummyObject *dobj = DUMMY_OBJECT(obj);
+
+    g_free(dobj->sv);
+}
+
+
+static const TypeInfo dummy_info = {
+    .name          = TYPE_DUMMY,
+    .parent        = TYPE_OBJECT,
+    .instance_size = sizeof(DummyObject),
+    .instance_init = dummy_init,
+    .instance_finalize = dummy_finalize,
+    .class_size = sizeof(DummyObjectClass),
+};
+
+static void test_dummy_createv(void)
+{
+    Error *err = NULL;
+    DummyObject *dobj = DUMMY_OBJECT(
+        object_new_propv(TYPE_DUMMY,
+                         "/objects",
+                         "dummy0",
+                         &err,
+                         "bv", "yes",
+                         "sv", "Hiss hiss hiss",
+                         NULL));
+
+    g_assert(dobj != NULL);
+    g_assert(err == NULL);
+    g_assert(g_str_equal(dobj->sv, "Hiss hiss hiss"));
+    g_assert(dobj->bv == true);
+
+    g_assert(object_resolve_path_component(
+                 container_get(object_get_root(),
+                               "/objects"),
+                 "dummy0") == OBJECT(dobj));
+
+    object_unparent(OBJECT(dobj));
+    object_unref(OBJECT(dobj));
+}
+
+
+static Object *new_helper(Error **errp,
+                          ...)
+{
+    va_list vargs;
+    Object *obj;
+
+    va_start(vargs, errp);
+    obj = object_new_proplist(TYPE_DUMMY,
+                              "/objects",
+                              "dummy0",
+                              errp,
+                              vargs);
+    va_end(vargs);
+    return obj;
+}
+
+static void test_dummy_createlist(void)
+{
+    Error *err = NULL;
+    DummyObject *dobj = DUMMY_OBJECT(
+        new_helper(&err,
+                   "bv", "yes",
+                   "sv", "Hiss hiss hiss",
+                   NULL));
+
+    g_assert(dobj != NULL);
+    g_assert(err == NULL);
+    g_assert(g_str_equal(dobj->sv, "Hiss hiss hiss"));
+    g_assert(dobj->bv == true);
+
+    g_assert(object_resolve_path_component(
+                 container_get(object_get_root(),
+                               "/objects"),
+                 "dummy0") == OBJECT(dobj));
+
+    object_unparent(OBJECT(dobj));
+    object_unref(OBJECT(dobj));
+}
+
+int main(int argc, char **argv)
+{
+    g_test_init(&argc, &argv, NULL);
+
+    module_call_init(MODULE_INIT_QOM);
+    type_register_static(&dummy_info);
+
+    g_test_add_func("/qom/proplist/createlist", test_dummy_createlist);
+    g_test_add_func("/qom/proplist/createv", test_dummy_createv);
+
+    return g_test_run();
+}