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

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