Re: [Qemu-devel] [PATCH v3 4/7] qom: add object_new_propv / object_new_proplist constructors

[Andreas Färber]

Hi Daniel/Paolo,

Am 01.05.2015 um 12:30 schrieb Daniel P. Berrange:
> 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.

With this I disagree. I can see the virtue of adding properties in one
go via some handy varargs function. But,

1) The function does something different from what its name implies to
me. It does not create a prop or proplist - instead of adding them it
sets existing ones. Suggest object_new_with_props()?

2) You seem to mix up *v and non-v functions. v is with va_list usually,
compare tests/libqtest.h.

3) Object construction is a tricky thing to get right. Anthony chose to
be stricter than C++ and not let object_new() fail, one of the reasons
we have the distinct realize step. Can we keep the two separate? qdev
with all its convenience helpers didn't mix those either.
I.e., use object_new() without Error** followed by object_set_props() or
anything with Error**. That tells you if there's an Error* you need to
unref the object. Otherwise it's in an unknown state.

4) What's the use case for this? I'm concerned about encouraging people
to hardcode properties like this, when doing it in C can let the
compiler detect any mismatches.

> 
> Usage would be:
> 
>    Error *err = NULL;
>    Object *obj;
>    obj = object_new_propv(TYPE_MEMORY_BACKEND_FILE,
>                           "/objects",

This is not an Object*. ;) I like it better as it's implemented below,
but cf. above for mixing this Error**-ing operation with object_new().

>                           "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 <address@hidden>
> ---
>  include/qom/object.h       |  67 ++++++++++++++++
>  qom/object.c               |  66 ++++++++++++++++
>  tests/.gitignore           |   1 +
>  tests/Makefile             |   5 +-
>  tests/check-qom-proplist.c | 190 
> +++++++++++++++++++++++++++++++++++++++++++++
>  5 files changed, 328 insertions(+), 1 deletion(-)
>  create mode 100644 tests/check-qom-proplist.c
> 
> diff --git a/include/qom/object.h b/include/qom/object.h
> index d2d7748..15ac314 100644
> --- a/include/qom/object.h
> +++ b/include/qom/object.h
> @@ -607,6 +607,73 @@ 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.
> + * @parent: the parent object
> + * @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.

Grammar. ("will"?)

> + * 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 when registering the object as a
> + * child of @parent in the objects hierarchy.

s/objects hierarchy/composition tree/

> + *
> + * The variadic parameters are a list of pairs of (propname, propvalue)
> + * strings. The propname of NULL indicates the end of the property

%NULL

> + * 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,
> + *                          container_get(object_get_root(), "/objects")

If this is used in multiple places, please introduce a helper like I did
for /machine. The reason being avoiding hardcoded string paths.

> + *                          "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));
> + *   }

Please see in the top of the file for examples how to enclose sample code.

> + *
> + * The returned object will have one stable reference maintained
> + * for as long as it is present in the object hierarchy.
> + *
> + * Returns: The newly allocated, instantiated & initialized object.
> + */
> +Object *object_new_propv(const char *typename,
> +                         Object *parent,
> +                         const char *id,
> +                         Error **errp,
> +                         ...)
> +    __attribute__((sentinel));

First time I see this in QEMU - is it safe to use unconditionally?
(clang, older gcc, etc.)

