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

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

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 <berrange@redhat.com>
> ---
>  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 <berrange@redhat.com>
> + */
> +
> +#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

On 08/05/2015 19:10, Andreas Färber wrote:
>>    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().

Right, this was my main request on review and I had fixed up the commit
message in the pull request.

I'm certainly okay with a separate object_set_props function (better:
object_parse_props) and object_parse_propv for the va_list case.

Paolo

>>                           "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       |  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 <berrange@redhat.com>
>> + */
>> +
>> +#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
>

Am 08.05.2015 um 19:18 schrieb Paolo Bonzini:
> On 08/05/2015 19:10, Andreas Färber wrote:
>>>    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().
> 
> Right, this was my main request on review

Hm, didn't make it to the list then...? Only the one reply to 0/7.

Andreas

> and I had fixed up the commit
> message in the pull request.
> 
> I'm certainly okay with a separate object_set_props function (better:
> object_parse_props) and object_parse_propv for the va_list case.
> 
> Paolo

On 05/08/2015 11:10 AM, Andreas Färber wrote:
> 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.
>>

>> +
>> +    object_unref(OBJECT(obj));
>> +    return obj;
>> +
>> + error:
> 
> Intentionally indented?

Yes. Emacs c-mode defaults to indenting like this on purpose, in order
to leave column 1 reserved for the start of a function.  Besides, things
like 'diff -p' search for content in column 1, and if top-level labels
are not indented to column 2, then they get interpreted as function
names, making the diff a bit less useful.

Libvirt has gone one step further and enforces this indentation style
during its 'make syntax-check'; I'm sure if we wanted to do likewise in
qemu, we could patch scripts/checkpatch.pl to enforce a particular
style.  But right now, I'm personally okay with not worrying about it.

On Fri, May 08, 2015 at 07:10:49PM +0200, Andreas Färber wrote:
> 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()?

Sure, with_props() looks fine.

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

Ok, I didn't see that qemu had a convention on that, so will change
to match.

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

I don't really think that forcing the callers to call new + set_props
separately is really makng it more reliable - in fact the contrary - it
means that the callers have more complex boilerplate code which they all
have to tediously duplicate in exactly the same way. With the single
object_new_with_props call, you know that if it returns NULL then it
failed and you have no cleanup that you need todo which is about as
reliable as it gets.

That said, I can see the value in having a standalone object_set_props()
method as a general feature. So I will add that, and simply make the
object_new_with_props method call object_new + object_set_props + 

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

I use it in the VNC server when I convert it to use generic TLS encryption
code over to use the QCryptoTLSCreds object - it reduced a 100+ line
method into just two calls to object_new_propv. See vnc_display_create_creds()
in this RFC patch:

  https://lists.gnu.org/archive/html/qemu-devel/2015-04/msg02062.html


Then, I've got a bunch of unit tests related to that series which are
using it, again to reduce the amount of code it takes to create and
set props on this TLS creds object.

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

Yep, that's a docs mistake.

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

Sure, will do that.

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

Ok.

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

I'm actually not sure - will look into this.

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

Ok

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

Ah, I didn't realize that it did that.

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

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


> > +#define TYPE_DUMMY "qemu:dummy"
> 
> Is colon considered valid in a type name?

Nothing complained, but I can trivially remove the colon.

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

This is just the naming convention used by GObject and I hadn't noticed
that QEMU had its own convention. I'll change these as you suggest.


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

Ah, good point.

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

Possibly, I'll look into that.


Regards,
Daniel