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