Re: [Qemu-devel] [PATCH v3 4/7] qom: add object_new_propv / object_new_proplist constructors
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 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
Re: [Qemu-devel] [PATCH v3 4/7] qom: add object_new_propv / object_new_proplist constructors
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 -- SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Felix Imendörffer, Jane Smithard, Dilip Upmanyu, Graham Norton; HRB 21284 (AG Nürnberg)
Re: [Qemu-devel] [PATCH v3 4/7] qom: add object_new_propv / object_new_proplist constructors
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
Re: [Qemu-devel] [PATCH v3 4/7] qom: add object_new_propv / object_new_proplist constructors
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 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,
+
Re: [Qemu-devel] [PATCH v3 4/7] qom: add object_new_propv / object_new_proplist constructors
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. -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature
[Qemu-devel] [PATCH 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 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.
+ * 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.
+ *
+ * The variadic parameters are a list of pairs of (propname, propvalue)
+ * strings. The propname of NULL indicates the end of the property
+ * list. If the object implements the user creatable interface, the
+ * object will be marked complete once all the properties have been
+ * processed.
+ *
+ * Error *err = NULL;
+ * Object *obj;
+ *
+ * obj = object_new_propv(TYPE_MEMORY_BACKEND_FILE,
+ * container_get(object_get_root(), /objects)
+ * hostmem0,
+ * err,
+ * share, yes,
+ * mem-path, /dev/shm/somefile,
+ * prealloc, yes,
+ * size, 1048576,
+ * NULL);
+ *
+ * if (!obj) {
+ * g_printerr(Cannot create memory backend: %s\n,
+ *error_get_pretty(err));
+ * }
+ *
+ * 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));
+
+/**
+ * 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.
+ */
+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,