> +
> +/**
> + * object_new_proplist:
> + * @typename:  The name of the type of the object to instantiate.
> + * @parent: the parent object
> + * @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.

Needs to be object_new_propv() for referencing.

> + */
> +Object *object_new_proplist(const char *typename,
> +                            Object *parent,
> +                            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..2115542 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,71 @@ Object *object_new(const char *typename)
>      return object_new_with_type(ti);
>  }
>  
> +Object *object_new_propv(const char *typename,
> +                         Object *parent,
> +                         const char *id,
> +                         Error **errp,
> +                         ...)
> +{
> +    va_list vargs;
> +    Object *obj;
> +
> +    va_start(vargs, errp);
> +    obj = object_new_proplist(typename, parent, id, errp, vargs);
> +    va_end(vargs);
> +
> +    return obj;
> +}
> +
> +Object *object_new_proplist(const char *typename,
> +                            Object *parent,
> +                            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))) {

This check seems too late. If the type is abstract, object_new() will
have aborted.

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

This pattern is wrong. You need a local Error *err = NULL;, otherwise
you may be deferencing NULL.

> +            goto error;
> +        }
> +        propname = va_arg(vargs, char *);
> +    }
> +
> +    object_property_add_child(parent, id, obj, errp);
> +    if (*errp) {
> +        goto error;
> +    }
> +
> +    if (object_dynamic_cast(obj, TYPE_USER_CREATABLE)) {
> +        user_creatable_complete(obj, errp);
> +        if (*errp) {
> +            object_unparent(obj);
> +            goto error;
> +        }
> +    }
> +
> +    object_unref(OBJECT(obj));
> +    return obj;
> +
> + error:

Intentionally indented?

> +    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..9f16cdb
> --- /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 <address@hidden>
> + */
> +
> +#include <glib.h>
> +
> +#include "qom/object.h"
> +#include "qemu/module.h"
> +
> +
> +#define TYPE_DUMMY "qemu:dummy"

Is colon considered valid in a type name?

> +
> +typedef struct DummyObject DummyObject;
> +typedef struct DummyObjectClass DummyObjectClass;
> +
> +#define DUMMY_OBJECT(obj)                               \
> +    OBJECT_CHECK(DummyObject, (obj), TYPE_DUMMY)
> +
> +struct DummyObject {
> +    Object parent;

parent_obj for consistency please.

> +
> +    bool bv;
> +    char *sv;
> +};
> +
> +struct DummyObjectClass {
> +    ObjectClass parent;

parent_class for consistency. If you copied these, please indicate from
where so that we can fix that.

> +};
> +
> +
> +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;
> +    Object *parent = container_get(object_get_root(),
> +                                   "/objects");
> +    DummyObject *dobj = DUMMY_OBJECT(
> +        object_new_propv(TYPE_DUMMY,
> +                         parent,
> +                         "dummy0",
> +                         &err,
> +                         "bv", "yes",
> +                         "sv", "Hiss hiss hiss",
> +                         NULL));
> +
> +    g_assert(dobj != NULL);

I believe DUMMY_OBJECT() would assert in that case already. There is
object_dynamic_cast() in case that is undesired.

> +    g_assert(err == NULL);
> +    g_assert(g_str_equal(dobj->sv, "Hiss hiss hiss"));

Isn't there a GTester string comparison function for this that outputs
both strings in case of a mismatch?

> +    g_assert(dobj->bv == true);
> +
> +    g_assert(object_resolve_path_component(parent, "dummy0")
> +             == OBJECT(dobj));
> +
> +    object_unparent(OBJECT(dobj));
> +}
> +
> +
> +static Object *new_helper(Error **errp,
> +                          Object *parent,
> +                          ...)
> +{
> +    va_list vargs;
> +    Object *obj;
> +
> +    va_start(vargs, parent);
> +    obj = object_new_proplist(TYPE_DUMMY,
> +                              parent,
> +                              "dummy0",
> +                              errp,
> +                              vargs);
> +    va_end(vargs);
> +    return obj;
> +}
> +
> +static void test_dummy_createlist(void)
> +{
> +    Error *err = NULL;
> +    Object *parent = container_get(object_get_root(),
> +                                   "/objects");
> +    DummyObject *dobj = DUMMY_OBJECT(
> +        new_helper(&err,
> +                   parent,
> +                   "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(parent, "dummy0")
> +             == OBJECT(dobj));
> +
> +    object_unparent(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();
> +}

Regards,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Jennifer Guild, Dilip Upmanyu,
Graham Norton; HRB 21284 (AG Nürnberg)