* [Qemu-devel] [PATCH v4 0/8] qom: misc fixes & enhancements to support TLS work
@ 2015-05-13 16:14 Daniel P. Berrange
2015-05-13 16:14 ` [Qemu-devel] [PATCH v4 1/8] backends: fix typename of 'policy' enum property in hostmem obj Daniel P. Berrange
` (7 more replies)
0 siblings, 8 replies; 23+ messages in thread
From: Daniel P. Berrange @ 2015-05-13 16:14 UTC (permalink / raw)
To: qemu-devel; +Cc: Paolo Bonzini, Andreas Färber
This series contains the 7^H8 generic QOM API fixes and enhancements
that I previously posted as part of the large series refactoring
and extending the TLS support in QEMU:
https://lists.gnu.org/archive/html/qemu-devel/2015-04/msg02038.html
I'm sending it separately, since the patches are reasonably well
self-contained and thus hopefully suitable for quicker review and
merge.
The changes in this posting are in response to Andreas' feedback
on v3.
Changed in v4:
- Introduce object_get_objects_root() to replace duplicated
container_get(object_get_root(), "/objects") calls
- Add object_set_prop[sv]()and call them to populate properties
from object_new_with_prop[sv]() constructor
- Use <programlisting> in inline docs example
- Fix naming for '...' vs 'va_list' arg method variants to
match QEMU convention (va_list one has the 'v' suffix)
- Conditionally use __attribute__((sentinel)) only on new
enough GCC (>= 4.0)
- Use a local Error * object where Error **errp could be NULL
- Change class def to use parent_obj and parent_class for
first struct member
- Use g_assert_cmpstr() in test case assertions on strings
- Other misc typos in docs, commit messages and code.
Changed in v3:
- Fix test suite additions for change in object_new_propv API
Changed in v2:
- Pass "Object * parent" instead of "char *path" paremeter
- Rely on stable reference from parent to keep new object alive
- Use object_unparent() where appropriate
Daniel P. Berrange (8):
backends: fix typename of 'policy' enum property in hostmem obj
doc: document user creatable object types in help text
vl: create (most) objects before creating chardev backends
qom: add helper method for getting user objects root
qom: add object_new_with_props / object_new_withpropv constructors
qom: make enum string tables const-correct
qom: add a object_property_add_enum helper method
qom: don't pass string table to object_get_enum method
backends/hostmem.c | 22 ++--
include/hw/qdev-core.h | 2 +-
include/qapi/util.h | 2 +-
include/qapi/visitor-impl.h | 6 +-
include/qapi/visitor.h | 2 +-
include/qemu/compiler.h | 6 +
include/qom/object.h | 163 +++++++++++++++++++++++-
iothread.c | 4 +-
numa.c | 4 +-
qapi/qapi-dealloc-visitor.c | 3 +-
qapi/qapi-util.c | 2 +-
qapi/qapi-visit-core.c | 6 +-
qemu-options.hx | 70 ++++++++---
qmp.c | 6 +-
qom/object.c | 191 +++++++++++++++++++++++++++-
scripts/qapi-types.py | 4 +-
tests/.gitignore | 1 +
tests/Makefile | 5 +-
tests/check-qom-proplist.c | 301 ++++++++++++++++++++++++++++++++++++++++++++
vl.c | 40 +++++-
20 files changed, 783 insertions(+), 57 deletions(-)
create mode 100644 tests/check-qom-proplist.c
--
2.1.0
^ permalink raw reply [flat|nested] 23+ messages in thread
* [Qemu-devel] [PATCH v4 1/8] backends: fix typename of 'policy' enum property in hostmem obj
2015-05-13 16:14 [Qemu-devel] [PATCH v4 0/8] qom: misc fixes & enhancements to support TLS work Daniel P. Berrange
@ 2015-05-13 16:14 ` Daniel P. Berrange
2015-05-13 16:14 ` [Qemu-devel] [PATCH v4 2/8] doc: document user creatable object types in help text Daniel P. Berrange
` (6 subsequent siblings)
7 siblings, 0 replies; 23+ messages in thread
From: Daniel P. Berrange @ 2015-05-13 16:14 UTC (permalink / raw)
To: qemu-devel; +Cc: Paolo Bonzini, Andreas Färber
The 'policy' property was being registered with a typename of
'str', but it is in fact an enum of the 'HostMemPolicy' type.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
backends/hostmem.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/backends/hostmem.c b/backends/hostmem.c
index b7b6cf8..f6db33c 100644
--- a/backends/hostmem.c
+++ b/backends/hostmem.c
@@ -252,7 +252,7 @@ static void host_memory_backend_init(Object *obj)
object_property_add(obj, "host-nodes", "int",
host_memory_backend_get_host_nodes,
host_memory_backend_set_host_nodes, NULL, NULL, NULL);
- object_property_add(obj, "policy", "str",
+ object_property_add(obj, "policy", "HostMemPolicy",
host_memory_backend_get_policy,
host_memory_backend_set_policy, NULL, NULL, NULL);
}
--
2.1.0
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [Qemu-devel] [PATCH v4 2/8] doc: document user creatable object types in help text
2015-05-13 16:14 [Qemu-devel] [PATCH v4 0/8] qom: misc fixes & enhancements to support TLS work Daniel P. Berrange
2015-05-13 16:14 ` [Qemu-devel] [PATCH v4 1/8] backends: fix typename of 'policy' enum property in hostmem obj Daniel P. Berrange
@ 2015-05-13 16:14 ` Daniel P. Berrange
2015-05-13 16:14 ` [Qemu-devel] [PATCH v4 3/8] vl: create (most) objects before creating chardev backends Daniel P. Berrange
` (5 subsequent siblings)
7 siblings, 0 replies; 23+ messages in thread
From: Daniel P. Berrange @ 2015-05-13 16:14 UTC (permalink / raw)
To: qemu-devel; +Cc: Paolo Bonzini, Andreas Färber
The QEMU help for -object is essentially useless, just giving users
the generic syntax. Move it down into its own section and introduce
a nested table where each user creatable object can be documented.
The existing memory-backend-file, rng-random and rng-egd object
types are documented.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
qemu-options.hx | 70 ++++++++++++++++++++++++++++++++++++++++++++-------------
1 file changed, 54 insertions(+), 16 deletions(-)
diff --git a/qemu-options.hx b/qemu-options.hx
index ec356f6..00ae287 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -3421,22 +3421,6 @@ DEF("no-kvm-irqchip", 0, QEMU_OPTION_no_kvm_irqchip, "", QEMU_ARCH_I386)
HXCOMM Deprecated (ignored)
DEF("tdf", 0, QEMU_OPTION_tdf,"", QEMU_ARCH_ALL)
-DEF("object", HAS_ARG, QEMU_OPTION_object,
- "-object TYPENAME[,PROP1=VALUE1,...]\n"
- " create an new object of type TYPENAME setting properties\n"
- " in the order they are specified. Note that the 'id'\n"
- " property must be set. These objects are placed in the\n"
- " '/objects' path.\n",
- QEMU_ARCH_ALL)
-STEXI
-@item -object @var{typename}[,@var{prop1}=@var{value1},...]
-@findex -object
-Create an new object of type @var{typename} setting properties
-in the order they are specified. Note that the 'id'
-property must be set. These objects are placed in the
-'/objects' path.
-ETEXI
-
DEF("msg", HAS_ARG, QEMU_OPTION_msg,
"-msg timestamp[=on|off]\n"
" change the format of messages\n"
@@ -3462,6 +3446,60 @@ Dump json-encoded vmstate information for current machine type to file
in @var{file}
ETEXI
+DEFHEADING(Generic object creation)
+
+DEF("object", HAS_ARG, QEMU_OPTION_object,
+ "-object TYPENAME[,PROP1=VALUE1,...]\n"
+ " create a new object of type TYPENAME setting properties\n"
+ " in the order they are specified. Note that the 'id'\n"
+ " property must be set. These objects are placed in the\n"
+ " '/objects' path.\n",
+ QEMU_ARCH_ALL)
+STEXI
+@item -object @var{typename}[,@var{prop1}=@var{value1},...]
+@findex -object
+Create a new object of type @var{typename} setting properties
+in the order they are specified. Note that the 'id'
+property must be set. These objects are placed in the
+'/objects' path.
+
+@table @option
+
+@item -object memory-backend-file,id=@var{id},size=@var{size},mem-path=@var{dir},share=@var{on|off}
+
+Creates a memory file backend object, which can be used to back
+the guest RAM with huge pages. The @option{id} parameter is a
+unique ID that will be used to reference this memory region
+when configuring the @option{-numa} argument. The @option{size}
+option provides the size of the memory region, and accepts
+common suffixes, eg @option{500M}. The @option{mem-path} provides
+the path to either a shared memory or huge page filesystem mount.
+The @option{share} boolean option determines whether the memory
+region is marked as private to QEMU, or shared. The latter allows
+a co-operating external process to access the QEMU memory region.
+
+@item -object rng-random,id=@var{id},filename=@var{/dev/random}
+
+Creates a random number generator backend which obtains entropy from
+a device on the host. The @option{id} parameter is a unique ID that
+will be used to reference this entropy backend from the @option{virtio-rng}
+device. The @option{filename} parameter specifies which file to obtain
+entropy from and if omitted defaults to @option{/dev/random}.
+
+@item -object rng-egd,id=@var{id},chardev=@var{chardevid}
+
+Creates a random number generator backend which obtains entropy from
+an external daemon running on the host. The @option{id} parameter is
+a unique ID that will be used to reference this entropy backend from
+the @option{virtio-rng} device. The @option{chardev} parameter is
+the unique ID of a character device backend that provides the connection
+to the RNG daemon.
+
+@end table
+
+ETEXI
+
+
HXCOMM This is the last statement. Insert new options before this line!
STEXI
@end table
--
2.1.0
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [Qemu-devel] [PATCH v4 3/8] vl: create (most) objects before creating chardev backends
2015-05-13 16:14 [Qemu-devel] [PATCH v4 0/8] qom: misc fixes & enhancements to support TLS work Daniel P. Berrange
2015-05-13 16:14 ` [Qemu-devel] [PATCH v4 1/8] backends: fix typename of 'policy' enum property in hostmem obj Daniel P. Berrange
2015-05-13 16:14 ` [Qemu-devel] [PATCH v4 2/8] doc: document user creatable object types in help text Daniel P. Berrange
@ 2015-05-13 16:14 ` Daniel P. Berrange
2015-05-13 16:14 ` [Qemu-devel] [PATCH v4 4/8] qom: add helper method for getting user objects root Daniel P. Berrange
` (4 subsequent siblings)
7 siblings, 0 replies; 23+ messages in thread
From: Daniel P. Berrange @ 2015-05-13 16:14 UTC (permalink / raw)
To: qemu-devel; +Cc: Paolo Bonzini, Andreas Färber
Some types of object must be created before chardevs, other types of
object must be created after chardevs. As such there is no option but
to create objects in two phases.
This takes the decision to create as many object types as possible
right away before anyother backends are created, and only delay
creation of those few which have an explicit dependency on the
chardevs. Hopefully the set which need delaying will remain small
over time.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
vl.c | 40 +++++++++++++++++++++++++++++++++++++++-
1 file changed, 39 insertions(+), 1 deletion(-)
diff --git a/vl.c b/vl.c
index 15bccc4..b7c7511 100644
--- a/vl.c
+++ b/vl.c
@@ -2601,6 +2601,33 @@ static int machine_set_property(const char *name, const char *value,
return 0;
}
+
+/*
+ * Initial object creation happens before all other
+ * QEMU data types are created. The majority of objects
+ * can be created at this point. The rng-egd object
+ * cannot be created here, as it depends on the chardev
+ * already existing.
+ */
+static bool object_create_initial(const char *type)
+{
+ if (g_str_equal(type, "rng-egd")) {
+ return false;
+ }
+ return true;
+}
+
+
+/*
+ * The remainder of object creation happens after the
+ * creation of chardev, fsdev and device data types.
+ */
+static bool object_create_delayed(const char *type)
+{
+ return !object_create_initial(type);
+}
+
+
static int object_create(QemuOpts *opts, void *opaque)
{
Error *err = NULL;
@@ -2609,6 +2636,7 @@ static int object_create(QemuOpts *opts, void *opaque)
void *dummy = NULL;
OptsVisitor *ov;
QDict *pdict;
+ bool (*type_predicate)(const char *) = opaque;
ov = opts_visitor_new(opts);
pdict = qemu_opts_to_qdict(opts, NULL);
@@ -2623,6 +2651,9 @@ static int object_create(QemuOpts *opts, void *opaque)
if (err) {
goto out;
}
+ if (!type_predicate(type)) {
+ goto out;
+ }
qdict_del(pdict, "id");
visit_type_str(opts_get_visitor(ov), &id, "id", &err);
@@ -4031,6 +4062,12 @@ int main(int argc, char **argv, char **envp)
socket_init();
+ if (qemu_opts_foreach(qemu_find_opts("object"),
+ object_create,
+ object_create_initial, 0) != 0) {
+ exit(1);
+ }
+
if (qemu_opts_foreach(qemu_find_opts("chardev"), chardev_init_func, NULL, 1) != 0)
exit(1);
#ifdef CONFIG_VIRTFS
@@ -4050,7 +4087,8 @@ int main(int argc, char **argv, char **envp)
}
if (qemu_opts_foreach(qemu_find_opts("object"),
- object_create, NULL, 0) != 0) {
+ object_create,
+ object_create_delayed, 0) != 0) {
exit(1);
}
--
2.1.0
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [Qemu-devel] [PATCH v4 4/8] qom: add helper method for getting user objects root
2015-05-13 16:14 [Qemu-devel] [PATCH v4 0/8] qom: misc fixes & enhancements to support TLS work Daniel P. Berrange
` (2 preceding siblings ...)
2015-05-13 16:14 ` [Qemu-devel] [PATCH v4 3/8] vl: create (most) objects before creating chardev backends Daniel P. Berrange
@ 2015-05-13 16:14 ` Daniel P. Berrange
2015-05-19 13:30 ` Andreas Färber
2015-05-13 16:14 ` [Qemu-devel] [PATCH v4 5/8] qom: add object_new_with_props / object_new_withpropv constructors Daniel P. Berrange
` (3 subsequent siblings)
7 siblings, 1 reply; 23+ messages in thread
From: Daniel P. Berrange @ 2015-05-13 16:14 UTC (permalink / raw)
To: qemu-devel; +Cc: Paolo Bonzini, Andreas Färber
Add object_get_objects_root() method which is a convience for
obtaining the Object * located at /objects in the object
composition tree. Convert existing code over to use the new
API where appropriate.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
include/qom/object.h | 12 ++++++++++++
iothread.c | 4 +---
numa.c | 2 +-
qmp.c | 6 +++---
qom/object.c | 5 +++++
5 files changed, 22 insertions(+), 7 deletions(-)
diff --git a/include/qom/object.h b/include/qom/object.h
index d2d7748..d30c68c 100644
--- a/include/qom/object.h
+++ b/include/qom/object.h
@@ -1026,6 +1026,18 @@ const char *object_property_get_type(Object *obj, const char *name,
*/
Object *object_get_root(void);
+
+/**
+ * object_get_objects_root:
+ *
+ * Get the container object that holds user created
+ * object instances. This is the object at path
+ * "/objects"
+ *
+ * Returns: the user object container
+ */
+Object *object_get_objects_root(void);
+
/**
* object_get_canonical_path_component:
*
diff --git a/iothread.c b/iothread.c
index 0416fc4..98dcdf9 100644
--- a/iothread.c
+++ b/iothread.c
@@ -19,8 +19,6 @@
#include "qmp-commands.h"
#include "qemu/error-report.h"
-#define IOTHREADS_PATH "/objects"
-
typedef ObjectClass IOThreadClass;
#define IOTHREAD_GET_CLASS(obj) \
@@ -153,7 +151,7 @@ IOThreadInfoList *qmp_query_iothreads(Error **errp)
{
IOThreadInfoList *head = NULL;
IOThreadInfoList **prev = &head;
- Object *container = container_get(object_get_root(), IOTHREADS_PATH);
+ Object *container = object_get_objects_root();
object_child_foreach(container, query_one_iothread, &prev);
return head;
diff --git a/numa.c b/numa.c
index c975fb2..b14a5c1 100644
--- a/numa.c
+++ b/numa.c
@@ -486,7 +486,7 @@ MemdevList *qmp_query_memdev(Error **errp)
Object *obj;
MemdevList *list = NULL;
- obj = object_resolve_path("/objects", NULL);
+ obj = object_get_objects_root();
if (obj == NULL) {
return NULL;
}
diff --git a/qmp.c b/qmp.c
index 3f5dfe3..fa013e3 100644
--- a/qmp.c
+++ b/qmp.c
@@ -651,7 +651,7 @@ void object_add(const char *type, const char *id, const QDict *qdict,
}
}
- object_property_add_child(container_get(object_get_root(), "/objects"),
+ object_property_add_child(object_get_objects_root(),
id, obj, &local_err);
if (local_err) {
goto out;
@@ -659,7 +659,7 @@ void object_add(const char *type, const char *id, const QDict *qdict,
user_creatable_complete(obj, &local_err);
if (local_err) {
- object_property_del(container_get(object_get_root(), "/objects"),
+ object_property_del(object_get_objects_root(),
id, &error_abort);
goto out;
}
@@ -706,7 +706,7 @@ void qmp_object_del(const char *id, Error **errp)
Object *container;
Object *obj;
- container = container_get(object_get_root(), "/objects");
+ container = object_get_objects_root();
obj = object_resolve_path_component(container, id);
if (!obj) {
error_setg(errp, "object id not found");
diff --git a/qom/object.c b/qom/object.c
index b8dff43..baeece2 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -1054,6 +1054,11 @@ Object *object_get_root(void)
return root;
}
+Object *object_get_objects_root(void)
+{
+ return container_get(object_get_root(), "/objects");
+}
+
static void object_get_child_property(Object *obj, Visitor *v, void *opaque,
const char *name, Error **errp)
{
--
2.1.0
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [Qemu-devel] [PATCH v4 5/8] qom: add object_new_with_props / object_new_withpropv constructors
2015-05-13 16:14 [Qemu-devel] [PATCH v4 0/8] qom: misc fixes & enhancements to support TLS work Daniel P. Berrange
` (3 preceding siblings ...)
2015-05-13 16:14 ` [Qemu-devel] [PATCH v4 4/8] qom: add helper method for getting user objects root Daniel P. Berrange
@ 2015-05-13 16:14 ` Daniel P. Berrange
2015-05-19 15:52 ` Andreas Färber
2015-05-13 16:14 ` [Qemu-devel] [PATCH v4 6/8] qom: make enum string tables const-correct Daniel P. Berrange
` (2 subsequent siblings)
7 siblings, 1 reply; 23+ messages in thread
From: Daniel P. Berrange @ 2015-05-13 16:14 UTC (permalink / raw)
To: qemu-devel; +Cc: Paolo Bonzini, Andreas Färber
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.
First a pair of methods object_set_props / object_set_propv
are added which allow for a list of objects to be set in
one single API call.
Then object_new_with_props / object_new_with_propv constructors
are added which simplify the sequence of calls to create an
object, populate properties, register in the object composition
tree and mark the object complete, into a single method call.
Usage would be:
Error *err = NULL;
Object *obj;
obj = object_new_with_propv(TYPE_MEMORY_BACKEND_FILE,
object_get_objects_root(),
"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, using normal QOM
semantics for parsing from string format.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
include/qemu/compiler.h | 6 ++
include/qom/object.h | 128 +++++++++++++++++++++++++++++++
qom/object.c | 109 ++++++++++++++++++++++++++
tests/.gitignore | 1 +
tests/Makefile | 5 +-
tests/check-qom-proplist.c | 186 +++++++++++++++++++++++++++++++++++++++++++++
6 files changed, 434 insertions(+), 1 deletion(-)
create mode 100644 tests/check-qom-proplist.c
diff --git a/include/qemu/compiler.h b/include/qemu/compiler.h
index ac7c4c4..df9dd51 100644
--- a/include/qemu/compiler.h
+++ b/include/qemu/compiler.h
@@ -24,6 +24,12 @@
#define QEMU_WARN_UNUSED_RESULT
#endif
+#if QEMU_GNUC_PREREQ(4, 0)
+#define QEMU_SENTINEL __attribute__((sentinel))
+#else
+#define QEMU_SENTINEL
+#endif
+
#if QEMU_GNUC_PREREQ(4, 3)
#define QEMU_ARTIFICIAL __attribute__((always_inline, artificial))
#else
diff --git a/include/qom/object.h b/include/qom/object.h
index d30c68c..fcacdb0 100644
--- a/include/qom/object.h
+++ b/include/qom/object.h
@@ -607,6 +607,134 @@ Object *object_new(const char *typename);
Object *object_new_with_type(Type type);
/**
+ * object_new_with_props:
+ * @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 will 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 composition tree.
+ *
+ * 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.
+ *
+ * <example>
+ * <title>Creating an object with properties</title>
+ * <programlisting>
+ * Error *err = NULL;
+ * Object *obj;
+ *
+ * obj = object_new_with_props(TYPE_MEMORY_BACKEND_FILE,
+ * object_get_objects_root(),
+ * "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));
+ * }
+ * </programlisting>
+ * </example>
+ *
+ * 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_with_props(const char *typename,
+ Object *parent,
+ const char *id,
+ Error **errp,
+ ...) QEMU_SENTINEL;
+
+/**
+ * object_new_with_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
+ * @vargs: list of property names and values
+ *
+ * See object_new_with_props() for documentation.
+ */
+Object *object_new_with_propv(const char *typename,
+ Object *parent,
+ const char *id,
+ Error **errp,
+ va_list vargs);
+
+/**
+ * object_set_props:
+ * @obj: the object instance to set properties on
+ * @errp: pointer to error object
+ * @...: list of property names and values
+ *
+ * This function will set a list of properties on an existing object
+ * instance.
+ *
+ * The variadic parameters are a list of pairs of (propname, propvalue)
+ * strings. The propname of %NULL indicates the end of the property
+ * list.
+ *
+ * <example>
+ * <title>Update an object's properties</title>
+ * <programlisting>
+ * Error *err = NULL;
+ * Object *obj = ...get / create object...;
+ *
+ * obj = object_set_props(obj,
+ * &err,
+ * "share", "yes",
+ * "mem-path", "/dev/shm/somefile",
+ * "prealloc", "yes",
+ * "size", "1048576",
+ * NULL);
+ *
+ * if (!obj) {
+ * g_printerr("Cannot set properties: %s\n",
+ * error_get_pretty(err));
+ * }
+ * </programlisting>
+ * </example>
+ *
+ * The returned object will have one stable reference maintained
+ * for as long as it is present in the object hierarchy.
+ *
+ * Returns: -1 on error, 0 on success
+ */
+int object_set_props(Object *obj,
+ Error **errp,
+ ...) QEMU_SENTINEL;
+
+/**
+ * object_set_propv:
+ * @obj: the object instance to set properties on
+ * @errp: pointer to error object
+ * @vargs: list of property names and values
+ *
+ * See object_set_props() for documentation.
+ *
+ * Returns: -1 on error, 0 on success
+ */
+int object_set_propv(Object *obj,
+ 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 baeece2..55ab081 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,114 @@ Object *object_new(const char *typename)
return object_new_with_type(ti);
}
+
+Object *object_new_with_props(const char *typename,
+ Object *parent,
+ const char *id,
+ Error **errp,
+ ...)
+{
+ va_list vargs;
+ Object *obj;
+
+ va_start(vargs, errp);
+ obj = object_new_with_propv(typename, parent, id, errp, vargs);
+ va_end(vargs);
+
+ return obj;
+}
+
+
+Object *object_new_with_propv(const char *typename,
+ Object *parent,
+ const char *id,
+ Error **errp,
+ va_list vargs)
+{
+ Object *obj;
+ ObjectClass *klass;
+ Error *local_err = NULL;
+
+ klass = object_class_by_name(typename);
+ if (!klass) {
+ error_setg(errp, "invalid object type: %s", typename);
+ return NULL;
+ }
+
+ if (object_class_is_abstract(klass)) {
+ error_setg(errp, "object type '%s' is abstract", typename);
+ return NULL;
+ }
+ obj = object_new(typename);
+
+ if (object_set_propv(obj, &local_err, vargs) < 0) {
+ goto error;
+ }
+
+ object_property_add_child(parent, id, obj, &local_err);
+ if (local_err) {
+ goto error;
+ }
+
+ if (object_dynamic_cast(obj, TYPE_USER_CREATABLE)) {
+ user_creatable_complete(obj, &local_err);
+ if (local_err) {
+ object_unparent(obj);
+ goto error;
+ }
+ }
+
+ object_unref(OBJECT(obj));
+ return obj;
+
+ error:
+ if (local_err) {
+ error_propagate(errp, local_err);
+ }
+ object_unref(obj);
+ return NULL;
+}
+
+
+int object_set_props(Object *obj,
+ Error **errp,
+ ...)
+{
+ va_list vargs;
+ int ret;
+
+ va_start(vargs, errp);
+ ret = object_set_propv(obj, errp, vargs);
+ va_end(vargs);
+
+ return ret;
+}
+
+
+int object_set_propv(Object *obj,
+ Error **errp,
+ va_list vargs)
+{
+ const char *propname;
+ Error *local_err = NULL;
+
+ 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, &local_err);
+ if (local_err) {
+ error_propagate(errp, local_err);
+ return -1;
+ }
+ propname = va_arg(vargs, char *);
+ }
+
+ return 0;
+}
+
+
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 666aee2..1973d35 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)
@@ -264,7 +266,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
@@ -273,6 +275,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..e82532e
--- /dev/null
+++ b/tests/check-qom-proplist.c
@@ -0,0 +1,186 @@
+/*
+ * 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"
+
+typedef struct DummyObject DummyObject;
+typedef struct DummyObjectClass DummyObjectClass;
+
+#define DUMMY_OBJECT(obj) \
+ OBJECT_CHECK(DummyObject, (obj), TYPE_DUMMY)
+
+struct DummyObject {
+ Object parent_obj;
+
+ bool bv;
+ char *sv;
+};
+
+struct DummyObjectClass {
+ ObjectClass parent_class;
+};
+
+
+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 = object_get_objects_root();
+ DummyObject *dobj = DUMMY_OBJECT(
+ object_new_with_props(TYPE_DUMMY,
+ parent,
+ "dummy0",
+ &err,
+ "bv", "yes",
+ "sv", "Hiss hiss hiss",
+ NULL));
+
+ g_assert(err == NULL);
+ g_assert_cmpstr(dobj->sv, ==, "Hiss hiss hiss");
+ 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_with_propv(TYPE_DUMMY,
+ parent,
+ "dummy0",
+ errp,
+ vargs);
+ va_end(vargs);
+ return obj;
+}
+
+static void test_dummy_createlist(void)
+{
+ Error *err = NULL;
+ Object *parent = object_get_objects_root();
+ DummyObject *dobj = DUMMY_OBJECT(
+ new_helper(&err,
+ parent,
+ "bv", "yes",
+ "sv", "Hiss hiss hiss",
+ NULL));
+
+ g_assert(err == NULL);
+ g_assert_cmpstr(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();
+}
--
2.1.0
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [Qemu-devel] [PATCH v4 6/8] qom: make enum string tables const-correct
2015-05-13 16:14 [Qemu-devel] [PATCH v4 0/8] qom: misc fixes & enhancements to support TLS work Daniel P. Berrange
` (4 preceding siblings ...)
2015-05-13 16:14 ` [Qemu-devel] [PATCH v4 5/8] qom: add object_new_with_props / object_new_withpropv constructors Daniel P. Berrange
@ 2015-05-13 16:14 ` Daniel P. Berrange
2015-05-13 16:14 ` [Qemu-devel] [PATCH v4 7/8] qom: add a object_property_add_enum helper method Daniel P. Berrange
2015-05-13 16:14 ` [Qemu-devel] [PATCH v4 8/8] qom: don't pass string table to object_get_enum method Daniel P. Berrange
7 siblings, 0 replies; 23+ messages in thread
From: Daniel P. Berrange @ 2015-05-13 16:14 UTC (permalink / raw)
To: qemu-devel; +Cc: Paolo Bonzini, Andreas Färber
The enum string table parameters in various QOM/QAPI methods
are declared 'const char *strings[]'. This results in const
warnings if passed a variable that was declared as
static const char * const strings[] = { .... };
Add the extra const annotation to the parameters, since
neither the string elements, nor the array itself should
ever be modified.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
include/hw/qdev-core.h | 2 +-
include/qapi/util.h | 2 +-
include/qapi/visitor-impl.h | 6 +++---
include/qapi/visitor.h | 2 +-
include/qom/object.h | 2 +-
qapi/qapi-dealloc-visitor.c | 3 ++-
qapi/qapi-util.c | 2 +-
qapi/qapi-visit-core.c | 6 +++---
qom/object.c | 2 +-
scripts/qapi-types.py | 4 ++--
10 files changed, 16 insertions(+), 15 deletions(-)
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index d4be92f..ad9eb89 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -236,7 +236,7 @@ struct Property {
struct PropertyInfo {
const char *name;
const char *description;
- const char **enum_table;
+ const char * const *enum_table;
int (*print)(DeviceState *dev, Property *prop, char *dest, size_t len);
ObjectPropertyAccessor *get;
ObjectPropertyAccessor *set;
diff --git a/include/qapi/util.h b/include/qapi/util.h
index de9238b..7ad26c0 100644
--- a/include/qapi/util.h
+++ b/include/qapi/util.h
@@ -11,7 +11,7 @@
#ifndef QAPI_UTIL_H
#define QAPI_UTIL_H
-int qapi_enum_parse(const char *lookup[], const char *buf,
+int qapi_enum_parse(const char * const lookup[], const char *buf,
int max, int def, Error **errp);
#endif
diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h
index 09bb0fd..f4a2f74 100644
--- a/include/qapi/visitor-impl.h
+++ b/include/qapi/visitor-impl.h
@@ -30,7 +30,7 @@ struct Visitor
GenericList *(*next_list)(Visitor *v, GenericList **list, Error **errp);
void (*end_list)(Visitor *v, Error **errp);
- void (*type_enum)(Visitor *v, int *obj, const char *strings[],
+ void (*type_enum)(Visitor *v, int *obj, const char * const strings[],
const char *kind, const char *name, Error **errp);
void (*get_next_type)(Visitor *v, int *kind, const int *qobjects,
const char *name, Error **errp);
@@ -59,9 +59,9 @@ struct Visitor
void (*end_union)(Visitor *v, bool data_present, Error **errp);
};
-void input_type_enum(Visitor *v, int *obj, const char *strings[],
+void input_type_enum(Visitor *v, int *obj, const char * const strings[],
const char *kind, const char *name, Error **errp);
-void output_type_enum(Visitor *v, int *obj, const char *strings[],
+void output_type_enum(Visitor *v, int *obj, const char * const strings[],
const char *kind, const char *name, Error **errp);
#endif
diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
index 5934f59..00ba104 100644
--- a/include/qapi/visitor.h
+++ b/include/qapi/visitor.h
@@ -43,7 +43,7 @@ void visit_optional(Visitor *v, bool *present, const char *name,
Error **errp);
void visit_get_next_type(Visitor *v, int *obj, const int *qtypes,
const char *name, Error **errp);
-void visit_type_enum(Visitor *v, int *obj, const char *strings[],
+void visit_type_enum(Visitor *v, int *obj, const char * const strings[],
const char *kind, const char *name, Error **errp);
void visit_type_int(Visitor *v, int64_t *obj, const char *name, Error **errp);
void visit_type_uint8(Visitor *v, uint8_t *obj, const char *name, Error **errp);
diff --git a/include/qom/object.h b/include/qom/object.h
index fcacdb0..d07a506 100644
--- a/include/qom/object.h
+++ b/include/qom/object.h
@@ -1081,7 +1081,7 @@ int64_t object_property_get_int(Object *obj, const char *name,
* an enum).
*/
int object_property_get_enum(Object *obj, const char *name,
- const char *strings[], Error **errp);
+ const char * const strings[], Error **errp);
/**
* object_property_get_uint16List:
diff --git a/qapi/qapi-dealloc-visitor.c b/qapi/qapi-dealloc-visitor.c
index a14a1c7..d7f92c5 100644
--- a/qapi/qapi-dealloc-visitor.c
+++ b/qapi/qapi-dealloc-visitor.c
@@ -156,7 +156,8 @@ static void qapi_dealloc_type_size(Visitor *v, uint64_t *obj, const char *name,
{
}
-static void qapi_dealloc_type_enum(Visitor *v, int *obj, const char *strings[],
+static void qapi_dealloc_type_enum(Visitor *v, int *obj,
+ const char * const strings[],
const char *kind, const char *name,
Error **errp)
{
diff --git a/qapi/qapi-util.c b/qapi/qapi-util.c
index 1d8fb96..bcdc94d 100644
--- a/qapi/qapi-util.c
+++ b/qapi/qapi-util.c
@@ -14,7 +14,7 @@
#include "qapi/error.h"
#include "qapi/util.h"
-int qapi_enum_parse(const char *lookup[], const char *buf,
+int qapi_enum_parse(const char * const lookup[], const char *buf,
int max, int def, Error **errp)
{
int i;
diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c
index b66b93a..a18ba16 100644
--- a/qapi/qapi-visit-core.c
+++ b/qapi/qapi-visit-core.c
@@ -89,7 +89,7 @@ void visit_get_next_type(Visitor *v, int *obj, const int *qtypes,
}
}
-void visit_type_enum(Visitor *v, int *obj, const char *strings[],
+void visit_type_enum(Visitor *v, int *obj, const char * const strings[],
const char *kind, const char *name, Error **errp)
{
v->type_enum(v, obj, strings, kind, name, errp);
@@ -260,7 +260,7 @@ void visit_type_number(Visitor *v, double *obj, const char *name, Error **errp)
v->type_number(v, obj, name, errp);
}
-void output_type_enum(Visitor *v, int *obj, const char *strings[],
+void output_type_enum(Visitor *v, int *obj, const char * const strings[],
const char *kind, const char *name,
Error **errp)
{
@@ -279,7 +279,7 @@ void output_type_enum(Visitor *v, int *obj, const char *strings[],
visit_type_str(v, &enum_str, name, errp);
}
-void input_type_enum(Visitor *v, int *obj, const char *strings[],
+void input_type_enum(Visitor *v, int *obj, const char * const strings[],
const char *kind, const char *name,
Error **errp)
{
diff --git a/qom/object.c b/qom/object.c
index 55ab081..1421e5a 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -1070,7 +1070,7 @@ int64_t object_property_get_int(Object *obj, const char *name,
}
int object_property_get_enum(Object *obj, const char *name,
- const char *strings[], Error **errp)
+ const char * const strings[], Error **errp)
{
StringOutputVisitor *sov;
StringInputVisitor *siv;
diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index 2bf8145..55b2f6e 100644
--- a/scripts/qapi-types.py
+++ b/scripts/qapi-types.py
@@ -113,7 +113,7 @@ struct %(name)s
def generate_enum_lookup(name, values):
ret = mcgen('''
-const char *%(name)s_lookup[] = {
+const char * const %(name)s_lookup[] = {
''',
name=name)
i = 0
@@ -135,7 +135,7 @@ const char *%(name)s_lookup[] = {
def generate_enum(name, values):
lookup_decl = mcgen('''
-extern const char *%(name)s_lookup[];
+extern const char * const %(name)s_lookup[];
''',
name=name)
--
2.1.0
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [Qemu-devel] [PATCH v4 7/8] qom: add a object_property_add_enum helper method
2015-05-13 16:14 [Qemu-devel] [PATCH v4 0/8] qom: misc fixes & enhancements to support TLS work Daniel P. Berrange
` (5 preceding siblings ...)
2015-05-13 16:14 ` [Qemu-devel] [PATCH v4 6/8] qom: make enum string tables const-correct Daniel P. Berrange
@ 2015-05-13 16:14 ` Daniel P. Berrange
2015-05-13 16:14 ` [Qemu-devel] [PATCH v4 8/8] qom: don't pass string table to object_get_enum method Daniel P. Berrange
7 siblings, 0 replies; 23+ messages in thread
From: Daniel P. Berrange @ 2015-05-13 16:14 UTC (permalink / raw)
To: qemu-devel; +Cc: Paolo Bonzini, Andreas Färber
A QOM property can be parsed as enum using the visit_type_enum()
helper method, but this forces callers to use the more complex
generic object_property_add() method when registering it. It
also requires that users of that object have access to the
string map when they want to read the property value.
This patch introduces a specialized object_property_add_enum()
method which simplifies the use of enum properties, so the
setters/getters directly get passed the int value.
typedef enum {
MYDEV_TYPE_FROG,
MYDEV_TYPE_ALLIGATOR,
MYDEV_TYPE_PLATYPUS,
MYDEV_TYPE_LAST
} MyDevType;
Then provide a table of enum <-> string mappings
static const char *const mydevtypemap[MYDEV_TYPE_LAST + 1] = {
[MYDEV_TYPE_FROG] = "frog",
[MYDEV_TYPE_ALLIGATOR] = "alligator",
[MYDEV_TYPE_PLATYPUS] = "platypus",
[MYDEV_TYPE_LAST] = NULL,
};
Assuming an object struct of
typedef struct {
Object parent;
MyDevType devtype;
...other fields...
} MyDev;
The property can then be registered as follows:
static int mydev_prop_get_devtype(Object *obj,
Error **errp G_GNUC_UNUSED)
{
MyDev *dev = MYDEV(obj);
return dev->devtype;
}
static void mydev_prop_set_devtype(Object *obj,
int value,
Error **errp G_GNUC_UNUSED)
{
MyDev *dev = MYDEV(obj);
dev->endpoint = value;
}
object_property_add_enum(obj, "devtype",
mydevtypemap, "MyDevType",
mydev_prop_get_devtype,
mydev_prop_set_devtype,
NULL);
Note there is no need to check the range of 'value' in
the setter, because the string->enum conversion code will
have already done that and reported an error as required.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
include/qom/object.h | 19 ++++++++++++
qom/object.c | 58 +++++++++++++++++++++++++++++++++++++
tests/check-qom-proplist.c | 72 ++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 149 insertions(+)
diff --git a/include/qom/object.h b/include/qom/object.h
index d07a506..270a3ef 100644
--- a/include/qom/object.h
+++ b/include/qom/object.h
@@ -1344,6 +1344,25 @@ void object_property_add_bool(Object *obj, const char *name,
Error **errp);
/**
+ * object_property_add_enum:
+ * @obj: the object to add a property to
+ * @name: the name of the property
+ * @typename: the name of the enum data type
+ * @get: the getter or %NULL if the property is write-only.
+ * @set: the setter or %NULL if the property is read-only
+ * @errp: if an error occurs, a pointer to an area to store the error
+ *
+ * Add a enum property using getters/setters. This function will add a
+ * property of type '@typename'.
+ */
+void object_property_add_enum(Object *obj, const char *name,
+ const char *typename,
+ const char * const *strings,
+ int (*get)(Object *, Error **),
+ void (*set)(Object *, int, Error **),
+ Error **errp);
+
+/**
* object_property_add_tm:
* @obj: the object to add a property to
* @name: the name of the property
diff --git a/qom/object.c b/qom/object.c
index 1421e5a..f0923c5 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -1069,6 +1069,12 @@ int64_t object_property_get_int(Object *obj, const char *name,
return retval;
}
+typedef struct EnumProperty {
+ const char * const *strings;
+ int (*get)(Object *, Error **);
+ void (*set)(Object *, int, Error **);
+} EnumProperty;
+
int object_property_get_enum(Object *obj, const char *name,
const char * const strings[], Error **errp)
{
@@ -1657,6 +1663,58 @@ void object_property_add_bool(Object *obj, const char *name,
}
}
+static void property_get_enum(Object *obj, Visitor *v, void *opaque,
+ const char *name, Error **errp)
+{
+ EnumProperty *prop = opaque;
+ int value;
+
+ value = prop->get(obj, errp);
+ visit_type_enum(v, &value, prop->strings, NULL, name, errp);
+}
+
+static void property_set_enum(Object *obj, Visitor *v, void *opaque,
+ const char *name, Error **errp)
+{
+ EnumProperty *prop = opaque;
+ int value;
+
+ visit_type_enum(v, &value, prop->strings, NULL, name, errp);
+ prop->set(obj, value, errp);
+}
+
+static void property_release_enum(Object *obj, const char *name,
+ void *opaque)
+{
+ EnumProperty *prop = opaque;
+ g_free(prop);
+}
+
+void object_property_add_enum(Object *obj, const char *name,
+ const char *typename,
+ const char * const *strings,
+ int (*get)(Object *, Error **),
+ void (*set)(Object *, int, Error **),
+ Error **errp)
+{
+ Error *local_err = NULL;
+ EnumProperty *prop = g_malloc(sizeof(*prop));
+
+ prop->strings = strings;
+ prop->get = get;
+ prop->set = set;
+
+ object_property_add(obj, name, typename,
+ get ? property_get_enum : NULL,
+ set ? property_set_enum : NULL,
+ property_release_enum,
+ prop, &local_err);
+ if (local_err) {
+ error_propagate(errp, local_err);
+ g_free(prop);
+ }
+}
+
typedef struct TMProperty {
void (*get)(Object *, struct tm *, Error **);
} TMProperty;
diff --git a/tests/check-qom-proplist.c b/tests/check-qom-proplist.c
index e82532e..2266a38 100644
--- a/tests/check-qom-proplist.c
+++ b/tests/check-qom-proplist.c
@@ -32,10 +32,28 @@ typedef struct DummyObjectClass DummyObjectClass;
#define DUMMY_OBJECT(obj) \
OBJECT_CHECK(DummyObject, (obj), TYPE_DUMMY)
+typedef enum DummyAnimal DummyAnimal;
+
+enum DummyAnimal {
+ DUMMY_FROG,
+ DUMMY_ALLIGATOR,
+ DUMMY_PLATYPUS,
+
+ DUMMY_LAST,
+};
+
+static const char *const dummy_animal_map[DUMMY_LAST + 1] = {
+ [DUMMY_FROG] = "frog",
+ [DUMMY_ALLIGATOR] = "alligator",
+ [DUMMY_PLATYPUS] = "platypus",
+ [DUMMY_LAST] = NULL,
+};
+
struct DummyObject {
Object parent_obj;
bool bv;
+ DummyAnimal av;
char *sv;
};
@@ -62,6 +80,24 @@ static bool dummy_get_bv(Object *obj,
}
+static void dummy_set_av(Object *obj,
+ int value,
+ Error **errp)
+{
+ DummyObject *dobj = DUMMY_OBJECT(obj);
+
+ dobj->av = value;
+}
+
+static int dummy_get_av(Object *obj,
+ Error **errp)
+{
+ DummyObject *dobj = DUMMY_OBJECT(obj);
+
+ return dobj->av;
+}
+
+
static void dummy_set_sv(Object *obj,
const char *value,
Error **errp)
@@ -91,6 +127,12 @@ static void dummy_init(Object *obj)
dummy_get_sv,
dummy_set_sv,
NULL);
+ object_property_add_enum(obj, "av",
+ "DummyAnimal",
+ dummy_animal_map,
+ dummy_get_av,
+ dummy_set_av,
+ NULL);
}
static void dummy_finalize(Object *obj)
@@ -121,11 +163,13 @@ static void test_dummy_createv(void)
&err,
"bv", "yes",
"sv", "Hiss hiss hiss",
+ "av", "platypus",
NULL));
g_assert(err == NULL);
g_assert_cmpstr(dobj->sv, ==, "Hiss hiss hiss");
g_assert(dobj->bv == true);
+ g_assert(dobj->av == DUMMY_PLATYPUS);
g_assert(object_resolve_path_component(parent, "dummy0")
== OBJECT(dobj));
@@ -160,11 +204,13 @@ static void test_dummy_createlist(void)
parent,
"bv", "yes",
"sv", "Hiss hiss hiss",
+ "av", "platypus",
NULL));
g_assert(err == NULL);
g_assert_cmpstr(dobj->sv, ==, "Hiss hiss hiss");
g_assert(dobj->bv == true);
+ g_assert(dobj->av == DUMMY_PLATYPUS);
g_assert(object_resolve_path_component(parent, "dummy0")
== OBJECT(dobj));
@@ -172,6 +218,31 @@ static void test_dummy_createlist(void)
object_unparent(OBJECT(dobj));
}
+static void test_dummy_badenum(void)
+{
+ Error *err = NULL;
+ Object *parent = object_get_objects_root();
+ DUMMY_OBJECT(
+ object_new_with_props(TYPE_DUMMY,
+ parent,
+ "dummy0",
+ &err,
+ "bv", "yes",
+ "sv", "Hiss hiss hiss",
+ "av", "yeti",
+ NULL));
+
+ g_assert(err != NULL);
+ g_assert_cmpstr(error_get_pretty(err), ==,
+ "Invalid parameter 'yeti'");
+
+ g_assert(object_resolve_path_component(parent, "dummy0")
+ == NULL);
+
+ error_free(err);
+}
+
+
int main(int argc, char **argv)
{
g_test_init(&argc, &argv, NULL);
@@ -181,6 +252,7 @@ int main(int argc, char **argv)
g_test_add_func("/qom/proplist/createlist", test_dummy_createlist);
g_test_add_func("/qom/proplist/createv", test_dummy_createv);
+ g_test_add_func("/qom/proplist/badenum", test_dummy_badenum);
return g_test_run();
}
--
2.1.0
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [Qemu-devel] [PATCH v4 8/8] qom: don't pass string table to object_get_enum method
2015-05-13 16:14 [Qemu-devel] [PATCH v4 0/8] qom: misc fixes & enhancements to support TLS work Daniel P. Berrange
` (6 preceding siblings ...)
2015-05-13 16:14 ` [Qemu-devel] [PATCH v4 7/8] qom: add a object_property_add_enum helper method Daniel P. Berrange
@ 2015-05-13 16:14 ` Daniel P. Berrange
7 siblings, 0 replies; 23+ messages in thread
From: Daniel P. Berrange @ 2015-05-13 16:14 UTC (permalink / raw)
To: qemu-devel; +Cc: Paolo Bonzini, Andreas Färber
Now that properties can be explicitly registered as an enum
type, there is no need to pass the string table to the
object_get_enum method. The object property registration
already has a pointer to the string table.
In changing this method signature, the hostmem backend object
has to be converted to use the new enum property registration
code, which simplifies it somewhat.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
backends/hostmem.c | 22 ++++++++--------------
include/qom/object.h | 4 ++--
numa.c | 2 +-
qom/object.c | 19 +++++++++++++++++--
tests/check-qom-proplist.c | 43 +++++++++++++++++++++++++++++++++++++++++++
5 files changed, 71 insertions(+), 19 deletions(-)
diff --git a/backends/hostmem.c b/backends/hostmem.c
index f6db33c..7d74be0 100644
--- a/backends/hostmem.c
+++ b/backends/hostmem.c
@@ -113,24 +113,17 @@ host_memory_backend_set_host_nodes(Object *obj, Visitor *v, void *opaque,
#endif
}
-static void
-host_memory_backend_get_policy(Object *obj, Visitor *v, void *opaque,
- const char *name, Error **errp)
+static int
+host_memory_backend_get_policy(Object *obj, Error **errp G_GNUC_UNUSED)
{
HostMemoryBackend *backend = MEMORY_BACKEND(obj);
- int policy = backend->policy;
-
- visit_type_enum(v, &policy, HostMemPolicy_lookup, NULL, name, errp);
+ return backend->policy;
}
static void
-host_memory_backend_set_policy(Object *obj, Visitor *v, void *opaque,
- const char *name, Error **errp)
+host_memory_backend_set_policy(Object *obj, int policy, Error **errp)
{
HostMemoryBackend *backend = MEMORY_BACKEND(obj);
- int policy;
-
- visit_type_enum(v, &policy, HostMemPolicy_lookup, NULL, name, errp);
backend->policy = policy;
#ifndef CONFIG_NUMA
@@ -252,9 +245,10 @@ static void host_memory_backend_init(Object *obj)
object_property_add(obj, "host-nodes", "int",
host_memory_backend_get_host_nodes,
host_memory_backend_set_host_nodes, NULL, NULL, NULL);
- object_property_add(obj, "policy", "HostMemPolicy",
- host_memory_backend_get_policy,
- host_memory_backend_set_policy, NULL, NULL, NULL);
+ object_property_add_enum(obj, "policy", "HostMemPolicy",
+ HostMemPolicy_lookup,
+ host_memory_backend_get_policy,
+ host_memory_backend_set_policy, NULL);
}
MemoryRegion *
diff --git a/include/qom/object.h b/include/qom/object.h
index 270a3ef..334a5c3 100644
--- a/include/qom/object.h
+++ b/include/qom/object.h
@@ -1073,7 +1073,7 @@ int64_t object_property_get_int(Object *obj, const char *name,
* object_property_get_enum:
* @obj: the object
* @name: the name of the property
- * @strings: strings corresponding to enums
+ * @typename: the name of the enum data type
* @errp: returns an error if this function fails
*
* Returns: the value of the property, converted to an integer, or
@@ -1081,7 +1081,7 @@ int64_t object_property_get_int(Object *obj, const char *name,
* an enum).
*/
int object_property_get_enum(Object *obj, const char *name,
- const char * const strings[], Error **errp);
+ const char *typename, Error **errp);
/**
* object_property_get_uint16List:
diff --git a/numa.c b/numa.c
index b14a5c1..250e42d 100644
--- a/numa.c
+++ b/numa.c
@@ -457,7 +457,7 @@ static int query_memdev(Object *obj, void *opaque)
m->value->policy = object_property_get_enum(obj,
"policy",
- HostMemPolicy_lookup,
+ "HostMemPolicy",
&err);
if (err) {
goto error;
diff --git a/qom/object.c b/qom/object.c
index f0923c5..841baff 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -1076,12 +1076,27 @@ typedef struct EnumProperty {
} EnumProperty;
int object_property_get_enum(Object *obj, const char *name,
- const char * const strings[], Error **errp)
+ const char *typename, Error **errp)
{
StringOutputVisitor *sov;
StringInputVisitor *siv;
char *str;
int ret;
+ ObjectProperty *prop = object_property_find(obj, name, errp);
+ EnumProperty *enumprop;
+
+ if (prop == NULL) {
+ return 0;
+ }
+
+ if (!g_str_equal(prop->type, typename)) {
+ error_setg(errp, "Property %s on %s is not '%s' enum type",
+ name, object_class_get_name(
+ object_get_class(obj)), typename);
+ return 0;
+ }
+
+ enumprop = prop->opaque;
sov = string_output_visitor_new(false);
object_property_get(obj, string_output_get_visitor(sov), name, errp);
@@ -1089,7 +1104,7 @@ int object_property_get_enum(Object *obj, const char *name,
siv = string_input_visitor_new(str);
string_output_visitor_cleanup(sov);
visit_type_enum(string_input_get_visitor(siv),
- &ret, strings, NULL, name, errp);
+ &ret, enumprop->strings, NULL, name, errp);
g_free(str);
string_input_visitor_cleanup(siv);
diff --git a/tests/check-qom-proplist.c b/tests/check-qom-proplist.c
index 2266a38..8b764a1 100644
--- a/tests/check-qom-proplist.c
+++ b/tests/check-qom-proplist.c
@@ -243,6 +243,48 @@ static void test_dummy_badenum(void)
}
+static void test_dummy_getenum(void)
+{
+ Error *err = NULL;
+ int val;
+ Object *parent = object_get_objects_root();
+ DummyObject *dobj = DUMMY_OBJECT(
+ object_new_with_props(TYPE_DUMMY,
+ parent,
+ "dummy0",
+ &err,
+ "av", "platypus",
+ NULL));
+
+ g_assert(err == NULL);
+ g_assert(dobj->av == DUMMY_PLATYPUS);
+
+ val = object_property_get_enum(OBJECT(dobj),
+ "av",
+ "DummyAnimal",
+ &err);
+ g_assert(err == NULL);
+ g_assert(val == DUMMY_PLATYPUS);
+
+ /* A bad enum type name */
+ val = object_property_get_enum(OBJECT(dobj),
+ "av",
+ "BadAnimal",
+ &err);
+ g_assert(err != NULL);
+ error_free(err);
+ err = NULL;
+
+ /* A non-enum property name */
+ val = object_property_get_enum(OBJECT(dobj),
+ "iv",
+ "DummyAnimal",
+ &err);
+ g_assert(err != NULL);
+ error_free(err);
+}
+
+
int main(int argc, char **argv)
{
g_test_init(&argc, &argv, NULL);
@@ -253,6 +295,7 @@ int main(int argc, char **argv)
g_test_add_func("/qom/proplist/createlist", test_dummy_createlist);
g_test_add_func("/qom/proplist/createv", test_dummy_createv);
g_test_add_func("/qom/proplist/badenum", test_dummy_badenum);
+ g_test_add_func("/qom/proplist/getenum", test_dummy_getenum);
return g_test_run();
}
--
2.1.0
^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH v4 4/8] qom: add helper method for getting user objects root
2015-05-13 16:14 ` [Qemu-devel] [PATCH v4 4/8] qom: add helper method for getting user objects root Daniel P. Berrange
@ 2015-05-19 13:30 ` Andreas Färber
0 siblings, 0 replies; 23+ messages in thread
From: Andreas Färber @ 2015-05-19 13:30 UTC (permalink / raw)
To: Daniel P. Berrange, qemu-devel; +Cc: Paolo Bonzini
Am 13.05.2015 um 18:14 schrieb Daniel P. Berrange:
> Add object_get_objects_root() method which is a convience for
> obtaining the Object * located at /objects in the object
> composition tree. Convert existing code over to use the new
> API where appropriate.
>
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
> include/qom/object.h | 12 ++++++++++++
> iothread.c | 4 +---
> numa.c | 2 +-
> qmp.c | 6 +++---
> qom/object.c | 5 +++++
> 5 files changed, 22 insertions(+), 7 deletions(-)
Thanks, applied to qom-next:
https://github.com/afaerber/qemu-cpu/commits/qom-next
Andreas
--
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Dilip Upmanyu, Graham Norton; HRB
21284 (AG Nürnberg)
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH v4 5/8] qom: add object_new_with_props / object_new_withpropv constructors
2015-05-13 16:14 ` [Qemu-devel] [PATCH v4 5/8] qom: add object_new_with_props / object_new_withpropv constructors Daniel P. Berrange
@ 2015-05-19 15:52 ` Andreas Färber
2015-05-19 15:55 ` Daniel P. Berrange
0 siblings, 1 reply; 23+ messages in thread
From: Andreas Färber @ 2015-05-19 15:52 UTC (permalink / raw)
To: Daniel P. Berrange, qemu-devel; +Cc: Paolo Bonzini
Am 13.05.2015 um 18:14 schrieb Daniel P. Berrange:
> +Object *object_new_with_propv(const char *typename,
> + Object *parent,
> + const char *id,
> + Error **errp,
> + va_list vargs)
> +{
> + Object *obj;
> + ObjectClass *klass;
> + Error *local_err = NULL;
> +
> + klass = object_class_by_name(typename);
> + if (!klass) {
> + error_setg(errp, "invalid object type: %s", typename);
> + return NULL;
> + }
> +
> + if (object_class_is_abstract(klass)) {
> + error_setg(errp, "object type '%s' is abstract", typename);
> + return NULL;
> + }
> + obj = object_new(typename);
> +
> + if (object_set_propv(obj, &local_err, vargs) < 0) {
> + goto error;
> + }
> +
> + object_property_add_child(parent, id, obj, &local_err);
> + if (local_err) {
> + goto error;
> + }
> +
> + if (object_dynamic_cast(obj, TYPE_USER_CREATABLE)) {
> + user_creatable_complete(obj, &local_err);
> + if (local_err) {
> + object_unparent(obj);
> + goto error;
> + }
> + }
> +
> + object_unref(OBJECT(obj));
> + return obj;
This looks fishy. Either we return obj and need to retain a ref for that
(caller's responsibility to unref) or we unref it and return void.
Or am I misreading the code?
> +
> + error:
> + if (local_err) {
> + error_propagate(errp, local_err);
> + }
> + object_unref(obj);
> + return NULL;
> +}
[snip]
Rest looks good.
Regards,
Andreas
--
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Dilip Upmanyu, Graham Norton; HRB
21284 (AG Nürnberg)
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH v4 5/8] qom: add object_new_with_props / object_new_withpropv constructors
2015-05-19 15:52 ` Andreas Färber
@ 2015-05-19 15:55 ` Daniel P. Berrange
2015-05-19 16:08 ` Andreas Färber
2015-05-19 16:11 ` Paolo Bonzini
0 siblings, 2 replies; 23+ messages in thread
From: Daniel P. Berrange @ 2015-05-19 15:55 UTC (permalink / raw)
To: Andreas Färber; +Cc: Paolo Bonzini, qemu-devel
On Tue, May 19, 2015 at 05:52:14PM +0200, Andreas Färber wrote:
> Am 13.05.2015 um 18:14 schrieb Daniel P. Berrange:
> > +Object *object_new_with_propv(const char *typename,
> > + Object *parent,
> > + const char *id,
> > + Error **errp,
> > + va_list vargs)
> > +{
> > + Object *obj;
> > + ObjectClass *klass;
> > + Error *local_err = NULL;
> > +
> > + klass = object_class_by_name(typename);
> > + if (!klass) {
> > + error_setg(errp, "invalid object type: %s", typename);
> > + return NULL;
> > + }
> > +
> > + if (object_class_is_abstract(klass)) {
> > + error_setg(errp, "object type '%s' is abstract", typename);
> > + return NULL;
> > + }
> > + obj = object_new(typename);
> > +
> > + if (object_set_propv(obj, &local_err, vargs) < 0) {
> > + goto error;
> > + }
> > +
> > + object_property_add_child(parent, id, obj, &local_err);
> > + if (local_err) {
> > + goto error;
> > + }
> > +
> > + if (object_dynamic_cast(obj, TYPE_USER_CREATABLE)) {
> > + user_creatable_complete(obj, &local_err);
> > + if (local_err) {
> > + object_unparent(obj);
> > + goto error;
> > + }
> > + }
> > +
> > + object_unref(OBJECT(obj));
> > + return obj;
>
> This looks fishy. Either we return obj and need to retain a ref for that
> (caller's responsibility to unref) or we unref it and return void.
> Or am I misreading the code?
Paolo told me on previous posting that object_property_add_child()
holds a reference on 'obj' for as long as it is registered in the
object hierarchy composition. So it sufficient to rely on that long
term reference, and let the caller dispose of the object by calling
object_unparent(obj) when finally done.
Regards,
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH v4 5/8] qom: add object_new_with_props / object_new_withpropv constructors
2015-05-19 15:55 ` Daniel P. Berrange
@ 2015-05-19 16:08 ` Andreas Färber
2015-05-19 16:11 ` Paolo Bonzini
1 sibling, 0 replies; 23+ messages in thread
From: Andreas Färber @ 2015-05-19 16:08 UTC (permalink / raw)
To: Daniel P. Berrange; +Cc: Paolo Bonzini, qemu-devel
Am 19.05.2015 um 17:55 schrieb Daniel P. Berrange:
> On Tue, May 19, 2015 at 05:52:14PM +0200, Andreas Färber wrote:
>> Am 13.05.2015 um 18:14 schrieb Daniel P. Berrange:
>>> +Object *object_new_with_propv(const char *typename,
>>> + Object *parent,
>>> + const char *id,
>>> + Error **errp,
>>> + va_list vargs)
>>> +{
>>> + Object *obj;
>>> + ObjectClass *klass;
>>> + Error *local_err = NULL;
>>> +
>>> + klass = object_class_by_name(typename);
>>> + if (!klass) {
>>> + error_setg(errp, "invalid object type: %s", typename);
>>> + return NULL;
>>> + }
>>> +
>>> + if (object_class_is_abstract(klass)) {
>>> + error_setg(errp, "object type '%s' is abstract", typename);
>>> + return NULL;
>>> + }
>>> + obj = object_new(typename);
>>> +
>>> + if (object_set_propv(obj, &local_err, vargs) < 0) {
>>> + goto error;
>>> + }
>>> +
>>> + object_property_add_child(parent, id, obj, &local_err);
>>> + if (local_err) {
>>> + goto error;
>>> + }
>>> +
>>> + if (object_dynamic_cast(obj, TYPE_USER_CREATABLE)) {
>>> + user_creatable_complete(obj, &local_err);
>>> + if (local_err) {
>>> + object_unparent(obj);
>>> + goto error;
>>> + }
>>> + }
>>> +
>>> + object_unref(OBJECT(obj));
>>> + return obj;
>>
>> This looks fishy. Either we return obj and need to retain a ref for that
>> (caller's responsibility to unref) or we unref it and return void.
>> Or am I misreading the code?
>
> Paolo told me on previous posting that object_property_add_child()
> holds a reference on 'obj' for as long as it is registered in the
> object hierarchy composition. So it sufficient to rely on that long
> term reference, and let the caller dispose of the object by calling
> object_unparent(obj) when finally done.
Heh, that's why I like reviewing my patches myself. That reference can
go away via QMP. For the CPU I have a patch pending to fix some corner
cases. We can do that as follow-up here though.
Regards,
Andreas
--
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Dilip Upmanyu, Graham Norton; HRB
21284 (AG Nürnberg)
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH v4 5/8] qom: add object_new_with_props / object_new_withpropv constructors
2015-05-19 15:55 ` Daniel P. Berrange
2015-05-19 16:08 ` Andreas Färber
@ 2015-05-19 16:11 ` Paolo Bonzini
2015-05-20 14:44 ` Eduardo Habkost
1 sibling, 1 reply; 23+ messages in thread
From: Paolo Bonzini @ 2015-05-19 16:11 UTC (permalink / raw)
To: Daniel P. Berrange, Andreas Färber; +Cc: qemu-devel
On 19/05/2015 17:55, Daniel P. Berrange wrote:
> Paolo told me on previous posting that object_property_add_child()
> holds a reference on 'obj' for as long as it is registered in the
> object hierarchy composition. So it sufficient to rely on that long
> term reference, and let the caller dispose of the object by calling
> object_unparent(obj) when finally done.
For an example of the same pattern:
DeviceState *qdev_try_create(BusState *bus, const char *type)
{
DeviceState *dev;
if (object_class_by_name(type) == NULL) {
return NULL;
}
dev = DEVICE(object_new(type));
if (!dev) {
return NULL;
}
if (!bus) {
bus = sysbus_get_default();
}
qdev_set_parent_bus(dev, bus);
object_unref(OBJECT(dev));
return dev;
}
Effectively this is idea as GObject's "floating reference".
qdev_set_parent_bus (in qdev_try_create) and object_property_add_child
(in Daniel's patches) "sink" the floating reference by doing
object_unref. If we had floating references, the object would be
returned to the caller unref'ed anyway.
Of course, the reference can go away via QMP. But that will only happen
after the caller would have called object_unref itself.
Paolo
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH v4 5/8] qom: add object_new_with_props / object_new_withpropv constructors
2015-05-19 16:11 ` Paolo Bonzini
@ 2015-05-20 14:44 ` Eduardo Habkost
2015-05-20 15:18 ` Daniel P. Berrange
0 siblings, 1 reply; 23+ messages in thread
From: Eduardo Habkost @ 2015-05-20 14:44 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: Andreas Färber, qemu-devel
On Tue, May 19, 2015 at 06:11:05PM +0200, Paolo Bonzini wrote:
> On 19/05/2015 17:55, Daniel P. Berrange wrote:
> > Paolo told me on previous posting that object_property_add_child()
> > holds a reference on 'obj' for as long as it is registered in the
> > object hierarchy composition. So it sufficient to rely on that long
> > term reference, and let the caller dispose of the object by calling
> > object_unparent(obj) when finally done.
>
> For an example of the same pattern:
>
> DeviceState *qdev_try_create(BusState *bus, const char *type)
> {
> DeviceState *dev;
>
> if (object_class_by_name(type) == NULL) {
> return NULL;
> }
> dev = DEVICE(object_new(type));
> if (!dev) {
> return NULL;
> }
>
> if (!bus) {
> bus = sysbus_get_default();
> }
>
> qdev_set_parent_bus(dev, bus);
> object_unref(OBJECT(dev));
> return dev;
> }
>
> Effectively this is idea as GObject's "floating reference".
> qdev_set_parent_bus (in qdev_try_create) and object_property_add_child
> (in Daniel's patches) "sink" the floating reference by doing
> object_unref. If we had floating references, the object would be
> returned to the caller unref'ed anyway.
I was agreeing with Andreas at first (because it would make the
reference ownership rules simpler and easier to understand), until I
noticed that every call of qdev_try_create() and object_resolve_path()
in the code would need an additional object_unref() call if we didn't
use this pattern.
But it bothers me that this exceptional behavior is not documented on
neither qdev_try_create() or object_resolve_path().
>
> Of course, the reference can go away via QMP. But that will only happen
> after the caller would have called object_unref itself.
But the caller won't ever call object_unref() because it doesn't own any
reference, right? In this case, can we clarify the rules about how long
can callers safely expect the object to stay around? Can the object be
disposed in another thread? Can it be disposed only when some specific
events happen?
--
Eduardo
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH v4 5/8] qom: add object_new_with_props / object_new_withpropv constructors
2015-05-20 14:44 ` Eduardo Habkost
@ 2015-05-20 15:18 ` Daniel P. Berrange
2015-05-20 15:32 ` Andreas Färber
2015-05-20 16:06 ` Eduardo Habkost
0 siblings, 2 replies; 23+ messages in thread
From: Daniel P. Berrange @ 2015-05-20 15:18 UTC (permalink / raw)
To: Eduardo Habkost; +Cc: Paolo Bonzini, Andreas Färber, qemu-devel
On Wed, May 20, 2015 at 11:44:19AM -0300, Eduardo Habkost wrote:
> On Tue, May 19, 2015 at 06:11:05PM +0200, Paolo Bonzini wrote:
> > On 19/05/2015 17:55, Daniel P. Berrange wrote:
> > > Paolo told me on previous posting that object_property_add_child()
> > > holds a reference on 'obj' for as long as it is registered in the
> > > object hierarchy composition. So it sufficient to rely on that long
> > > term reference, and let the caller dispose of the object by calling
> > > object_unparent(obj) when finally done.
> >
> > For an example of the same pattern:
> >
> > DeviceState *qdev_try_create(BusState *bus, const char *type)
> > {
> > DeviceState *dev;
> >
> > if (object_class_by_name(type) == NULL) {
> > return NULL;
> > }
> > dev = DEVICE(object_new(type));
> > if (!dev) {
> > return NULL;
> > }
> >
> > if (!bus) {
> > bus = sysbus_get_default();
> > }
> >
> > qdev_set_parent_bus(dev, bus);
> > object_unref(OBJECT(dev));
> > return dev;
> > }
> >
> > Effectively this is idea as GObject's "floating reference".
> > qdev_set_parent_bus (in qdev_try_create) and object_property_add_child
> > (in Daniel's patches) "sink" the floating reference by doing
> > object_unref. If we had floating references, the object would be
> > returned to the caller unref'ed anyway.
>
> I was agreeing with Andreas at first (because it would make the
> reference ownership rules simpler and easier to understand), until I
> noticed that every call of qdev_try_create() and object_resolve_path()
> in the code would need an additional object_unref() call if we didn't
> use this pattern.
>
> But it bothers me that this exceptional behavior is not documented on
> neither qdev_try_create() or object_resolve_path().
>
> >
> > Of course, the reference can go away via QMP. But that will only happen
> > after the caller would have called object_unref itself.
>
> But the caller won't ever call object_unref() because it doesn't own any
> reference, right? In this case, can we clarify the rules about how long
> can callers safely expect the object to stay around? Can the object be
> disposed in another thread? Can it be disposed only when some specific
> events happen?
In the inline docs for object_new_with_props I wrote
* The returned object will have one stable reference maintained
* for as long as it is present in the object hierarchy.
We could expand it to explicitly say that 'object_unparent' is required
to remove the object from the hierarchy and free it.
Regards,
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH v4 5/8] qom: add object_new_with_props / object_new_withpropv constructors
2015-05-20 15:18 ` Daniel P. Berrange
@ 2015-05-20 15:32 ` Andreas Färber
2015-05-20 16:06 ` Eduardo Habkost
1 sibling, 0 replies; 23+ messages in thread
From: Andreas Färber @ 2015-05-20 15:32 UTC (permalink / raw)
To: Daniel P. Berrange, Eduardo Habkost; +Cc: Paolo Bonzini, qemu-devel
Am 20.05.2015 um 17:18 schrieb Daniel P. Berrange:
> On Wed, May 20, 2015 at 11:44:19AM -0300, Eduardo Habkost wrote:
>> On Tue, May 19, 2015 at 06:11:05PM +0200, Paolo Bonzini wrote:
>>> On 19/05/2015 17:55, Daniel P. Berrange wrote:
>>>> Paolo told me on previous posting that object_property_add_child()
>>>> holds a reference on 'obj' for as long as it is registered in the
>>>> object hierarchy composition. So it sufficient to rely on that long
>>>> term reference, and let the caller dispose of the object by calling
>>>> object_unparent(obj) when finally done.
>>>
>>> For an example of the same pattern:
>>>
>>> DeviceState *qdev_try_create(BusState *bus, const char *type)
>>> {
>>> DeviceState *dev;
>>>
>>> if (object_class_by_name(type) == NULL) {
>>> return NULL;
>>> }
>>> dev = DEVICE(object_new(type));
>>> if (!dev) {
>>> return NULL;
>>> }
>>>
>>> if (!bus) {
>>> bus = sysbus_get_default();
>>> }
>>>
>>> qdev_set_parent_bus(dev, bus);
>>> object_unref(OBJECT(dev));
>>> return dev;
>>> }
>>>
>>> Effectively this is idea as GObject's "floating reference".
>>> qdev_set_parent_bus (in qdev_try_create) and object_property_add_child
>>> (in Daniel's patches) "sink" the floating reference by doing
>>> object_unref. If we had floating references, the object would be
>>> returned to the caller unref'ed anyway.
>>
>> I was agreeing with Andreas at first (because it would make the
>> reference ownership rules simpler and easier to understand), until I
>> noticed that every call of qdev_try_create() and object_resolve_path()
>> in the code would need an additional object_unref() call if we didn't
>> use this pattern.
>>
>> But it bothers me that this exceptional behavior is not documented on
>> neither qdev_try_create() or object_resolve_path().
>>
>>>
>>> Of course, the reference can go away via QMP. But that will only happen
>>> after the caller would have called object_unref itself.
>>
>> But the caller won't ever call object_unref() because it doesn't own any
>> reference, right? In this case, can we clarify the rules about how long
>> can callers safely expect the object to stay around? Can the object be
>> disposed in another thread? Can it be disposed only when some specific
>> events happen?
>
> In the inline docs for object_new_with_props I wrote
>
> * The returned object will have one stable reference maintained
> * for as long as it is present in the object hierarchy.
>
> We could expand it to explicitly say that 'object_unparent' is required
> to remove the object from the hierarchy and free it.
I've already queued this series - let's hold off changes for a bit. :)
Regards,
Andreas
--
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Dilip Upmanyu, Graham Norton; HRB
21284 (AG Nürnberg)
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH v4 5/8] qom: add object_new_with_props / object_new_withpropv constructors
2015-05-20 15:18 ` Daniel P. Berrange
2015-05-20 15:32 ` Andreas Färber
@ 2015-05-20 16:06 ` Eduardo Habkost
2015-05-20 16:10 ` Daniel P. Berrange
2015-05-20 16:11 ` Andreas Färber
1 sibling, 2 replies; 23+ messages in thread
From: Eduardo Habkost @ 2015-05-20 16:06 UTC (permalink / raw)
To: Daniel P. Berrange; +Cc: Paolo Bonzini, Andreas Färber, qemu-devel
On Wed, May 20, 2015 at 04:18:03PM +0100, Daniel P. Berrange wrote:
> On Wed, May 20, 2015 at 11:44:19AM -0300, Eduardo Habkost wrote:
> > On Tue, May 19, 2015 at 06:11:05PM +0200, Paolo Bonzini wrote:
> > > On 19/05/2015 17:55, Daniel P. Berrange wrote:
> > > > Paolo told me on previous posting that object_property_add_child()
> > > > holds a reference on 'obj' for as long as it is registered in the
> > > > object hierarchy composition. So it sufficient to rely on that long
> > > > term reference, and let the caller dispose of the object by calling
> > > > object_unparent(obj) when finally done.
> > >
> > > For an example of the same pattern:
> > >
> > > DeviceState *qdev_try_create(BusState *bus, const char *type)
> > > {
> > > DeviceState *dev;
> > >
> > > if (object_class_by_name(type) == NULL) {
> > > return NULL;
> > > }
> > > dev = DEVICE(object_new(type));
> > > if (!dev) {
> > > return NULL;
> > > }
> > >
> > > if (!bus) {
> > > bus = sysbus_get_default();
> > > }
> > >
> > > qdev_set_parent_bus(dev, bus);
> > > object_unref(OBJECT(dev));
> > > return dev;
> > > }
> > >
> > > Effectively this is idea as GObject's "floating reference".
> > > qdev_set_parent_bus (in qdev_try_create) and object_property_add_child
> > > (in Daniel's patches) "sink" the floating reference by doing
> > > object_unref. If we had floating references, the object would be
> > > returned to the caller unref'ed anyway.
> >
> > I was agreeing with Andreas at first (because it would make the
> > reference ownership rules simpler and easier to understand), until I
> > noticed that every call of qdev_try_create() and object_resolve_path()
> > in the code would need an additional object_unref() call if we didn't
> > use this pattern.
> >
> > But it bothers me that this exceptional behavior is not documented on
> > neither qdev_try_create() or object_resolve_path().
> >
> > >
> > > Of course, the reference can go away via QMP. But that will only happen
> > > after the caller would have called object_unref itself.
> >
> > But the caller won't ever call object_unref() because it doesn't own any
> > reference, right? In this case, can we clarify the rules about how long
> > can callers safely expect the object to stay around? Can the object be
> > disposed in another thread? Can it be disposed only when some specific
> > events happen?
>
> In the inline docs for object_new_with_props I wrote
>
> * The returned object will have one stable reference maintained
> * for as long as it is present in the object hierarchy.
>
> We could expand it to explicitly say that 'object_unparent' is required
> to remove the object from the hierarchy and free it.
What's missing to me is some clarification on how long it is safe to
assume that the object won't be removed from the hierarchy by other
code.
--
Eduardo
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH v4 5/8] qom: add object_new_with_props / object_new_withpropv constructors
2015-05-20 16:06 ` Eduardo Habkost
@ 2015-05-20 16:10 ` Daniel P. Berrange
2015-05-20 16:11 ` Andreas Färber
1 sibling, 0 replies; 23+ messages in thread
From: Daniel P. Berrange @ 2015-05-20 16:10 UTC (permalink / raw)
To: Eduardo Habkost; +Cc: Paolo Bonzini, Andreas Färber, qemu-devel
On Wed, May 20, 2015 at 01:06:21PM -0300, Eduardo Habkost wrote:
> On Wed, May 20, 2015 at 04:18:03PM +0100, Daniel P. Berrange wrote:
> > On Wed, May 20, 2015 at 11:44:19AM -0300, Eduardo Habkost wrote:
> > > On Tue, May 19, 2015 at 06:11:05PM +0200, Paolo Bonzini wrote:
> > > > On 19/05/2015 17:55, Daniel P. Berrange wrote:
> > > > > Paolo told me on previous posting that object_property_add_child()
> > > > > holds a reference on 'obj' for as long as it is registered in the
> > > > > object hierarchy composition. So it sufficient to rely on that long
> > > > > term reference, and let the caller dispose of the object by calling
> > > > > object_unparent(obj) when finally done.
> > > >
> > > > For an example of the same pattern:
> > > >
> > > > DeviceState *qdev_try_create(BusState *bus, const char *type)
> > > > {
> > > > DeviceState *dev;
> > > >
> > > > if (object_class_by_name(type) == NULL) {
> > > > return NULL;
> > > > }
> > > > dev = DEVICE(object_new(type));
> > > > if (!dev) {
> > > > return NULL;
> > > > }
> > > >
> > > > if (!bus) {
> > > > bus = sysbus_get_default();
> > > > }
> > > >
> > > > qdev_set_parent_bus(dev, bus);
> > > > object_unref(OBJECT(dev));
> > > > return dev;
> > > > }
> > > >
> > > > Effectively this is idea as GObject's "floating reference".
> > > > qdev_set_parent_bus (in qdev_try_create) and object_property_add_child
> > > > (in Daniel's patches) "sink" the floating reference by doing
> > > > object_unref. If we had floating references, the object would be
> > > > returned to the caller unref'ed anyway.
> > >
> > > I was agreeing with Andreas at first (because it would make the
> > > reference ownership rules simpler and easier to understand), until I
> > > noticed that every call of qdev_try_create() and object_resolve_path()
> > > in the code would need an additional object_unref() call if we didn't
> > > use this pattern.
> > >
> > > But it bothers me that this exceptional behavior is not documented on
> > > neither qdev_try_create() or object_resolve_path().
> > >
> > > >
> > > > Of course, the reference can go away via QMP. But that will only happen
> > > > after the caller would have called object_unref itself.
> > >
> > > But the caller won't ever call object_unref() because it doesn't own any
> > > reference, right? In this case, can we clarify the rules about how long
> > > can callers safely expect the object to stay around? Can the object be
> > > disposed in another thread? Can it be disposed only when some specific
> > > events happen?
> >
> > In the inline docs for object_new_with_props I wrote
> >
> > * The returned object will have one stable reference maintained
> > * for as long as it is present in the object hierarchy.
> >
> > We could expand it to explicitly say that 'object_unparent' is required
> > to remove the object from the hierarchy and free it.
>
> What's missing to me is some clarification on how long it is safe to
> assume that the object won't be removed from the hierarchy by other
> code.
It seems that there is implicit assumption existing in QEMU, that objects
will only be deleted from the hierarchy by code running in the main event
loop thread. So if a method in the main event loop has got a pointer to
the object it can assume it is safe to use. If anything spawns off a
background thread, or passes the object pointer to a non-main loop thread,
then it should first have acquired an extra reference on the object, and
that thread would have to release the reference when done. I guess this
needs documenting somewhere for clarity
Regards,
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH v4 5/8] qom: add object_new_with_props / object_new_withpropv constructors
2015-05-20 16:06 ` Eduardo Habkost
2015-05-20 16:10 ` Daniel P. Berrange
@ 2015-05-20 16:11 ` Andreas Färber
2015-05-20 16:22 ` Eduardo Habkost
1 sibling, 1 reply; 23+ messages in thread
From: Andreas Färber @ 2015-05-20 16:11 UTC (permalink / raw)
To: Eduardo Habkost; +Cc: Paolo Bonzini, qemu-devel
Am 20.05.2015 um 18:06 schrieb Eduardo Habkost:
> On Wed, May 20, 2015 at 04:18:03PM +0100, Daniel P. Berrange wrote:
>> On Wed, May 20, 2015 at 11:44:19AM -0300, Eduardo Habkost wrote:
>>> On Tue, May 19, 2015 at 06:11:05PM +0200, Paolo Bonzini wrote:
>>>> On 19/05/2015 17:55, Daniel P. Berrange wrote:
>>>>> Paolo told me on previous posting that object_property_add_child()
>>>>> holds a reference on 'obj' for as long as it is registered in the
>>>>> object hierarchy composition. So it sufficient to rely on that long
>>>>> term reference, and let the caller dispose of the object by calling
>>>>> object_unparent(obj) when finally done.
>>>>
>>>> For an example of the same pattern:
>>>>
>>>> DeviceState *qdev_try_create(BusState *bus, const char *type)
>>>> {
>>>> DeviceState *dev;
>>>>
>>>> if (object_class_by_name(type) == NULL) {
>>>> return NULL;
>>>> }
>>>> dev = DEVICE(object_new(type));
>>>> if (!dev) {
>>>> return NULL;
>>>> }
>>>>
>>>> if (!bus) {
>>>> bus = sysbus_get_default();
>>>> }
>>>>
>>>> qdev_set_parent_bus(dev, bus);
>>>> object_unref(OBJECT(dev));
>>>> return dev;
>>>> }
>>>>
>>>> Effectively this is idea as GObject's "floating reference".
>>>> qdev_set_parent_bus (in qdev_try_create) and object_property_add_child
>>>> (in Daniel's patches) "sink" the floating reference by doing
>>>> object_unref. If we had floating references, the object would be
>>>> returned to the caller unref'ed anyway.
>>>
>>> I was agreeing with Andreas at first (because it would make the
>>> reference ownership rules simpler and easier to understand), until I
>>> noticed that every call of qdev_try_create() and object_resolve_path()
>>> in the code would need an additional object_unref() call if we didn't
>>> use this pattern.
>>>
>>> But it bothers me that this exceptional behavior is not documented on
>>> neither qdev_try_create() or object_resolve_path().
>>>
>>>>
>>>> Of course, the reference can go away via QMP. But that will only happen
>>>> after the caller would have called object_unref itself.
>>>
>>> But the caller won't ever call object_unref() because it doesn't own any
>>> reference, right? In this case, can we clarify the rules about how long
>>> can callers safely expect the object to stay around? Can the object be
>>> disposed in another thread? Can it be disposed only when some specific
>>> events happen?
>>
>> In the inline docs for object_new_with_props I wrote
>>
>> * The returned object will have one stable reference maintained
>> * for as long as it is present in the object hierarchy.
>>
>> We could expand it to explicitly say that 'object_unparent' is required
>> to remove the object from the hierarchy and free it.
>
> What's missing to me is some clarification on how long it is safe to
> assume that the object won't be removed from the hierarchy by other
> code.
There is no guarantee. Therefore any caller of object_resolve_path()
must ref and unref as needed if doing more than a single operation, such
as setting a link target.
qdev is a different matter - there's too many instances to fix properly
right away. The current code serves to balance the ref count for
hot-unplug etc. to work. But it is not a template to copy for new QOM
code IMO.
Regards,
Andreas
--
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Dilip Upmanyu, Graham Norton; HRB
21284 (AG Nürnberg)
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH v4 5/8] qom: add object_new_with_props / object_new_withpropv constructors
2015-05-20 16:11 ` Andreas Färber
@ 2015-05-20 16:22 ` Eduardo Habkost
2015-05-20 16:24 ` Andreas Färber
0 siblings, 1 reply; 23+ messages in thread
From: Eduardo Habkost @ 2015-05-20 16:22 UTC (permalink / raw)
To: Andreas Färber; +Cc: Paolo Bonzini, qemu-devel
On Wed, May 20, 2015 at 06:11:33PM +0200, Andreas Färber wrote:
> Am 20.05.2015 um 18:06 schrieb Eduardo Habkost:
> > On Wed, May 20, 2015 at 04:18:03PM +0100, Daniel P. Berrange wrote:
> >> On Wed, May 20, 2015 at 11:44:19AM -0300, Eduardo Habkost wrote:
> >>> On Tue, May 19, 2015 at 06:11:05PM +0200, Paolo Bonzini wrote:
> >>>> On 19/05/2015 17:55, Daniel P. Berrange wrote:
> >>>>> Paolo told me on previous posting that object_property_add_child()
> >>>>> holds a reference on 'obj' for as long as it is registered in the
> >>>>> object hierarchy composition. So it sufficient to rely on that long
> >>>>> term reference, and let the caller dispose of the object by calling
> >>>>> object_unparent(obj) when finally done.
> >>>>
> >>>> For an example of the same pattern:
> >>>>
> >>>> DeviceState *qdev_try_create(BusState *bus, const char *type)
> >>>> {
> >>>> DeviceState *dev;
> >>>>
> >>>> if (object_class_by_name(type) == NULL) {
> >>>> return NULL;
> >>>> }
> >>>> dev = DEVICE(object_new(type));
> >>>> if (!dev) {
> >>>> return NULL;
> >>>> }
> >>>>
> >>>> if (!bus) {
> >>>> bus = sysbus_get_default();
> >>>> }
> >>>>
> >>>> qdev_set_parent_bus(dev, bus);
> >>>> object_unref(OBJECT(dev));
> >>>> return dev;
> >>>> }
> >>>>
> >>>> Effectively this is idea as GObject's "floating reference".
> >>>> qdev_set_parent_bus (in qdev_try_create) and object_property_add_child
> >>>> (in Daniel's patches) "sink" the floating reference by doing
> >>>> object_unref. If we had floating references, the object would be
> >>>> returned to the caller unref'ed anyway.
> >>>
> >>> I was agreeing with Andreas at first (because it would make the
> >>> reference ownership rules simpler and easier to understand), until I
> >>> noticed that every call of qdev_try_create() and object_resolve_path()
> >>> in the code would need an additional object_unref() call if we didn't
> >>> use this pattern.
> >>>
> >>> But it bothers me that this exceptional behavior is not documented on
> >>> neither qdev_try_create() or object_resolve_path().
> >>>
> >>>>
> >>>> Of course, the reference can go away via QMP. But that will only happen
> >>>> after the caller would have called object_unref itself.
> >>>
> >>> But the caller won't ever call object_unref() because it doesn't own any
> >>> reference, right? In this case, can we clarify the rules about how long
> >>> can callers safely expect the object to stay around? Can the object be
> >>> disposed in another thread? Can it be disposed only when some specific
> >>> events happen?
> >>
> >> In the inline docs for object_new_with_props I wrote
> >>
> >> * The returned object will have one stable reference maintained
> >> * for as long as it is present in the object hierarchy.
> >>
> >> We could expand it to explicitly say that 'object_unparent' is required
> >> to remove the object from the hierarchy and free it.
> >
> > What's missing to me is some clarification on how long it is safe to
> > assume that the object won't be removed from the hierarchy by other
> > code.
>
> There is no guarantee. Therefore any caller of object_resolve_path()
> must ref and unref as needed if doing more than a single operation, such
> as setting a link target.
So, does that mean zero guarantee, or guarantee for "a single
operation"? It can't be both.
(And how exactly "a single operation" should be defined?)
--
Eduardo
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH v4 5/8] qom: add object_new_with_props / object_new_withpropv constructors
2015-05-20 16:22 ` Eduardo Habkost
@ 2015-05-20 16:24 ` Andreas Färber
2015-05-20 16:44 ` Eduardo Habkost
0 siblings, 1 reply; 23+ messages in thread
From: Andreas Färber @ 2015-05-20 16:24 UTC (permalink / raw)
To: Eduardo Habkost; +Cc: Paolo Bonzini, qemu-devel
Am 20.05.2015 um 18:22 schrieb Eduardo Habkost:
> On Wed, May 20, 2015 at 06:11:33PM +0200, Andreas Färber wrote:
>> Am 20.05.2015 um 18:06 schrieb Eduardo Habkost:
>>> On Wed, May 20, 2015 at 04:18:03PM +0100, Daniel P. Berrange wrote:
>>>> On Wed, May 20, 2015 at 11:44:19AM -0300, Eduardo Habkost wrote:
>>>>> On Tue, May 19, 2015 at 06:11:05PM +0200, Paolo Bonzini wrote:
>>>>>> On 19/05/2015 17:55, Daniel P. Berrange wrote:
>>>>>>> Paolo told me on previous posting that object_property_add_child()
>>>>>>> holds a reference on 'obj' for as long as it is registered in the
>>>>>>> object hierarchy composition. So it sufficient to rely on that long
>>>>>>> term reference, and let the caller dispose of the object by calling
>>>>>>> object_unparent(obj) when finally done.
>>>>>>
>>>>>> For an example of the same pattern:
>>>>>>
>>>>>> DeviceState *qdev_try_create(BusState *bus, const char *type)
>>>>>> {
>>>>>> DeviceState *dev;
>>>>>>
>>>>>> if (object_class_by_name(type) == NULL) {
>>>>>> return NULL;
>>>>>> }
>>>>>> dev = DEVICE(object_new(type));
>>>>>> if (!dev) {
>>>>>> return NULL;
>>>>>> }
>>>>>>
>>>>>> if (!bus) {
>>>>>> bus = sysbus_get_default();
>>>>>> }
>>>>>>
>>>>>> qdev_set_parent_bus(dev, bus);
>>>>>> object_unref(OBJECT(dev));
>>>>>> return dev;
>>>>>> }
>>>>>>
>>>>>> Effectively this is idea as GObject's "floating reference".
>>>>>> qdev_set_parent_bus (in qdev_try_create) and object_property_add_child
>>>>>> (in Daniel's patches) "sink" the floating reference by doing
>>>>>> object_unref. If we had floating references, the object would be
>>>>>> returned to the caller unref'ed anyway.
>>>>>
>>>>> I was agreeing with Andreas at first (because it would make the
>>>>> reference ownership rules simpler and easier to understand), until I
>>>>> noticed that every call of qdev_try_create() and object_resolve_path()
>>>>> in the code would need an additional object_unref() call if we didn't
>>>>> use this pattern.changes
>>>>>
>>>>> But it bothers me that this exceptional behavior is not documented on
>>>>> neither qdev_try_create() or object_resolve_path().
>>>>>
>>>>>>
>>>>>> Of course, the reference can go away via QMP. But that will only happen
>>>>>> after the caller would have called object_unref itself.
>>>>>
>>>>> But the caller won't ever call object_unref() because it doesn't own any
>>>>> reference, right? In this case, can we clarify the rules about how long
>>>>> can callers safely expect the object to stay around? Can the object be
>>>>> disposed in another thread? Can it be disposed only when some specific
>>>>> events happen?
>>>>
>>>> In the inline docs for object_new_with_props I wrote
>>>>
>>>> * The returned object will have one stable reference maintained
>>>> * for as long as it is present in the object hierarchy.
>>>>
>>>> We could expand it to explicitly say that 'object_unparent' is required
>>>> to remove the object from the hierarchy and free it.
>>>
>>> What's missing to me is some clarification on how long it is safe to
>>> assume that the object won't be removed from the hierarchy by other
>>> code.
>>
>> There is no guarantee. Therefore any caller of object_resolve_path()
>> must ref and unref as needed if doing more than a single operation, such
>> as setting a link target.
>
> So, does that mean zero guarantee, or guarantee for "a single
> operation"? It can't be both.
>
> (And how exactly "a single operation" should be defined?)
I see your point, after all object_ref() is a single instruction, too.
We could of course change it to return a ref'ed object, but that would
require a lot of code review and probably unrefs. Mostly for objects
that don't go away, but still.
Andreas
--
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Dilip Upmanyu, Graham Norton; HRB
21284 (AG Nürnberg)
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH v4 5/8] qom: add object_new_with_props / object_new_withpropv constructors
2015-05-20 16:24 ` Andreas Färber
@ 2015-05-20 16:44 ` Eduardo Habkost
0 siblings, 0 replies; 23+ messages in thread
From: Eduardo Habkost @ 2015-05-20 16:44 UTC (permalink / raw)
To: Andreas Färber; +Cc: Paolo Bonzini, qemu-devel
On Wed, May 20, 2015 at 06:24:47PM +0200, Andreas Färber wrote:
> Am 20.05.2015 um 18:22 schrieb Eduardo Habkost:
> > On Wed, May 20, 2015 at 06:11:33PM +0200, Andreas Färber wrote:
> >> Am 20.05.2015 um 18:06 schrieb Eduardo Habkost:
> >>> On Wed, May 20, 2015 at 04:18:03PM +0100, Daniel P. Berrange wrote:
> >>>> On Wed, May 20, 2015 at 11:44:19AM -0300, Eduardo Habkost wrote:
> >>>>> On Tue, May 19, 2015 at 06:11:05PM +0200, Paolo Bonzini wrote:
> >>>>>> On 19/05/2015 17:55, Daniel P. Berrange wrote:
> >>>>>>> Paolo told me on previous posting that object_property_add_child()
> >>>>>>> holds a reference on 'obj' for as long as it is registered in the
> >>>>>>> object hierarchy composition. So it sufficient to rely on that long
> >>>>>>> term reference, and let the caller dispose of the object by calling
> >>>>>>> object_unparent(obj) when finally done.
> >>>>>>
> >>>>>> For an example of the same pattern:
> >>>>>>
> >>>>>> DeviceState *qdev_try_create(BusState *bus, const char *type)
> >>>>>> {
> >>>>>> DeviceState *dev;
> >>>>>>
> >>>>>> if (object_class_by_name(type) == NULL) {
> >>>>>> return NULL;
> >>>>>> }
> >>>>>> dev = DEVICE(object_new(type));
> >>>>>> if (!dev) {
> >>>>>> return NULL;
> >>>>>> }
> >>>>>>
> >>>>>> if (!bus) {
> >>>>>> bus = sysbus_get_default();
> >>>>>> }
> >>>>>>
> >>>>>> qdev_set_parent_bus(dev, bus);
> >>>>>> object_unref(OBJECT(dev));
> >>>>>> return dev;
> >>>>>> }
> >>>>>>
> >>>>>> Effectively this is idea as GObject's "floating reference".
> >>>>>> qdev_set_parent_bus (in qdev_try_create) and object_property_add_child
> >>>>>> (in Daniel's patches) "sink" the floating reference by doing
> >>>>>> object_unref. If we had floating references, the object would be
> >>>>>> returned to the caller unref'ed anyway.
> >>>>>
> >>>>> I was agreeing with Andreas at first (because it would make the
> >>>>> reference ownership rules simpler and easier to understand), until I
> >>>>> noticed that every call of qdev_try_create() and object_resolve_path()
> >>>>> in the code would need an additional object_unref() call if we didn't
> >>>>> use this pattern.changes
> >>>>>
> >>>>> But it bothers me that this exceptional behavior is not documented on
> >>>>> neither qdev_try_create() or object_resolve_path().
> >>>>>
> >>>>>>
> >>>>>> Of course, the reference can go away via QMP. But that will only happen
> >>>>>> after the caller would have called object_unref itself.
> >>>>>
> >>>>> But the caller won't ever call object_unref() because it doesn't own any
> >>>>> reference, right? In this case, can we clarify the rules about how long
> >>>>> can callers safely expect the object to stay around? Can the object be
> >>>>> disposed in another thread? Can it be disposed only when some specific
> >>>>> events happen?
> >>>>
> >>>> In the inline docs for object_new_with_props I wrote
> >>>>
> >>>> * The returned object will have one stable reference maintained
> >>>> * for as long as it is present in the object hierarchy.
> >>>>
> >>>> We could expand it to explicitly say that 'object_unparent' is required
> >>>> to remove the object from the hierarchy and free it.
> >>>
> >>> What's missing to me is some clarification on how long it is safe to
> >>> assume that the object won't be removed from the hierarchy by other
> >>> code.
> >>
> >> There is no guarantee. Therefore any caller of object_resolve_path()
> >> must ref and unref as needed if doing more than a single operation, such
> >> as setting a link target.
> >
> > So, does that mean zero guarantee, or guarantee for "a single
> > operation"? It can't be both.
> >
> > (And how exactly "a single operation" should be defined?)
>
> I see your point, after all object_ref() is a single instruction, too.
>
> We could of course change it to return a ref'ed object, but that would
> require a lot of code review and probably unrefs. Mostly for objects
> that don't go away, but still.
I don't think we should change it to return a ref'ed object, the
existing "floating reference" pattern is useful and keeps code simpler.
But even if we have plans to change it later, it would be nice to
clarify what are the assumptions/guarantees of the existing code.
--
Eduardo
^ permalink raw reply [flat|nested] 23+ messages in thread
end of thread, other threads:[~2015-05-20 16:44 UTC | newest]
Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-05-13 16:14 [Qemu-devel] [PATCH v4 0/8] qom: misc fixes & enhancements to support TLS work Daniel P. Berrange
2015-05-13 16:14 ` [Qemu-devel] [PATCH v4 1/8] backends: fix typename of 'policy' enum property in hostmem obj Daniel P. Berrange
2015-05-13 16:14 ` [Qemu-devel] [PATCH v4 2/8] doc: document user creatable object types in help text Daniel P. Berrange
2015-05-13 16:14 ` [Qemu-devel] [PATCH v4 3/8] vl: create (most) objects before creating chardev backends Daniel P. Berrange
2015-05-13 16:14 ` [Qemu-devel] [PATCH v4 4/8] qom: add helper method for getting user objects root Daniel P. Berrange
2015-05-19 13:30 ` Andreas Färber
2015-05-13 16:14 ` [Qemu-devel] [PATCH v4 5/8] qom: add object_new_with_props / object_new_withpropv constructors Daniel P. Berrange
2015-05-19 15:52 ` Andreas Färber
2015-05-19 15:55 ` Daniel P. Berrange
2015-05-19 16:08 ` Andreas Färber
2015-05-19 16:11 ` Paolo Bonzini
2015-05-20 14:44 ` Eduardo Habkost
2015-05-20 15:18 ` Daniel P. Berrange
2015-05-20 15:32 ` Andreas Färber
2015-05-20 16:06 ` Eduardo Habkost
2015-05-20 16:10 ` Daniel P. Berrange
2015-05-20 16:11 ` Andreas Färber
2015-05-20 16:22 ` Eduardo Habkost
2015-05-20 16:24 ` Andreas Färber
2015-05-20 16:44 ` Eduardo Habkost
2015-05-13 16:14 ` [Qemu-devel] [PATCH v4 6/8] qom: make enum string tables const-correct Daniel P. Berrange
2015-05-13 16:14 ` [Qemu-devel] [PATCH v4 7/8] qom: add a object_property_add_enum helper method Daniel P. Berrange
2015-05-13 16:14 ` [Qemu-devel] [PATCH v4 8/8] qom: don't pass string table to object_get_enum method Daniel P. Berrange
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.