* [Qemu-devel] [PATCH v2 0/3] simplify struct QOM properties and use the result for GUEST_PANICKED
@ 2017-02-22 18:04 Paolo Bonzini
2017-02-22 18:04 ` [Qemu-devel] [PATCH 1/3] qom-qobject: introduce object_property_{g, s}et_ptr Paolo Bonzini
` (4 more replies)
0 siblings, 5 replies; 13+ messages in thread
From: Paolo Bonzini @ 2017-02-22 18:04 UTC (permalink / raw)
To: qemu-devel; +Cc: eblake, marcandre.lureau
This is an alternative approach to simplifying the crash information
patches.
Currently, crash information is exposed twice, through a QOM property
and through a method. This is because accessing QOM properties with
QAPI struct types is a huge pain in the neck. Patch 1 fixes this by
providing a simple and well-tested API, that takes care of integrating
the QOM->QObject->QAPI steps into a single function.
Patch 2 then eliminates the get_crash_info method. Patch 3 finally
cleans up qemu_system_guest_panicked by passing a CPUState* argument
instead of a GuestPanicInformation struct.
Paolo
v1->v2:
include missing changes to tests/qapi-schema/qapi-schema-test.out
tweaks to doc comments
wrap long lines
Paolo Bonzini (3):
qom-qobject: introduce object_property_{g,s}et_ptr
cpu: implement get_crash_info through QOM properties
vl: pass CPUState to qemu_system_guest_panicked
include/qom/cpu.h | 1 -
include/qom/qom-qobject.h | 68 +++++++++++-
include/sysemu/sysemu.h | 2 +-
kvm-all.c | 2 +-
qom/cpu.c | 11 +-
qom/qom-qobject.c | 52 ++++++++++
target/i386/cpu.c | 2 +-
tests/Makefile.include | 2 +-
tests/check-qom-proplist.c | 177 +++++++++++++++++++++++++++++++-
tests/qapi-schema/qapi-schema-test.json | 8 ++
tests/qapi-schema/qapi-schema-test.out | 6 ++
vl.c | 13 ++-
12 files changed, 326 insertions(+), 18 deletions(-)
--
2.9.3
^ permalink raw reply [flat|nested] 13+ messages in thread
* [Qemu-devel] [PATCH 1/3] qom-qobject: introduce object_property_{g, s}et_ptr
2017-02-22 18:04 [Qemu-devel] [PATCH v2 0/3] simplify struct QOM properties and use the result for GUEST_PANICKED Paolo Bonzini
@ 2017-02-22 18:04 ` Paolo Bonzini
2017-02-22 23:34 ` Eric Blake
` (2 more replies)
2017-02-22 18:04 ` [Qemu-devel] [PATCH 2/3] cpu: implement get_crash_info through QOM properties Paolo Bonzini
` (3 subsequent siblings)
4 siblings, 3 replies; 13+ messages in thread
From: Paolo Bonzini @ 2017-02-22 18:04 UTC (permalink / raw)
To: qemu-devel; +Cc: eblake, marcandre.lureau
The functions simplify the handling of QOM properties whose type
is a QAPI struct. They go through a QObject just like the other
functions that access a QOM property through its C type.
Like QAPI_CLONE, the functions are wrapped by macros that take a
QAPI type name and use it to build the name of a visitor function.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
include/qom/qom-qobject.h | 68 +++++++++++-
qom/qom-qobject.c | 52 ++++++++++
tests/Makefile.include | 2 +-
tests/check-qom-proplist.c | 177 +++++++++++++++++++++++++++++++-
tests/qapi-schema/qapi-schema-test.json | 8 ++
tests/qapi-schema/qapi-schema-test.out | 6 ++
6 files changed, 309 insertions(+), 4 deletions(-)
diff --git a/include/qom/qom-qobject.h b/include/qom/qom-qobject.h
index 77cd717..fab9c4f 100644
--- a/include/qom/qom-qobject.h
+++ b/include/qom/qom-qobject.h
@@ -30,7 +30,7 @@ struct QObject *object_property_get_qobject(Object *obj, const char *name,
/**
* object_property_set_qobject:
* @obj: the object
- * @ret: The value that will be written to the property.
+ * @ret: The value that will be written to the property
* @name: the name of the property
* @errp: returns an error if this function fails
*
@@ -39,4 +39,70 @@ struct QObject *object_property_get_qobject(Object *obj, const char *name,
void object_property_set_qobject(Object *obj, struct QObject *qobj,
const char *name, struct Error **errp);
+/**
+ * object_property_get_ptr:
+ * @obj: the object
+ * @name: the name of the property
+ * @visit_type: the visitor function for the returned object
+ * @errp: returns an error if this function fails
+ *
+ * Return: the value of an object's property, unmarshaled into a C object
+ * through a QAPI type visitor, or NULL if an error occurs.
+ */
+void *object_property_get_ptr(Object *obj, const char *name,
+ void (*visit_type)(Visitor *, const char *,
+ void **, Error **),
+ Error **errp);
+
+/**
+ * OBJECT_PROPERTY_GET_PTR:
+ * @obj: the object
+ * @name: the name of the property
+ * @type: the name of the C struct type that is returned
+ * @errp: returns an error if this function fails
+ *
+ * Return: the value of an object's property, unmarshaled into a C object
+ * through a QAPI type visitor, or NULL if an error occurs.
+ */
+#define OBJECT_PROPERTY_GET_PTR(obj, name, type, errp) \
+ ((type *) \
+ object_property_get_ptr(obj, name, \
+ (void (*)(Visitor *, const char *, void**, \
+ Error **))visit_type_ ## type, \
+ errp))
+
+/**
+ * object_property_set_ptr:
+ * @obj: the object
+ * @ptr: The C struct that will be written to the property
+ * @name: the name of the property
+ * @visit_type: the visitor function for @ptr's type
+ * @errp: returns an error if this function fails
+ *
+ * Sets an object's property to a C object's value, using a QAPI
+ * type visitor to marshal the C struct into the property value.
+ */
+void object_property_set_ptr(Object *obj, void *ptr, const char *name,
+ void (*visit_type)(Visitor *, const char *,
+ void **, Error **),
+ Error **errp);
+
+/**
+ * OBJECT_PROPERTY_SET_PTR:
+ * @obj: the object
+ * @ptr: The C struct that will be written to the property
+ * @name: the name of the property
+ * @type: the name of the C struct type pointed to by @ptr
+ * @errp: returns an error if this function fails
+ *
+ * Sets an object's property to a C object's value, using a QAPI
+ * type visitor to marshal the C struct into the property value.
+ */
+#define OBJECT_PROPERTY_SET_PTR(obj, ptr, name, type, errp) \
+ object_property_set_ptr(obj, ptr + type_check(type, typeof(*ptr)), \
+ name, \
+ (void (*)(Visitor *, const char *, void**, \
+ Error **))visit_type_ ## type, \
+ errp)
+
#endif
diff --git a/qom/qom-qobject.c b/qom/qom-qobject.c
index 447e4a0..d4675be 100644
--- a/qom/qom-qobject.c
+++ b/qom/qom-qobject.c
@@ -44,3 +44,55 @@ QObject *object_property_get_qobject(Object *obj, const char *name,
visit_free(v);
return ret;
}
+
+void object_property_set_ptr(Object *obj, void *ptr, const char *name,
+ void (*visit_type)(Visitor *, const char *,
+ void **, Error **),
+ Error **errp)
+{
+ Error *local_err = NULL;
+ QObject *ret = NULL;
+ Visitor *v;
+ v = qobject_output_visitor_new(&ret);
+ visit_type(v, name, &ptr, &local_err);
+ if (local_err) {
+ error_propagate(errp, local_err);
+ visit_free(v);
+ return;
+ }
+ visit_complete(v, &ret);
+ visit_free(v);
+
+ /* Do not use object_property_set_qobject until we switch it
+ * to use qobject_input_visitor_new in strict mode. See the
+ * /qom/proplist/get-set-ptr/contravariant unit test.
+ */
+ v = qobject_input_visitor_new(ret, true);
+ object_property_set(obj, v, name, errp);
+ visit_free(v);
+ qobject_decref(ret);
+}
+
+void *object_property_get_ptr(Object *obj, const char *name,
+ void (*visit_type)(Visitor *, const char *,
+ void **, Error **),
+ Error **errp)
+{
+ QObject *ret;
+ Visitor *v;
+ void *ptr = NULL;
+
+ ret = object_property_get_qobject(obj, name, errp);
+ if (!ret) {
+ return NULL;
+ }
+
+ /* Do not enable strict mode to allow passing covariant
+ * data types.
+ */
+ v = qobject_input_visitor_new(ret, false);
+ visit_type(v, name, &ptr, errp);
+ qobject_decref(ret);
+ visit_free(v);
+ return ptr;
+}
diff --git a/tests/Makefile.include b/tests/Makefile.include
index e60bb6c..1a1b6e2 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -519,7 +519,7 @@ tests/check-qfloat$(EXESUF): tests/check-qfloat.o $(test-util-obj-y)
tests/check-qnull$(EXESUF): tests/check-qnull.o $(test-util-obj-y)
tests/check-qjson$(EXESUF): tests/check-qjson.o $(test-util-obj-y)
tests/check-qom-interface$(EXESUF): tests/check-qom-interface.o $(test-qom-obj-y)
-tests/check-qom-proplist$(EXESUF): tests/check-qom-proplist.o $(test-qom-obj-y)
+tests/check-qom-proplist$(EXESUF): tests/check-qom-proplist.o $(test-qom-obj-y) $(test-qapi-obj-y)
tests/test-char$(EXESUF): tests/test-char.o $(test-util-obj-y) $(qtest-obj-y) $(test-io-obj-y) $(chardev-obj-y)
tests/test-coroutine$(EXESUF): tests/test-coroutine.o $(test-block-obj-y)
diff --git a/tests/check-qom-proplist.c b/tests/check-qom-proplist.c
index a16cefc..1bf0320 100644
--- a/tests/check-qom-proplist.c
+++ b/tests/check-qom-proplist.c
@@ -22,8 +22,11 @@
#include "qapi/error.h"
#include "qom/object.h"
+#include "qom/qom-qobject.h"
#include "qemu/module.h"
+#include "test-qapi-types.h"
+#include "test-qapi-visit.h"
#define TYPE_DUMMY "qemu-dummy"
@@ -56,6 +59,8 @@ struct DummyObject {
bool bv;
DummyAnimal av;
char *sv;
+
+ UserDefOne *qv;
};
struct DummyObjectClass {
@@ -120,12 +125,42 @@ static char *dummy_get_sv(Object *obj,
static void dummy_init(Object *obj)
{
+ DummyObject *dobj = DUMMY_OBJECT(obj);
+
object_property_add_bool(obj, "bv",
dummy_get_bv,
dummy_set_bv,
NULL);
+ dobj->qv = g_new0(UserDefOne, 1);
+ dobj->qv->string = g_strdup("dummy string");
+}
+
+
+static void dummy_get_qv(Object *obj, Visitor *v, const char *name,
+ void *opaque, Error **errp)
+{
+ DummyObject *dobj = DUMMY_OBJECT(obj);
+
+ visit_type_UserDefOne(v, name, &dobj->qv, errp);
}
+static void dummy_set_qv(Object *obj, Visitor *v, const char *name,
+ void *opaque, Error **errp)
+{
+ DummyObject *dobj = DUMMY_OBJECT(obj);
+ UserDefOne *qv = NULL;
+ Error *local_err = NULL;
+
+ visit_type_UserDefOne(v, name, &qv, &local_err);
+ if (local_err) {
+ g_assert(qv == NULL);
+ error_propagate(errp, local_err);
+ return;
+ }
+
+ qapi_free_UserDefOne(dobj->qv);
+ dobj->qv = qv;
+}
static void dummy_class_init(ObjectClass *cls, void *data)
{
@@ -143,6 +178,13 @@ static void dummy_class_init(ObjectClass *cls, void *data)
dummy_get_av,
dummy_set_av,
NULL);
+ object_class_property_add(cls, "qv",
+ "UserDefOne",
+ dummy_get_qv,
+ dummy_set_qv,
+ NULL,
+ NULL,
+ NULL);
}
@@ -151,9 +193,9 @@ static void dummy_finalize(Object *obj)
DummyObject *dobj = DUMMY_OBJECT(obj);
g_free(dobj->sv);
+ qapi_free_UserDefOne(dobj->qv);
}
-
static const TypeInfo dummy_info = {
.name = TYPE_DUMMY,
.parent = TYPE_OBJECT,
@@ -473,7 +515,8 @@ static void test_dummy_iterator(void)
ObjectProperty *prop;
ObjectPropertyIterator iter;
- bool seenbv = false, seensv = false, seenav = false, seentype;
+ bool seenbv = false, seensv = false, seenav = false;
+ bool seenqv = false, seentype = false;
object_property_iter_init(&iter, OBJECT(dobj));
while ((prop = object_property_iter_next(&iter))) {
@@ -483,6 +526,8 @@ static void test_dummy_iterator(void)
seensv = true;
} else if (g_str_equal(prop->name, "av")) {
seenav = true;
+ } else if (g_str_equal(prop->name, "qv")) {
+ seenqv = true;
} else if (g_str_equal(prop->name, "type")) {
/* This prop comes from the base Object class */
seentype = true;
@@ -494,6 +539,7 @@ static void test_dummy_iterator(void)
g_assert(seenbv);
g_assert(seenav);
g_assert(seensv);
+ g_assert(seenqv);
g_assert(seentype);
object_unparent(OBJECT(dobj));
@@ -513,6 +559,129 @@ static void test_dummy_delchild(void)
object_unparent(OBJECT(dev));
}
+static void test_dummy_get_set_ptr_struct(void)
+{
+ DummyObject *dobj = DUMMY_OBJECT(object_new(TYPE_DUMMY));
+ Error *local_err = NULL;
+ const char *s = "my other dummy string";
+ UserDefOne *ret;
+ UserDefOne val;
+
+ ret = OBJECT_PROPERTY_GET_PTR(OBJECT(dobj), "qv",
+ UserDefOne, &local_err);
+ g_assert(!local_err);
+
+ g_assert_cmpint(ret->integer, ==, 0);
+ g_assert_cmpstr(ret->string, ==, "dummy string");
+ g_assert(!ret->has_enum1);
+ qapi_free_UserDefOne(ret);
+
+ val.integer = 42;
+ val.string = g_strdup(s);
+ val.has_enum1 = true;
+ val.enum1 = ENUM_ONE_VALUE1;
+ OBJECT_PROPERTY_SET_PTR(OBJECT(dobj), &val, "qv",
+ UserDefOne, &local_err);
+ g_assert(!local_err);
+
+ ret = OBJECT_PROPERTY_GET_PTR(OBJECT(dobj), "qv",
+ UserDefOne, &local_err);
+ g_assert(!local_err);
+
+ g_assert_cmpint(ret->integer, ==, val.integer);
+ g_assert_cmpstr(ret->string, ==, val.string);
+ g_assert(ret->has_enum1);
+ g_assert_cmpint(ret->enum1, ==, val.enum1);
+ g_free(val.string);
+ qapi_free_UserDefOne(ret);
+}
+
+static void test_dummy_get_set_ptr_contravariant(void)
+{
+ DummyObject *dobj = DUMMY_OBJECT(object_new(TYPE_DUMMY));
+ Error *local_err = NULL;
+ UserDefOneMore *ret;
+ UserDefOneMore val;
+
+ /* You cannot retrieve a contravariant (subclass) type... */
+ ret = OBJECT_PROPERTY_GET_PTR(OBJECT(dobj), "qv",
+ UserDefOneMore, &local_err);
+ error_free_or_abort(&local_err);
+ g_assert(!ret);
+
+ /* And you cannot set one either. */
+ val.integer = 42;
+ val.string = g_strdup("unused");
+ val.has_enum1 = false;
+ val.boolean = false;
+
+ OBJECT_PROPERTY_SET_PTR(OBJECT(dobj), &val, "qv",
+ UserDefOneMore, &local_err);
+ g_assert(local_err);
+}
+
+static void test_dummy_get_set_ptr_covariant(void)
+{
+ DummyObject *dobj = DUMMY_OBJECT(object_new(TYPE_DUMMY));
+ Error *local_err = NULL;
+ UserDefZero *ret;
+ UserDefZero val;
+
+ /* You can retrieve a covariant (superclass) type... */
+ ret = OBJECT_PROPERTY_GET_PTR(OBJECT(dobj), "qv",
+ UserDefZero, &local_err);
+ g_assert(!local_err);
+
+ g_assert_cmpint(ret->integer, ==, 0);
+ qapi_free_UserDefZero(ret);
+
+ /* But you cannot set one. */
+ val.integer = 42;
+ OBJECT_PROPERTY_SET_PTR(OBJECT(dobj), &val, "qv",
+ UserDefZero, &local_err);
+ error_free_or_abort(&local_err);
+
+ /* Test that the property has not been modified at all */
+ ret = OBJECT_PROPERTY_GET_PTR(OBJECT(dobj), "qv",
+ UserDefZero, &local_err);
+ g_assert(!local_err);
+
+ g_assert_cmpint(ret->integer, ==, 0);
+ qapi_free_UserDefZero(ret);
+}
+
+static void test_dummy_get_set_ptr_error(void)
+{
+ DummyObject *dobj = DUMMY_OBJECT(object_new(TYPE_DUMMY));
+ Error *local_err = NULL;
+ const char *s = "my other dummy string";
+ UserDefOne *ret;
+ UserDefOne val;
+
+ ret = OBJECT_PROPERTY_GET_PTR(OBJECT(dobj), "blah",
+ UserDefOne, &local_err);
+ error_free_or_abort(&local_err);
+ g_assert(!ret);
+
+ val.integer = 42;
+ val.string = g_strdup(s);
+ val.has_enum1 = true;
+ val.enum1 = 100;
+ OBJECT_PROPERTY_SET_PTR(OBJECT(dobj), &val, "qv",
+ UserDefOne, &local_err);
+ error_free_or_abort(&local_err);
+
+ ret = OBJECT_PROPERTY_GET_PTR(OBJECT(dobj), "qv",
+ UserDefOne, &local_err);
+ g_assert(!local_err);
+
+ /* Test that the property has not been modified at all */
+ g_assert_cmpint(ret->integer, ==, 0);
+ g_assert_cmpstr(ret->string, ==, "dummy string");
+ g_assert(!ret->has_enum1);
+ qapi_free_UserDefOne(ret);
+}
+
int main(int argc, char **argv)
{
g_test_init(&argc, &argv, NULL);
@@ -530,5 +699,9 @@ int main(int argc, char **argv)
g_test_add_func("/qom/proplist/iterator", test_dummy_iterator);
g_test_add_func("/qom/proplist/delchild", test_dummy_delchild);
+ g_test_add_func("/qom/proplist/get-set-ptr/struct", test_dummy_get_set_ptr_struct);
+ g_test_add_func("/qom/proplist/get-set-ptr/error", test_dummy_get_set_ptr_error);
+ g_test_add_func("/qom/proplist/get-set-ptr/covariant", test_dummy_get_set_ptr_covariant);
+ g_test_add_func("/qom/proplist/get-set-ptr/contravariant", test_dummy_get_set_ptr_contravariant);
return g_test_run();
}
diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json
index f4d8cc4..4e3f6ff 100644
--- a/tests/qapi-schema/qapi-schema-test.json
+++ b/tests/qapi-schema/qapi-schema-test.json
@@ -91,6 +91,14 @@
'*enum1': 'EnumOne' } } # intentional forward reference
##
+# @UserDefOneMore:
+# for testing nested structs
+##
+{ 'struct': 'UserDefOneMore',
+ 'base': 'UserDefOne',
+ 'data': { 'boolean': 'bool' } }
+
+##
# @EnumOne:
##
{ 'enum': 'EnumOne',
diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out
index bc8d496..d3a2990 100644
--- a/tests/qapi-schema/qapi-schema-test.out
+++ b/tests/qapi-schema/qapi-schema-test.out
@@ -107,6 +107,9 @@ object UserDefOne
base UserDefZero
member string: str optional=False
member enum1: EnumOne optional=True
+object UserDefOneMore
+ base UserDefOne
+ member boolean: bool optional=False
object UserDefOptions
member i64: intList optional=True
member u64: uint64List optional=True
@@ -283,6 +286,9 @@ for testing override of default naming heuristic
doc symbol=UserDefOne expr=('struct', 'UserDefOne')
body=
for testing nested structs
+doc symbol=UserDefOneMore expr=('struct', 'UserDefOneMore')
+ body=
+for testing nested structs
doc symbol=EnumOne expr=('enum', 'EnumOne')
doc symbol=UserDefZero expr=('struct', 'UserDefZero')
doc symbol=UserDefTwoDictDict expr=('struct', 'UserDefTwoDictDict')
--
2.9.3
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [Qemu-devel] [PATCH 2/3] cpu: implement get_crash_info through QOM properties
2017-02-22 18:04 [Qemu-devel] [PATCH v2 0/3] simplify struct QOM properties and use the result for GUEST_PANICKED Paolo Bonzini
2017-02-22 18:04 ` [Qemu-devel] [PATCH 1/3] qom-qobject: introduce object_property_{g, s}et_ptr Paolo Bonzini
@ 2017-02-22 18:04 ` Paolo Bonzini
2017-02-22 18:04 ` [Qemu-devel] [PATCH 3/3] vl: pass CPUState to qemu_system_guest_panicked Paolo Bonzini
` (2 subsequent siblings)
4 siblings, 0 replies; 13+ messages in thread
From: Paolo Bonzini @ 2017-02-22 18:04 UTC (permalink / raw)
To: qemu-devel; +Cc: eblake, marcandre.lureau
Provide a generic implementation for all CPU subclasses.
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
include/qom/cpu.h | 1 -
qom/cpu.c | 11 ++++-------
target/i386/cpu.c | 2 +-
3 files changed, 5 insertions(+), 9 deletions(-)
diff --git a/include/qom/cpu.h b/include/qom/cpu.h
index 1bc3ad2..04d3a2c 100644
--- a/include/qom/cpu.h
+++ b/include/qom/cpu.h
@@ -158,7 +158,6 @@ typedef struct CPUClass {
uint8_t *buf, int len, bool is_write);
void (*dump_state)(CPUState *cpu, FILE *f, fprintf_function cpu_fprintf,
int flags);
- GuestPanicInformation* (*get_crash_info)(CPUState *cpu);
void (*dump_statistics)(CPUState *cpu, FILE *f,
fprintf_function cpu_fprintf, int flags);
int64_t (*get_arch_id)(CPUState *cpu);
diff --git a/qom/cpu.c b/qom/cpu.c
index 7e005af..a9482ce 100644
--- a/qom/cpu.c
+++ b/qom/cpu.c
@@ -22,6 +22,8 @@
#include "qapi/error.h"
#include "qemu-common.h"
#include "qom/cpu.h"
+#include "qom/qom-qobject.h"
+#include "qapi-visit.h"
#include "sysemu/hw_accel.h"
#include "qemu/notify.h"
#include "qemu/log.h"
@@ -220,13 +222,8 @@ static bool cpu_common_exec_interrupt(CPUState *cpu, int int_req)
GuestPanicInformation *cpu_get_crash_info(CPUState *cpu)
{
- CPUClass *cc = CPU_GET_CLASS(cpu);
- GuestPanicInformation *res = NULL;
-
- if (cc->get_crash_info) {
- res = cc->get_crash_info(cpu);
- }
- return res;
+ return OBJECT_PROPERTY_GET_PTR(OBJECT(cpu), "crash-information",
+ GuestPanicInformation, NULL);
}
void cpu_dump_state(CPUState *cpu, FILE *f, fprintf_function cpu_fprintf,
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 63be816..3071769 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -3516,6 +3516,7 @@ static GuestPanicInformation *x86_cpu_get_crash_info(CPUState *cs)
return panic_info;
}
+
static void x86_cpu_get_crash_info_qom(Object *obj, Visitor *v,
const char *name, void *opaque,
Error **errp)
@@ -3731,7 +3732,6 @@ static void x86_cpu_common_class_init(ObjectClass *oc, void *data)
cc->do_interrupt = x86_cpu_do_interrupt;
cc->cpu_exec_interrupt = x86_cpu_exec_interrupt;
cc->dump_state = x86_cpu_dump_state;
- cc->get_crash_info = x86_cpu_get_crash_info;
cc->set_pc = x86_cpu_set_pc;
cc->synchronize_from_tb = x86_cpu_synchronize_from_tb;
cc->gdb_read_register = x86_cpu_gdb_read_register;
--
2.9.3
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [Qemu-devel] [PATCH 3/3] vl: pass CPUState to qemu_system_guest_panicked
2017-02-22 18:04 [Qemu-devel] [PATCH v2 0/3] simplify struct QOM properties and use the result for GUEST_PANICKED Paolo Bonzini
2017-02-22 18:04 ` [Qemu-devel] [PATCH 1/3] qom-qobject: introduce object_property_{g, s}et_ptr Paolo Bonzini
2017-02-22 18:04 ` [Qemu-devel] [PATCH 2/3] cpu: implement get_crash_info through QOM properties Paolo Bonzini
@ 2017-02-22 18:04 ` Paolo Bonzini
2017-02-22 18:13 ` [Qemu-devel] [PATCH v2 0/3] simplify struct QOM properties and use the result for GUEST_PANICKED no-reply
2017-02-23 8:34 ` Marc-André Lureau
4 siblings, 0 replies; 13+ messages in thread
From: Paolo Bonzini @ 2017-02-22 18:04 UTC (permalink / raw)
To: qemu-devel; +Cc: eblake, marcandre.lureau
qemu_system_guest_panicked was already using current_cpu implicitly,
so it makes sense for it to receive a CPUState. This lets the
function call cpu_get_crash_info and free the result.
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
include/sysemu/sysemu.h | 2 +-
kvm-all.c | 2 +-
vl.c | 13 ++++++++++---
3 files changed, 12 insertions(+), 5 deletions(-)
diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
index 576c7ce..a02f53a 100644
--- a/include/sysemu/sysemu.h
+++ b/include/sysemu/sysemu.h
@@ -66,7 +66,7 @@ int qemu_shutdown_requested_get(void);
int qemu_reset_requested_get(void);
void qemu_system_killed(int signal, pid_t pid);
void qemu_system_reset(bool report);
-void qemu_system_guest_panicked(GuestPanicInformation *info);
+void qemu_system_guest_panicked(CPUState *cpu);
size_t qemu_target_page_bits(void);
void qemu_add_exit_notifier(Notifier *notify);
diff --git a/kvm-all.c b/kvm-all.c
index 7ad20b7..edecef0 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -2071,7 +2071,7 @@ int kvm_cpu_exec(CPUState *cpu)
case KVM_SYSTEM_EVENT_CRASH:
kvm_cpu_synchronize_state(cpu);
qemu_mutex_lock_iothread();
- qemu_system_guest_panicked(cpu_get_crash_info(cpu));
+ qemu_system_guest_panicked(cpu);
qemu_mutex_unlock_iothread();
ret = 0;
break;
diff --git a/vl.c b/vl.c
index e307ae0..7f5159d 100644
--- a/vl.c
+++ b/vl.c
@@ -1680,13 +1680,20 @@ void qemu_system_reset(bool report)
cpu_synchronize_all_post_reset();
}
-void qemu_system_guest_panicked(GuestPanicInformation *info)
+void qemu_system_guest_panicked(CPUState *cpu)
{
+ GuestPanicInformation *info = NULL;
+
qemu_log_mask(LOG_GUEST_ERROR, "Guest crashed\n");
- if (current_cpu) {
- current_cpu->crash_occurred = true;
+ if (!cpu) {
+ cpu = current_cpu;
+ }
+ if (cpu) {
+ cpu->crash_occurred = true;
+ info = cpu_get_crash_info(cpu);
}
+
qapi_event_send_guest_panicked(GUEST_PANIC_ACTION_PAUSE,
!!info, info, &error_abort);
vm_stop(RUN_STATE_GUEST_PANICKED);
--
2.9.3
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH v2 0/3] simplify struct QOM properties and use the result for GUEST_PANICKED
2017-02-22 18:04 [Qemu-devel] [PATCH v2 0/3] simplify struct QOM properties and use the result for GUEST_PANICKED Paolo Bonzini
` (2 preceding siblings ...)
2017-02-22 18:04 ` [Qemu-devel] [PATCH 3/3] vl: pass CPUState to qemu_system_guest_panicked Paolo Bonzini
@ 2017-02-22 18:13 ` no-reply
2017-02-23 8:34 ` Marc-André Lureau
4 siblings, 0 replies; 13+ messages in thread
From: no-reply @ 2017-02-22 18:13 UTC (permalink / raw)
To: pbonzini; +Cc: famz, qemu-devel, marcandre.lureau
Hi,
This series seems to have some coding style problems. See output below for
more information:
Type: series
Subject: [Qemu-devel] [PATCH v2 0/3] simplify struct QOM properties and use the result for GUEST_PANICKED
Message-id: 20170222180423.26571-1-pbonzini@redhat.com
=== TEST SCRIPT BEGIN ===
#!/bin/bash
BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0
# Useful git options
git config --local diff.renamelimit 0
git config --local diff.renames True
commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
failed=1
echo
fi
n=$((n+1))
done
exit $failed
=== TEST SCRIPT END ===
Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
3c23639 vl: pass CPUState to qemu_system_guest_panicked
e22eb3a cpu: implement get_crash_info through QOM properties
a359a36 qom-qobject: introduce object_property_{g, s}et_ptr
=== OUTPUT BEGIN ===
Checking PATCH 1/3: qom-qobject: introduce object_property_{g, s}et_ptr...
WARNING: line over 80 characters
#427: FILE: tests/check-qom-proplist.c:702:
+ g_test_add_func("/qom/proplist/get-set-ptr/struct", test_dummy_get_set_ptr_struct);
WARNING: line over 80 characters
#428: FILE: tests/check-qom-proplist.c:703:
+ g_test_add_func("/qom/proplist/get-set-ptr/error", test_dummy_get_set_ptr_error);
ERROR: line over 90 characters
#429: FILE: tests/check-qom-proplist.c:704:
+ g_test_add_func("/qom/proplist/get-set-ptr/covariant", test_dummy_get_set_ptr_covariant);
ERROR: line over 90 characters
#430: FILE: tests/check-qom-proplist.c:705:
+ g_test_add_func("/qom/proplist/get-set-ptr/contravariant", test_dummy_get_set_ptr_contravariant);
total: 2 errors, 2 warnings, 419 lines checked
Your patch has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
Checking PATCH 2/3: cpu: implement get_crash_info through QOM properties...
Checking PATCH 3/3: vl: pass CPUState to qemu_system_guest_panicked...
=== OUTPUT END ===
Test command exited with code: 1
---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@freelists.org
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH 1/3] qom-qobject: introduce object_property_{g, s}et_ptr
2017-02-22 18:04 ` [Qemu-devel] [PATCH 1/3] qom-qobject: introduce object_property_{g, s}et_ptr Paolo Bonzini
@ 2017-02-22 23:34 ` Eric Blake
2017-02-23 8:33 ` Marc-André Lureau
2017-02-24 14:54 ` Markus Armbruster
2 siblings, 0 replies; 13+ messages in thread
From: Eric Blake @ 2017-02-22 23:34 UTC (permalink / raw)
To: Paolo Bonzini, qemu-devel; +Cc: marcandre.lureau
[-- Attachment #1: Type: text/plain, Size: 1032 bytes --]
On 02/22/2017 12:04 PM, Paolo Bonzini wrote:
> The functions simplify the handling of QOM properties whose type
> is a QAPI struct. They go through a QObject just like the other
> functions that access a QOM property through its C type.
>
> Like QAPI_CLONE, the functions are wrapped by macros that take a
> QAPI type name and use it to build the name of a visitor function.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> include/qom/qom-qobject.h | 68 +++++++++++-
> qom/qom-qobject.c | 52 ++++++++++
> tests/Makefile.include | 2 +-
> tests/check-qom-proplist.c | 177 +++++++++++++++++++++++++++++++-
> tests/qapi-schema/qapi-schema-test.json | 8 ++
> tests/qapi-schema/qapi-schema-test.out | 6 ++
> 6 files changed, 309 insertions(+), 4 deletions(-)
>
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH 1/3] qom-qobject: introduce object_property_{g, s}et_ptr
2017-02-22 18:04 ` [Qemu-devel] [PATCH 1/3] qom-qobject: introduce object_property_{g, s}et_ptr Paolo Bonzini
2017-02-22 23:34 ` Eric Blake
@ 2017-02-23 8:33 ` Marc-André Lureau
2017-02-24 14:54 ` Markus Armbruster
2 siblings, 0 replies; 13+ messages in thread
From: Marc-André Lureau @ 2017-02-23 8:33 UTC (permalink / raw)
To: Paolo Bonzini, qemu-devel
Hi
On Wed, Feb 22, 2017 at 10:07 PM Paolo Bonzini <pbonzini@redhat.com> wrote:
> The functions simplify the handling of QOM properties whose type
> is a QAPI struct. They go through a QObject just like the other
> functions that access a QOM property through its C type.
>
> Like QAPI_CLONE, the functions are wrapped by macros that take a
> QAPI type name and use it to build the name of a visitor function.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Please fix the tests leaks:
diff --git a/tests/check-qom-proplist.c b/tests/check-qom-proplist.c
index 1bf0320854..379d3abe6b 100644
--- a/tests/check-qom-proplist.c
+++ b/tests/check-qom-proplist.c
@@ -594,6 +594,7 @@ static void test_dummy_get_set_ptr_struct(void)
g_assert_cmpint(ret->enum1, ==, val.enum1);
g_free(val.string);
qapi_free_UserDefOne(ret);
+ object_unref(OBJECT(dobj));
}
static void test_dummy_get_set_ptr_contravariant(void)
@@ -617,7 +618,9 @@ static void test_dummy_get_set_ptr_contravariant(void)
OBJECT_PROPERTY_SET_PTR(OBJECT(dobj), &val, "qv",
UserDefOneMore, &local_err);
- g_assert(local_err);
+ error_free_or_abort(&local_err);
+ object_unref(OBJECT(dobj));
+ g_free(val.string);
}
static void test_dummy_get_set_ptr_covariant(void)
@@ -648,6 +651,7 @@ static void test_dummy_get_set_ptr_covariant(void)
g_assert_cmpint(ret->integer, ==, 0);
qapi_free_UserDefZero(ret);
+ object_unref(OBJECT(dobj));
}
static void test_dummy_get_set_ptr_error(void)
@@ -680,6 +684,8 @@ static void test_dummy_get_set_ptr_error(void)
g_assert_cmpstr(ret->string, ==, "dummy string");
g_assert(!ret->has_enum1);
qapi_free_UserDefOne(ret);
+ object_unref(OBJECT(dobj));
+ g_free(val.string);
}
int main(int argc, char **argv)
> ---
> include/qom/qom-qobject.h | 68 +++++++++++-
> qom/qom-qobject.c | 52 ++++++++++
> tests/Makefile.include | 2 +-
> tests/check-qom-proplist.c | 177
> +++++++++++++++++++++++++++++++-
> tests/qapi-schema/qapi-schema-test.json | 8 ++
> tests/qapi-schema/qapi-schema-test.out | 6 ++
> 6 files changed, 309 insertions(+), 4 deletions(-)
>
> diff --git a/include/qom/qom-qobject.h b/include/qom/qom-qobject.h
> index 77cd717..fab9c4f 100644
> --- a/include/qom/qom-qobject.h
> +++ b/include/qom/qom-qobject.h
> @@ -30,7 +30,7 @@ struct QObject *object_property_get_qobject(Object *obj,
> const char *name,
> /**
> * object_property_set_qobject:
> * @obj: the object
> - * @ret: The value that will be written to the property.
> + * @ret: The value that will be written to the property
> * @name: the name of the property
> * @errp: returns an error if this function fails
> *
> @@ -39,4 +39,70 @@ struct QObject *object_property_get_qobject(Object
> *obj, const char *name,
> void object_property_set_qobject(Object *obj, struct QObject *qobj,
> const char *name, struct Error **errp);
>
> +/**
> + * object_property_get_ptr:
> + * @obj: the object
> + * @name: the name of the property
> + * @visit_type: the visitor function for the returned object
> + * @errp: returns an error if this function fails
> + *
> + * Return: the value of an object's property, unmarshaled into a C object
> + * through a QAPI type visitor, or NULL if an error occurs.
> + */
> +void *object_property_get_ptr(Object *obj, const char *name,
> + void (*visit_type)(Visitor *, const char *,
> + void **, Error **),
> + Error **errp);
> +
> +/**
> + * OBJECT_PROPERTY_GET_PTR:
> + * @obj: the object
> + * @name: the name of the property
> + * @type: the name of the C struct type that is returned
> + * @errp: returns an error if this function fails
> + *
> + * Return: the value of an object's property, unmarshaled into a C object
> + * through a QAPI type visitor, or NULL if an error occurs.
> + */
> +#define OBJECT_PROPERTY_GET_PTR(obj, name, type, errp)
> \
> + ((type *)
> \
> + object_property_get_ptr(obj, name,
> \
> + (void (*)(Visitor *, const char *, void**,
> \
> + Error **))visit_type_ ## type,
> \
> + errp))
> +
> +/**
> + * object_property_set_ptr:
> + * @obj: the object
> + * @ptr: The C struct that will be written to the property
> + * @name: the name of the property
> + * @visit_type: the visitor function for @ptr's type
> + * @errp: returns an error if this function fails
> + *
> + * Sets an object's property to a C object's value, using a QAPI
> + * type visitor to marshal the C struct into the property value.
> + */
> +void object_property_set_ptr(Object *obj, void *ptr, const char *name,
> + void (*visit_type)(Visitor *, const char *,
> + void **, Error **),
> + Error **errp);
> +
> +/**
> + * OBJECT_PROPERTY_SET_PTR:
> + * @obj: the object
> + * @ptr: The C struct that will be written to the property
> + * @name: the name of the property
> + * @type: the name of the C struct type pointed to by @ptr
> + * @errp: returns an error if this function fails
> + *
> + * Sets an object's property to a C object's value, using a QAPI
> + * type visitor to marshal the C struct into the property value.
> + */
> +#define OBJECT_PROPERTY_SET_PTR(obj, ptr, name, type, errp)
> \
> + object_property_set_ptr(obj, ptr + type_check(type, typeof(*ptr)),
> \
> + name,
> \
> + (void (*)(Visitor *, const char *, void**,
> \
> + Error **))visit_type_ ## type,
> \
> + errp)
> +
> #endif
> diff --git a/qom/qom-qobject.c b/qom/qom-qobject.c
> index 447e4a0..d4675be 100644
> --- a/qom/qom-qobject.c
> +++ b/qom/qom-qobject.c
> @@ -44,3 +44,55 @@ QObject *object_property_get_qobject(Object *obj, const
> char *name,
> visit_free(v);
> return ret;
> }
> +
> +void object_property_set_ptr(Object *obj, void *ptr, const char *name,
> + void (*visit_type)(Visitor *, const char *,
> + void **, Error **),
> + Error **errp)
> +{
> + Error *local_err = NULL;
> + QObject *ret = NULL;
> + Visitor *v;
> + v = qobject_output_visitor_new(&ret);
> + visit_type(v, name, &ptr, &local_err);
> + if (local_err) {
> + error_propagate(errp, local_err);
> + visit_free(v);
> + return;
> + }
> + visit_complete(v, &ret);
> + visit_free(v);
> +
> + /* Do not use object_property_set_qobject until we switch it
> + * to use qobject_input_visitor_new in strict mode. See the
> + * /qom/proplist/get-set-ptr/contravariant unit test.
> + */
> + v = qobject_input_visitor_new(ret, true);
> + object_property_set(obj, v, name, errp);
> + visit_free(v);
> + qobject_decref(ret);
> +}
> +
> +void *object_property_get_ptr(Object *obj, const char *name,
> + void (*visit_type)(Visitor *, const char *,
> + void **, Error **),
> + Error **errp)
> +{
> + QObject *ret;
> + Visitor *v;
> + void *ptr = NULL;
> +
> + ret = object_property_get_qobject(obj, name, errp);
> + if (!ret) {
> + return NULL;
> + }
> +
> + /* Do not enable strict mode to allow passing covariant
> + * data types.
> + */
> + v = qobject_input_visitor_new(ret, false);
> + visit_type(v, name, &ptr, errp);
> + qobject_decref(ret);
> + visit_free(v);
> + return ptr;
> +}
> diff --git a/tests/Makefile.include b/tests/Makefile.include
> index e60bb6c..1a1b6e2 100644
> --- a/tests/Makefile.include
> +++ b/tests/Makefile.include
> @@ -519,7 +519,7 @@ tests/check-qfloat$(EXESUF): tests/check-qfloat.o
> $(test-util-obj-y)
> tests/check-qnull$(EXESUF): tests/check-qnull.o $(test-util-obj-y)
> tests/check-qjson$(EXESUF): tests/check-qjson.o $(test-util-obj-y)
> tests/check-qom-interface$(EXESUF): tests/check-qom-interface.o
> $(test-qom-obj-y)
> -tests/check-qom-proplist$(EXESUF): tests/check-qom-proplist.o
> $(test-qom-obj-y)
> +tests/check-qom-proplist$(EXESUF): tests/check-qom-proplist.o
> $(test-qom-obj-y) $(test-qapi-obj-y)
>
> tests/test-char$(EXESUF): tests/test-char.o $(test-util-obj-y)
> $(qtest-obj-y) $(test-io-obj-y) $(chardev-obj-y)
> tests/test-coroutine$(EXESUF): tests/test-coroutine.o $(test-block-obj-y)
> diff --git a/tests/check-qom-proplist.c b/tests/check-qom-proplist.c
> index a16cefc..1bf0320 100644
> --- a/tests/check-qom-proplist.c
> +++ b/tests/check-qom-proplist.c
> @@ -22,8 +22,11 @@
>
> #include "qapi/error.h"
> #include "qom/object.h"
> +#include "qom/qom-qobject.h"
> #include "qemu/module.h"
>
> +#include "test-qapi-types.h"
> +#include "test-qapi-visit.h"
>
> #define TYPE_DUMMY "qemu-dummy"
>
> @@ -56,6 +59,8 @@ struct DummyObject {
> bool bv;
> DummyAnimal av;
> char *sv;
> +
> + UserDefOne *qv;
> };
>
> struct DummyObjectClass {
> @@ -120,12 +125,42 @@ static char *dummy_get_sv(Object *obj,
>
> static void dummy_init(Object *obj)
> {
> + DummyObject *dobj = DUMMY_OBJECT(obj);
> +
> object_property_add_bool(obj, "bv",
> dummy_get_bv,
> dummy_set_bv,
> NULL);
> + dobj->qv = g_new0(UserDefOne, 1);
> + dobj->qv->string = g_strdup("dummy string");
> +}
> +
> +
> +static void dummy_get_qv(Object *obj, Visitor *v, const char *name,
> + void *opaque, Error **errp)
> +{
> + DummyObject *dobj = DUMMY_OBJECT(obj);
> +
> + visit_type_UserDefOne(v, name, &dobj->qv, errp);
> }
>
> +static void dummy_set_qv(Object *obj, Visitor *v, const char *name,
> + void *opaque, Error **errp)
> +{
> + DummyObject *dobj = DUMMY_OBJECT(obj);
> + UserDefOne *qv = NULL;
> + Error *local_err = NULL;
> +
> + visit_type_UserDefOne(v, name, &qv, &local_err);
> + if (local_err) {
> + g_assert(qv == NULL);
> + error_propagate(errp, local_err);
> + return;
> + }
> +
> + qapi_free_UserDefOne(dobj->qv);
> + dobj->qv = qv;
> +}
>
> static void dummy_class_init(ObjectClass *cls, void *data)
> {
> @@ -143,6 +178,13 @@ static void dummy_class_init(ObjectClass *cls, void
> *data)
> dummy_get_av,
> dummy_set_av,
> NULL);
> + object_class_property_add(cls, "qv",
> + "UserDefOne",
> + dummy_get_qv,
> + dummy_set_qv,
> + NULL,
> + NULL,
> + NULL);
> }
>
>
> @@ -151,9 +193,9 @@ static void dummy_finalize(Object *obj)
> DummyObject *dobj = DUMMY_OBJECT(obj);
>
> g_free(dobj->sv);
> + qapi_free_UserDefOne(dobj->qv);
> }
>
> -
> static const TypeInfo dummy_info = {
> .name = TYPE_DUMMY,
> .parent = TYPE_OBJECT,
> @@ -473,7 +515,8 @@ static void test_dummy_iterator(void)
>
> ObjectProperty *prop;
> ObjectPropertyIterator iter;
> - bool seenbv = false, seensv = false, seenav = false, seentype;
> + bool seenbv = false, seensv = false, seenav = false;
> + bool seenqv = false, seentype = false;
>
> object_property_iter_init(&iter, OBJECT(dobj));
> while ((prop = object_property_iter_next(&iter))) {
> @@ -483,6 +526,8 @@ static void test_dummy_iterator(void)
> seensv = true;
> } else if (g_str_equal(prop->name, "av")) {
> seenav = true;
> + } else if (g_str_equal(prop->name, "qv")) {
> + seenqv = true;
> } else if (g_str_equal(prop->name, "type")) {
> /* This prop comes from the base Object class */
> seentype = true;
> @@ -494,6 +539,7 @@ static void test_dummy_iterator(void)
> g_assert(seenbv);
> g_assert(seenav);
> g_assert(seensv);
> + g_assert(seenqv);
> g_assert(seentype);
>
> object_unparent(OBJECT(dobj));
> @@ -513,6 +559,129 @@ static void test_dummy_delchild(void)
> object_unparent(OBJECT(dev));
> }
>
> +static void test_dummy_get_set_ptr_struct(void)
> +{
> + DummyObject *dobj = DUMMY_OBJECT(object_new(TYPE_DUMMY));
> + Error *local_err = NULL;
> + const char *s = "my other dummy string";
> + UserDefOne *ret;
> + UserDefOne val;
> +
> + ret = OBJECT_PROPERTY_GET_PTR(OBJECT(dobj), "qv",
> + UserDefOne, &local_err);
> + g_assert(!local_err);
> +
> + g_assert_cmpint(ret->integer, ==, 0);
> + g_assert_cmpstr(ret->string, ==, "dummy string");
> + g_assert(!ret->has_enum1);
> + qapi_free_UserDefOne(ret);
> +
> + val.integer = 42;
> + val.string = g_strdup(s);
> + val.has_enum1 = true;
> + val.enum1 = ENUM_ONE_VALUE1;
> + OBJECT_PROPERTY_SET_PTR(OBJECT(dobj), &val, "qv",
> + UserDefOne, &local_err);
> + g_assert(!local_err);
> +
> + ret = OBJECT_PROPERTY_GET_PTR(OBJECT(dobj), "qv",
> + UserDefOne, &local_err);
> + g_assert(!local_err);
> +
> + g_assert_cmpint(ret->integer, ==, val.integer);
> + g_assert_cmpstr(ret->string, ==, val.string);
> + g_assert(ret->has_enum1);
> + g_assert_cmpint(ret->enum1, ==, val.enum1);
> + g_free(val.string);
> + qapi_free_UserDefOne(ret);
> +}
> +
> +static void test_dummy_get_set_ptr_contravariant(void)
> +{
> + DummyObject *dobj = DUMMY_OBJECT(object_new(TYPE_DUMMY));
> + Error *local_err = NULL;
> + UserDefOneMore *ret;
> + UserDefOneMore val;
> +
> + /* You cannot retrieve a contravariant (subclass) type... */
> + ret = OBJECT_PROPERTY_GET_PTR(OBJECT(dobj), "qv",
> + UserDefOneMore, &local_err);
> + error_free_or_abort(&local_err);
> + g_assert(!ret);
> +
> + /* And you cannot set one either. */
> + val.integer = 42;
> + val.string = g_strdup("unused");
> + val.has_enum1 = false;
> + val.boolean = false;
> +
> + OBJECT_PROPERTY_SET_PTR(OBJECT(dobj), &val, "qv",
> + UserDefOneMore, &local_err);
> + g_assert(local_err);
> +}
> +
> +static void test_dummy_get_set_ptr_covariant(void)
> +{
> + DummyObject *dobj = DUMMY_OBJECT(object_new(TYPE_DUMMY));
> + Error *local_err = NULL;
> + UserDefZero *ret;
> + UserDefZero val;
> +
> + /* You can retrieve a covariant (superclass) type... */
> + ret = OBJECT_PROPERTY_GET_PTR(OBJECT(dobj), "qv",
> + UserDefZero, &local_err);
> + g_assert(!local_err);
> +
> + g_assert_cmpint(ret->integer, ==, 0);
> + qapi_free_UserDefZero(ret);
> +
> + /* But you cannot set one. */
> + val.integer = 42;
> + OBJECT_PROPERTY_SET_PTR(OBJECT(dobj), &val, "qv",
> + UserDefZero, &local_err);
> + error_free_or_abort(&local_err);
> +
> + /* Test that the property has not been modified at all */
> + ret = OBJECT_PROPERTY_GET_PTR(OBJECT(dobj), "qv",
> + UserDefZero, &local_err);
> + g_assert(!local_err);
> +
> + g_assert_cmpint(ret->integer, ==, 0);
> + qapi_free_UserDefZero(ret);
> +}
> +
> +static void test_dummy_get_set_ptr_error(void)
> +{
> + DummyObject *dobj = DUMMY_OBJECT(object_new(TYPE_DUMMY));
> + Error *local_err = NULL;
> + const char *s = "my other dummy string";
> + UserDefOne *ret;
> + UserDefOne val;
> +
> + ret = OBJECT_PROPERTY_GET_PTR(OBJECT(dobj), "blah",
> + UserDefOne, &local_err);
> + error_free_or_abort(&local_err);
> + g_assert(!ret);
> +
> + val.integer = 42;
> + val.string = g_strdup(s);
> + val.has_enum1 = true;
> + val.enum1 = 100;
> + OBJECT_PROPERTY_SET_PTR(OBJECT(dobj), &val, "qv",
> + UserDefOne, &local_err);
> + error_free_or_abort(&local_err);
> +
> + ret = OBJECT_PROPERTY_GET_PTR(OBJECT(dobj), "qv",
> + UserDefOne, &local_err);
> + g_assert(!local_err);
> +
> + /* Test that the property has not been modified at all */
> + g_assert_cmpint(ret->integer, ==, 0);
> + g_assert_cmpstr(ret->string, ==, "dummy string");
> + g_assert(!ret->has_enum1);
> + qapi_free_UserDefOne(ret);
> +}
> +
> int main(int argc, char **argv)
> {
> g_test_init(&argc, &argv, NULL);
> @@ -530,5 +699,9 @@ int main(int argc, char **argv)
> g_test_add_func("/qom/proplist/iterator", test_dummy_iterator);
> g_test_add_func("/qom/proplist/delchild", test_dummy_delchild);
>
> + g_test_add_func("/qom/proplist/get-set-ptr/struct",
> test_dummy_get_set_ptr_struct);
> + g_test_add_func("/qom/proplist/get-set-ptr/error",
> test_dummy_get_set_ptr_error);
> + g_test_add_func("/qom/proplist/get-set-ptr/covariant",
> test_dummy_get_set_ptr_covariant);
> + g_test_add_func("/qom/proplist/get-set-ptr/contravariant",
> test_dummy_get_set_ptr_contravariant);
> return g_test_run();
> }
> diff --git a/tests/qapi-schema/qapi-schema-test.json
> b/tests/qapi-schema/qapi-schema-test.json
> index f4d8cc4..4e3f6ff 100644
> --- a/tests/qapi-schema/qapi-schema-test.json
> +++ b/tests/qapi-schema/qapi-schema-test.json
> @@ -91,6 +91,14 @@
> '*enum1': 'EnumOne' } } # intentional forward reference
>
> ##
> +# @UserDefOneMore:
> +# for testing nested structs
> +##
> +{ 'struct': 'UserDefOneMore',
> + 'base': 'UserDefOne',
> + 'data': { 'boolean': 'bool' } }
> +
> +##
> # @EnumOne:
> ##
> { 'enum': 'EnumOne',
> diff --git a/tests/qapi-schema/qapi-schema-test.out
> b/tests/qapi-schema/qapi-schema-test.out
> index bc8d496..d3a2990 100644
> --- a/tests/qapi-schema/qapi-schema-test.out
> +++ b/tests/qapi-schema/qapi-schema-test.out
> @@ -107,6 +107,9 @@ object UserDefOne
> base UserDefZero
> member string: str optional=False
> member enum1: EnumOne optional=True
> +object UserDefOneMore
> + base UserDefOne
> + member boolean: bool optional=False
> object UserDefOptions
> member i64: intList optional=True
> member u64: uint64List optional=True
> @@ -283,6 +286,9 @@ for testing override of default naming heuristic
> doc symbol=UserDefOne expr=('struct', 'UserDefOne')
> body=
> for testing nested structs
> +doc symbol=UserDefOneMore expr=('struct', 'UserDefOneMore')
> + body=
> +for testing nested structs
> doc symbol=EnumOne expr=('enum', 'EnumOne')
> doc symbol=UserDefZero expr=('struct', 'UserDefZero')
> doc symbol=UserDefTwoDictDict expr=('struct', 'UserDefTwoDictDict')
> --
> 2.9.3
>
>
>
> --
Marc-André Lureau
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH v2 0/3] simplify struct QOM properties and use the result for GUEST_PANICKED
2017-02-22 18:04 [Qemu-devel] [PATCH v2 0/3] simplify struct QOM properties and use the result for GUEST_PANICKED Paolo Bonzini
` (3 preceding siblings ...)
2017-02-22 18:13 ` [Qemu-devel] [PATCH v2 0/3] simplify struct QOM properties and use the result for GUEST_PANICKED no-reply
@ 2017-02-23 8:34 ` Marc-André Lureau
4 siblings, 0 replies; 13+ messages in thread
From: Marc-André Lureau @ 2017-02-23 8:34 UTC (permalink / raw)
To: Paolo Bonzini, qemu-devel
On Wed, Feb 22, 2017 at 10:07 PM Paolo Bonzini <pbonzini@redhat.com> wrote:
> This is an alternative approach to simplifying the crash information
> patches.
>
> Currently, crash information is exposed twice, through a QOM property
> and through a method. This is because accessing QOM properties with
> QAPI struct types is a huge pain in the neck. Patch 1 fixes this by
> providing a simple and well-tested API, that takes care of integrating
> the QOM->QObject->QAPI steps into a single function.
>
> Patch 2 then eliminates the get_crash_info method. Patch 3 finally
> cleans up qemu_system_guest_panicked by passing a CPUState* argument
> instead of a GuestPanicInformation struct.
>
> Paolo
>
> v1->v2:
> include missing changes to tests/qapi-schema/qapi-schema-test.out
> tweaks to doc comments
> wrap long lines
>
> Paolo Bonzini (3):
> qom-qobject: introduce object_property_{g,s}et_ptr
> cpu: implement get_crash_info through QOM properties
> vl: pass CPUState to qemu_system_guest_panicked
>
With the test leaks fixed,
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>
>
> include/qom/cpu.h | 1 -
> include/qom/qom-qobject.h | 68 +++++++++++-
> include/sysemu/sysemu.h | 2 +-
> kvm-all.c | 2 +-
> qom/cpu.c | 11 +-
> qom/qom-qobject.c | 52 ++++++++++
> target/i386/cpu.c | 2 +-
> tests/Makefile.include | 2 +-
> tests/check-qom-proplist.c | 177
> +++++++++++++++++++++++++++++++-
> tests/qapi-schema/qapi-schema-test.json | 8 ++
> tests/qapi-schema/qapi-schema-test.out | 6 ++
> vl.c | 13 ++-
> 12 files changed, 326 insertions(+), 18 deletions(-)
>
> --
> 2.9.3
>
>
> --
Marc-André Lureau
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH 1/3] qom-qobject: introduce object_property_{g, s}et_ptr
2017-02-22 18:04 ` [Qemu-devel] [PATCH 1/3] qom-qobject: introduce object_property_{g, s}et_ptr Paolo Bonzini
2017-02-22 23:34 ` Eric Blake
2017-02-23 8:33 ` Marc-André Lureau
@ 2017-02-24 14:54 ` Markus Armbruster
2017-02-24 15:29 ` Paolo Bonzini
2 siblings, 1 reply; 13+ messages in thread
From: Markus Armbruster @ 2017-02-24 14:54 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, marcandre.lureau
Paolo Bonzini <pbonzini@redhat.com> writes:
> The functions simplify the handling of QOM properties whose type
> is a QAPI struct. They go through a QObject just like the other
> functions that access a QOM property through its C type.
>
> Like QAPI_CLONE, the functions are wrapped by macros that take a
> QAPI type name and use it to build the name of a visitor function.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> include/qom/qom-qobject.h | 68 +++++++++++-
> qom/qom-qobject.c | 52 ++++++++++
> tests/Makefile.include | 2 +-
> tests/check-qom-proplist.c | 177 +++++++++++++++++++++++++++++++-
> tests/qapi-schema/qapi-schema-test.json | 8 ++
> tests/qapi-schema/qapi-schema-test.out | 6 ++
> 6 files changed, 309 insertions(+), 4 deletions(-)
>
> diff --git a/include/qom/qom-qobject.h b/include/qom/qom-qobject.h
> index 77cd717..fab9c4f 100644
> --- a/include/qom/qom-qobject.h
> +++ b/include/qom/qom-qobject.h
> @@ -30,7 +30,7 @@ struct QObject *object_property_get_qobject(Object *obj, const char *name,
> /**
> * object_property_set_qobject:
> * @obj: the object
> - * @ret: The value that will be written to the property.
> + * @ret: The value that will be written to the property
> * @name: the name of the property
> * @errp: returns an error if this function fails
> *
> @@ -39,4 +39,70 @@ struct QObject *object_property_get_qobject(Object *obj, const char *name,
> void object_property_set_qobject(Object *obj, struct QObject *qobj,
> const char *name, struct Error **errp);
>
> +/**
> + * object_property_get_ptr:
> + * @obj: the object
> + * @name: the name of the property
> + * @visit_type: the visitor function for the returned object
> + * @errp: returns an error if this function fails
> + *
> + * Return: the value of an object's property, unmarshaled into a C object
> + * through a QAPI type visitor, or NULL if an error occurs.
> + */
> +void *object_property_get_ptr(Object *obj, const char *name,
> + void (*visit_type)(Visitor *, const char *,
> + void **, Error **),
> + Error **errp);
> +
> +/**
> + * OBJECT_PROPERTY_GET_PTR:
> + * @obj: the object
> + * @name: the name of the property
> + * @type: the name of the C struct type that is returned
> + * @errp: returns an error if this function fails
> + *
> + * Return: the value of an object's property, unmarshaled into a C object
> + * through a QAPI type visitor, or NULL if an error occurs.
> + */
> +#define OBJECT_PROPERTY_GET_PTR(obj, name, type, errp) \
> + ((type *) \
> + object_property_get_ptr(obj, name, \
> + (void (*)(Visitor *, const char *, void**, \
> + Error **))visit_type_ ## type, \
> + errp))
> +
> +/**
> + * object_property_set_ptr:
> + * @obj: the object
> + * @ptr: The C struct that will be written to the property
> + * @name: the name of the property
> + * @visit_type: the visitor function for @ptr's type
> + * @errp: returns an error if this function fails
> + *
> + * Sets an object's property to a C object's value, using a QAPI
> + * type visitor to marshal the C struct into the property value.
> + */
> +void object_property_set_ptr(Object *obj, void *ptr, const char *name,
> + void (*visit_type)(Visitor *, const char *,
> + void **, Error **),
> + Error **errp);
> +
> +/**
> + * OBJECT_PROPERTY_SET_PTR:
> + * @obj: the object
> + * @ptr: The C struct that will be written to the property
> + * @name: the name of the property
> + * @type: the name of the C struct type pointed to by @ptr
> + * @errp: returns an error if this function fails
> + *
> + * Sets an object's property to a C object's value, using a QAPI
> + * type visitor to marshal the C struct into the property value.
> + */
> +#define OBJECT_PROPERTY_SET_PTR(obj, ptr, name, type, errp) \
> + object_property_set_ptr(obj, ptr + type_check(type, typeof(*ptr)), \
> + name, \
> + (void (*)(Visitor *, const char *, void**, \
> + Error **))visit_type_ ## type, \
> + errp)
Same function type punning as we use for QAPI clone. I don't have
better ideas.
> +
> #endif
> diff --git a/qom/qom-qobject.c b/qom/qom-qobject.c
> index 447e4a0..d4675be 100644
> --- a/qom/qom-qobject.c
> +++ b/qom/qom-qobject.c
> @@ -44,3 +44,55 @@ QObject *object_property_get_qobject(Object *obj, const char *name,
> visit_free(v);
> return ret;
> }
> +
> +void object_property_set_ptr(Object *obj, void *ptr, const char *name,
> + void (*visit_type)(Visitor *, const char *,
> + void **, Error **),
> + Error **errp)
> +{
> + Error *local_err = NULL;
> + QObject *ret = NULL;
> + Visitor *v;
> + v = qobject_output_visitor_new(&ret);
> + visit_type(v, name, &ptr, &local_err);
> + if (local_err) {
> + error_propagate(errp, local_err);
> + visit_free(v);
> + return;
> + }
> + visit_complete(v, &ret);
> + visit_free(v);
> +
> + /* Do not use object_property_set_qobject until we switch it
> + * to use qobject_input_visitor_new in strict mode. See the
> + * /qom/proplist/get-set-ptr/contravariant unit test.
> + */
So no conflict with my "[PATCH 15/21] qom: Make
object_property_set_qobject()'s input visitor strict".
> + v = qobject_input_visitor_new(ret, true);
> + object_property_set(obj, v, name, errp);
> + visit_free(v);
> + qobject_decref(ret);
> +}
> +
> +void *object_property_get_ptr(Object *obj, const char *name,
> + void (*visit_type)(Visitor *, const char *,
> + void **, Error **),
> + Error **errp)
> +{
> + QObject *ret;
> + Visitor *v;
> + void *ptr = NULL;
> +
> + ret = object_property_get_qobject(obj, name, errp);
> + if (!ret) {
> + return NULL;
> + }
> +
> + /* Do not enable strict mode to allow passing covariant
> + * data types.
> + */
> + v = qobject_input_visitor_new(ret, false);
Hmm, the function contract only says "unmarshaled into a C object
through a QAPI type visitor":
/**
* object_property_get_ptr:
* @obj: the object
* @name: the name of the property
* @visit_type: the visitor function for the returned object
* @errp: returns an error if this function fails
*
* Return: the value of an object's property, unmarshaled into a C object
* through a QAPI type visitor, or NULL if an error occurs.
*/
No word about covariant / contravariant. Should it be a bit more
verbose?
> + visit_type(v, name, &ptr, errp);
> + qobject_decref(ret);
> + visit_free(v);
> + return ptr;
> +}
Going through QObject gets you duck typing. More on that below.
> diff --git a/tests/Makefile.include b/tests/Makefile.include
> index e60bb6c..1a1b6e2 100644
> --- a/tests/Makefile.include
> +++ b/tests/Makefile.include
> @@ -519,7 +519,7 @@ tests/check-qfloat$(EXESUF): tests/check-qfloat.o $(test-util-obj-y)
> tests/check-qnull$(EXESUF): tests/check-qnull.o $(test-util-obj-y)
> tests/check-qjson$(EXESUF): tests/check-qjson.o $(test-util-obj-y)
> tests/check-qom-interface$(EXESUF): tests/check-qom-interface.o $(test-qom-obj-y)
> -tests/check-qom-proplist$(EXESUF): tests/check-qom-proplist.o $(test-qom-obj-y)
> +tests/check-qom-proplist$(EXESUF): tests/check-qom-proplist.o $(test-qom-obj-y) $(test-qapi-obj-y)
>
> tests/test-char$(EXESUF): tests/test-char.o $(test-util-obj-y) $(qtest-obj-y) $(test-io-obj-y) $(chardev-obj-y)
> tests/test-coroutine$(EXESUF): tests/test-coroutine.o $(test-block-obj-y)
> diff --git a/tests/check-qom-proplist.c b/tests/check-qom-proplist.c
> index a16cefc..1bf0320 100644
> --- a/tests/check-qom-proplist.c
> +++ b/tests/check-qom-proplist.c
> @@ -22,8 +22,11 @@
>
> #include "qapi/error.h"
> #include "qom/object.h"
> +#include "qom/qom-qobject.h"
> #include "qemu/module.h"
>
> +#include "test-qapi-types.h"
> +#include "test-qapi-visit.h"
>
> #define TYPE_DUMMY "qemu-dummy"
>
> @@ -56,6 +59,8 @@ struct DummyObject {
> bool bv;
> DummyAnimal av;
> char *sv;
> +
> + UserDefOne *qv;
> };
>
> struct DummyObjectClass {
> @@ -120,12 +125,42 @@ static char *dummy_get_sv(Object *obj,
>
> static void dummy_init(Object *obj)
> {
> + DummyObject *dobj = DUMMY_OBJECT(obj);
> +
> object_property_add_bool(obj, "bv",
> dummy_get_bv,
> dummy_set_bv,
> NULL);
> + dobj->qv = g_new0(UserDefOne, 1);
> + dobj->qv->string = g_strdup("dummy string");
> +}
> +
> +
> +static void dummy_get_qv(Object *obj, Visitor *v, const char *name,
> + void *opaque, Error **errp)
> +{
> + DummyObject *dobj = DUMMY_OBJECT(obj);
> +
> + visit_type_UserDefOne(v, name, &dobj->qv, errp);
> }
>
> +static void dummy_set_qv(Object *obj, Visitor *v, const char *name,
> + void *opaque, Error **errp)
> +{
> + DummyObject *dobj = DUMMY_OBJECT(obj);
> + UserDefOne *qv = NULL;
> + Error *local_err = NULL;
> +
> + visit_type_UserDefOne(v, name, &qv, &local_err);
> + if (local_err) {
> + g_assert(qv == NULL);
> + error_propagate(errp, local_err);
> + return;
> + }
> +
> + qapi_free_UserDefOne(dobj->qv);
> + dobj->qv = qv;
> +}
>
> static void dummy_class_init(ObjectClass *cls, void *data)
> {
> @@ -143,6 +178,13 @@ static void dummy_class_init(ObjectClass *cls, void *data)
> dummy_get_av,
> dummy_set_av,
> NULL);
> + object_class_property_add(cls, "qv",
> + "UserDefOne",
> + dummy_get_qv,
> + dummy_set_qv,
> + NULL,
> + NULL,
> + NULL);
> }
>
>
> @@ -151,9 +193,9 @@ static void dummy_finalize(Object *obj)
> DummyObject *dobj = DUMMY_OBJECT(obj);
>
> g_free(dobj->sv);
> + qapi_free_UserDefOne(dobj->qv);
> }
>
> -
> static const TypeInfo dummy_info = {
> .name = TYPE_DUMMY,
> .parent = TYPE_OBJECT,
> @@ -473,7 +515,8 @@ static void test_dummy_iterator(void)
>
> ObjectProperty *prop;
> ObjectPropertyIterator iter;
> - bool seenbv = false, seensv = false, seenav = false, seentype;
> + bool seenbv = false, seensv = false, seenav = false;
> + bool seenqv = false, seentype = false;
>
> object_property_iter_init(&iter, OBJECT(dobj));
> while ((prop = object_property_iter_next(&iter))) {
> @@ -483,6 +526,8 @@ static void test_dummy_iterator(void)
> seensv = true;
> } else if (g_str_equal(prop->name, "av")) {
> seenav = true;
> + } else if (g_str_equal(prop->name, "qv")) {
> + seenqv = true;
> } else if (g_str_equal(prop->name, "type")) {
> /* This prop comes from the base Object class */
> seentype = true;
> @@ -494,6 +539,7 @@ static void test_dummy_iterator(void)
> g_assert(seenbv);
> g_assert(seenav);
> g_assert(seensv);
> + g_assert(seenqv);
> g_assert(seentype);
>
> object_unparent(OBJECT(dobj));
> @@ -513,6 +559,129 @@ static void test_dummy_delchild(void)
> object_unparent(OBJECT(dev));
> }
>
> +static void test_dummy_get_set_ptr_struct(void)
> +{
> + DummyObject *dobj = DUMMY_OBJECT(object_new(TYPE_DUMMY));
> + Error *local_err = NULL;
> + const char *s = "my other dummy string";
> + UserDefOne *ret;
> + UserDefOne val;
> +
> + ret = OBJECT_PROPERTY_GET_PTR(OBJECT(dobj), "qv",
> + UserDefOne, &local_err);
> + g_assert(!local_err);
> +
> + g_assert_cmpint(ret->integer, ==, 0);
> + g_assert_cmpstr(ret->string, ==, "dummy string");
> + g_assert(!ret->has_enum1);
> + qapi_free_UserDefOne(ret);
> +
> + val.integer = 42;
> + val.string = g_strdup(s);
> + val.has_enum1 = true;
> + val.enum1 = ENUM_ONE_VALUE1;
> + OBJECT_PROPERTY_SET_PTR(OBJECT(dobj), &val, "qv",
> + UserDefOne, &local_err);
> + g_assert(!local_err);
> +
> + ret = OBJECT_PROPERTY_GET_PTR(OBJECT(dobj), "qv",
> + UserDefOne, &local_err);
> + g_assert(!local_err);
> +
> + g_assert_cmpint(ret->integer, ==, val.integer);
> + g_assert_cmpstr(ret->string, ==, val.string);
> + g_assert(ret->has_enum1);
> + g_assert_cmpint(ret->enum1, ==, val.enum1);
> + g_free(val.string);
> + qapi_free_UserDefOne(ret);
> +}
This test tests getting and setting the exact type (UserDefOne).
UserDef one has these members:
integer: int
string: str
*enum1: EnumOne
> +
> +static void test_dummy_get_set_ptr_contravariant(void)
> +{
> + DummyObject *dobj = DUMMY_OBJECT(object_new(TYPE_DUMMY));
> + Error *local_err = NULL;
> + UserDefOneMore *ret;
> + UserDefOneMore val;
> +
> + /* You cannot retrieve a contravariant (subclass) type... */
> + ret = OBJECT_PROPERTY_GET_PTR(OBJECT(dobj), "qv",
> + UserDefOneMore, &local_err);
> + error_free_or_abort(&local_err);
> + g_assert(!ret);
> +
> + /* And you cannot set one either. */
> + val.integer = 42;
> + val.string = g_strdup("unused");
> + val.has_enum1 = false;
> + val.boolean = false;
> +
> + OBJECT_PROPERTY_SET_PTR(OBJECT(dobj), &val, "qv",
> + UserDefOneMore, &local_err);
> + g_assert(local_err);
> +}
This one tests getting and setting UserDefOneMore, which has UserDefOne
as base. Members:
integer: int
string: str
*enum1: EnumOne
boolean: bool
OBJECT_PROPERTY_GET_PTR() gets UserDefOne into UserDefOneMore. It fails
because the output visitor populates only integer, string and enum1, and
the input visitor then chokes on missing boolean.
OBJECT_PROPERTY_SET_PTR() sets UserDefOne from UserDefOneMore. It fails
because the output visitor populates integer, string, enum1 and boolean,
and the input visitor then chokes on boolean.
Because OBJECT_PROPERTY_{GET,SET}_PTR() go through QObject, the "is base
of" relationship doesn't actually matter. All you need is the "right"
member names and values.
Note "values", not necessarily types! Consider
{ 'struct': 'A', 'data': { 'i': 'int' } }
{ 'struct': 'B', 'data': { 'i': 'int8' } }
Converting from A via QObject to B using output and input visitor works
as long as the value of A.i is between -128 and 127.
This is what I meant by "duck typing".
Is it what you want?
Here's a non-ducky way to convert between QAPI types. QAPI guarantees
that a pointer to a QAPI type is also valid as pointer to its base type.
One can do:
UserDefOne *one;
UserDefOneMore *more;
*(UserDefOne *)more = *one; // get UserDefOne into UserDefOneMore
// members not in one are untouched
*one = *(UserDefOne *)more; // set UserDefOne from UserDefOneMore
// members not in one are ignored
Would this technique suffice for your problem?
> +
> +static void test_dummy_get_set_ptr_covariant(void)
> +{
> + DummyObject *dobj = DUMMY_OBJECT(object_new(TYPE_DUMMY));
> + Error *local_err = NULL;
> + UserDefZero *ret;
> + UserDefZero val;
> +
> + /* You can retrieve a covariant (superclass) type... */
> + ret = OBJECT_PROPERTY_GET_PTR(OBJECT(dobj), "qv",
> + UserDefZero, &local_err);
> + g_assert(!local_err);
> +
> + g_assert_cmpint(ret->integer, ==, 0);
> + qapi_free_UserDefZero(ret);
> +
> + /* But you cannot set one. */
> + val.integer = 42;
> + OBJECT_PROPERTY_SET_PTR(OBJECT(dobj), &val, "qv",
> + UserDefZero, &local_err);
> + error_free_or_abort(&local_err);
> +
> + /* Test that the property has not been modified at all */
> + ret = OBJECT_PROPERTY_GET_PTR(OBJECT(dobj), "qv",
> + UserDefZero, &local_err);
> + g_assert(!local_err);
> +
> + g_assert_cmpint(ret->integer, ==, 0);
> + qapi_free_UserDefZero(ret);
> +}
This one tests getting and setting UserDefZero, which is UserDefOne's
base (but that doesn't matter). Members:
integer
OBJECT_PROPERTY_GET_PTR() gets UserDefOne into UserDefZero. It succeeds
because the output visitor again populates integer, string and enum1,
and the (non-strict) input visitor ignores string and enum1.
OBJECT_PROPERTY_SET_PTR() sets UserDefOne from UserDefZero. It fails
because the output visitor populates only integer, and the input visitor
then chokes on missing string and boolean.
The assymetry is between this and the previous test is a bit odd.
> +
> +static void test_dummy_get_set_ptr_error(void)
> +{
> + DummyObject *dobj = DUMMY_OBJECT(object_new(TYPE_DUMMY));
> + Error *local_err = NULL;
> + const char *s = "my other dummy string";
> + UserDefOne *ret;
> + UserDefOne val;
> +
> + ret = OBJECT_PROPERTY_GET_PTR(OBJECT(dobj), "blah",
> + UserDefOne, &local_err);
> + error_free_or_abort(&local_err);
> + g_assert(!ret);
> +
> + val.integer = 42;
> + val.string = g_strdup(s);
> + val.has_enum1 = true;
> + val.enum1 = 100;
> + OBJECT_PROPERTY_SET_PTR(OBJECT(dobj), &val, "qv",
> + UserDefOne, &local_err);
> + error_free_or_abort(&local_err);
> +
> + ret = OBJECT_PROPERTY_GET_PTR(OBJECT(dobj), "qv",
> + UserDefOne, &local_err);
> + g_assert(!local_err);
> +
> + /* Test that the property has not been modified at all */
> + g_assert_cmpint(ret->integer, ==, 0);
> + g_assert_cmpstr(ret->string, ==, "dummy string");
> + g_assert(!ret->has_enum1);
> + qapi_free_UserDefOne(ret);
> +}
Not immediately obvious what exactly this one tests. Totally different
types?
> +
> int main(int argc, char **argv)
> {
> g_test_init(&argc, &argv, NULL);
> @@ -530,5 +699,9 @@ int main(int argc, char **argv)
> g_test_add_func("/qom/proplist/iterator", test_dummy_iterator);
> g_test_add_func("/qom/proplist/delchild", test_dummy_delchild);
>
> + g_test_add_func("/qom/proplist/get-set-ptr/struct", test_dummy_get_set_ptr_struct);
> + g_test_add_func("/qom/proplist/get-set-ptr/error", test_dummy_get_set_ptr_error);
> + g_test_add_func("/qom/proplist/get-set-ptr/covariant", test_dummy_get_set_ptr_covariant);
> + g_test_add_func("/qom/proplist/get-set-ptr/contravariant", test_dummy_get_set_ptr_contravariant);
> return g_test_run();
> }
> diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json
> index f4d8cc4..4e3f6ff 100644
> --- a/tests/qapi-schema/qapi-schema-test.json
> +++ b/tests/qapi-schema/qapi-schema-test.json
> @@ -91,6 +91,14 @@
> '*enum1': 'EnumOne' } } # intentional forward reference
>
> ##
> +# @UserDefOneMore:
> +# for testing nested structs
> +##
> +{ 'struct': 'UserDefOneMore',
> + 'base': 'UserDefOne',
> + 'data': { 'boolean': 'bool' } }
I guess you need the chain 'UserDefZero' base of 'UserDefOne' base of
'UserDefOneMore'.
The naming in qapi-schema-test.json have become... suboptimal.
> +
> +##
> # @EnumOne:
> ##
> { 'enum': 'EnumOne',
> diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out
> index bc8d496..d3a2990 100644
> --- a/tests/qapi-schema/qapi-schema-test.out
> +++ b/tests/qapi-schema/qapi-schema-test.out
> @@ -107,6 +107,9 @@ object UserDefOne
> base UserDefZero
> member string: str optional=False
> member enum1: EnumOne optional=True
> +object UserDefOneMore
> + base UserDefOne
> + member boolean: bool optional=False
> object UserDefOptions
> member i64: intList optional=True
> member u64: uint64List optional=True
> @@ -283,6 +286,9 @@ for testing override of default naming heuristic
> doc symbol=UserDefOne expr=('struct', 'UserDefOne')
> body=
> for testing nested structs
> +doc symbol=UserDefOneMore expr=('struct', 'UserDefOneMore')
> + body=
> +for testing nested structs
> doc symbol=EnumOne expr=('enum', 'EnumOne')
> doc symbol=UserDefZero expr=('struct', 'UserDefZero')
> doc symbol=UserDefTwoDictDict expr=('struct', 'UserDefTwoDictDict')
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH 1/3] qom-qobject: introduce object_property_{g, s}et_ptr
2017-02-24 14:54 ` Markus Armbruster
@ 2017-02-24 15:29 ` Paolo Bonzini
2017-02-24 16:54 ` Eric Blake
2017-02-24 19:18 ` Markus Armbruster
0 siblings, 2 replies; 13+ messages in thread
From: Paolo Bonzini @ 2017-02-24 15:29 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-devel, marcandre.lureau
On 24/02/2017 15:54, Markus Armbruster wrote:
>> + /* Do not use object_property_set_qobject until we switch it
>> + * to use qobject_input_visitor_new in strict mode. See the
>> + * /qom/proplist/get-set-ptr/contravariant unit test.
>> + */
>
> So no conflict with my "[PATCH 15/21] qom: Make
> object_property_set_qobject()'s input visitor strict".
No, and in fact this comment would be an example of why that patch is
good. With 15/21 we could just use object_property_set_qobject.
> Here's a non-ducky way to convert between QAPI types. QAPI guarantees
> that a pointer to a QAPI type is also valid as pointer to its base type.
> One can do:
>
> UserDefOne *one;
> UserDefOneMore *more;
>
> *(UserDefOne *)more = *one; // get UserDefOne into UserDefOneMore
> // members not in one are untouched
> *one = *(UserDefOne *)more; // set UserDefOne from UserDefOneMore
> // members not in one are ignored
>
> Would this technique suffice for your problem?
I am not sure. What I'm trying to do here is to keep backwards
compatibility in case a device provides UserDefOneMore for a well-known
property name, and another device provides UserDefOneAnother. As long
as all devices provide the same (duck-typed) base class, things work.
Maybe the right thing to do would be to define a union, but I wasn't
sure it was possible to do that in a fully backwards compatible way (can
you define a union where the discriminator is optional, for example?).
>> +
>> +static void test_dummy_get_set_ptr_covariant(void)
>> +{
>> + DummyObject *dobj = DUMMY_OBJECT(object_new(TYPE_DUMMY));
>> + Error *local_err = NULL;
>> + UserDefZero *ret;
>> + UserDefZero val;
>> +
>> + /* You can retrieve a covariant (superclass) type... */
>> + ret = OBJECT_PROPERTY_GET_PTR(OBJECT(dobj), "qv",
>> + UserDefZero, &local_err);
>> + g_assert(!local_err);
>> +
>> + g_assert_cmpint(ret->integer, ==, 0);
>> + qapi_free_UserDefZero(ret);
>> +
>> + /* But you cannot set one. */
>> + val.integer = 42;
>> + OBJECT_PROPERTY_SET_PTR(OBJECT(dobj), &val, "qv",
>> + UserDefZero, &local_err);
>> + error_free_or_abort(&local_err);
>> +
>> + /* Test that the property has not been modified at all */
>> + ret = OBJECT_PROPERTY_GET_PTR(OBJECT(dobj), "qv",
>> + UserDefZero, &local_err);
>> + g_assert(!local_err);
>> +
>> + g_assert_cmpint(ret->integer, ==, 0);
>> + qapi_free_UserDefZero(ret);
>> +}
> This one tests getting and setting UserDefZero, which is UserDefOne's
> base (but that doesn't matter). Members:
>
> integer
>
> OBJECT_PROPERTY_GET_PTR() gets UserDefOne into UserDefZero. It succeeds
> because the output visitor again populates integer, string and enum1,
> and the (non-strict) input visitor ignores string and enum1.
>
> OBJECT_PROPERTY_SET_PTR() sets UserDefOne from UserDefZero. It fails
> because the output visitor populates only integer, and the input visitor
> then chokes on missing string and boolean.
>
> The assymetry is between this and the previous test is a bit odd.
That's true, but it makes sense I think; unlike OOP languages we don't
have a QAPI equivalent of virtual methods, which is what makes it useful
to have contravariant arguments (i.e. passing an Apple* to a food(Meal*)
method).
If you're setting UserDefOne from UserDefOneMore, some of the values are
going to be lost. Presumably there was a reason why you used
UserDefOneMore, and therefore an error is the safe bet.
If you're getting UserDefOne from UserDefOneMore, some of the values are
going to be lost. However, it's reasonable that you didn't even know
that UserDefOneMore existed, which makes it sensible to allow reading
into a covariant type.
>> +
>> +static void test_dummy_get_set_ptr_error(void)
>> +{
>> + DummyObject *dobj = DUMMY_OBJECT(object_new(TYPE_DUMMY));
>> + Error *local_err = NULL;
>> + const char *s = "my other dummy string";
>> + UserDefOne *ret;
>> + UserDefOne val;
>> +
>> + ret = OBJECT_PROPERTY_GET_PTR(OBJECT(dobj), "blah",
>> + UserDefOne, &local_err);
>> + error_free_or_abort(&local_err);
>> + g_assert(!ret);
>> +
>> + val.integer = 42;
>> + val.string = g_strdup(s);
>> + val.has_enum1 = true;
>> + val.enum1 = 100;
>> + OBJECT_PROPERTY_SET_PTR(OBJECT(dobj), &val, "qv",
>> + UserDefOne, &local_err);
>> + error_free_or_abort(&local_err);
>> +
>> + ret = OBJECT_PROPERTY_GET_PTR(OBJECT(dobj), "qv",
>> + UserDefOne, &local_err);
>> + g_assert(!local_err);
>> +
>> + /* Test that the property has not been modified at all */
>> + g_assert_cmpint(ret->integer, ==, 0);
>> + g_assert_cmpstr(ret->string, ==, "dummy string");
>> + g_assert(!ret->has_enum1);
>> + qapi_free_UserDefOne(ret);
>> +}
>
> Not immediately obvious what exactly this one tests. Totally different
> types?
enum1 is out of range, so the set fails. The test checks that integer
and string parts of the property were not touched.
I'm leaving these three patches out of the pull request (and 2.9) while
the discussion goes on.
Paolo
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH 1/3] qom-qobject: introduce object_property_{g, s}et_ptr
2017-02-24 15:29 ` Paolo Bonzini
@ 2017-02-24 16:54 ` Eric Blake
2017-02-24 17:12 ` Paolo Bonzini
2017-02-24 19:18 ` Markus Armbruster
1 sibling, 1 reply; 13+ messages in thread
From: Eric Blake @ 2017-02-24 16:54 UTC (permalink / raw)
To: Paolo Bonzini, Markus Armbruster; +Cc: marcandre.lureau, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 2484 bytes --]
On 02/24/2017 09:29 AM, Paolo Bonzini wrote:
>
>> Here's a non-ducky way to convert between QAPI types. QAPI guarantees
>> that a pointer to a QAPI type is also valid as pointer to its base type.
>> One can do:
>>
>> UserDefOne *one;
>> UserDefOneMore *more;
>>
>> *(UserDefOne *)more = *one; // get UserDefOne into UserDefOneMore
>> // members not in one are untouched
>> *one = *(UserDefOne *)more; // set UserDefOne from UserDefOneMore
>> // members not in one are ignored
And rather than having to write the casts yourself, the generator
produces qapi_UserDefOneMore_base() which returns the proper UserDefOne
pointer (giving you a bit more type safety, and isolates you from any
generator change in layout).
>>
>> Would this technique suffice for your problem?
>
> I am not sure. What I'm trying to do here is to keep backwards
> compatibility in case a device provides UserDefOneMore for a well-known
> property name, and another device provides UserDefOneAnother. As long
> as all devices provide the same (duck-typed) base class, things work.
>
> Maybe the right thing to do would be to define a union, but I wasn't
> sure it was possible to do that in a fully backwards compatible way (can
> you define a union where the discriminator is optional, for example?).
Not yet, although I've discussed the idea of an optional discriminator
several times before. As soon as we have a killer use case where an
optional discriminator makes sense, it shouldn't be too hard to add that
support into the generator.
>
> If you're setting UserDefOne from UserDefOneMore, some of the values are
> going to be lost. Presumably there was a reason why you used
> UserDefOneMore, and therefore an error is the safe bet.
>
> If you're getting UserDefOne from UserDefOneMore, some of the values are
> going to be lost. However, it's reasonable that you didn't even know
> that UserDefOneMore existed, which makes it sensible to allow reading
> into a covariant type.
How often to we add qapi subtypes, but not adjust the rest of the code
base to cope with it existing? Is it going to be less of a maintenance
burden just patching all the uses of the property getters to deal with
the new type than it is to keep the non-strict visitor?
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH 1/3] qom-qobject: introduce object_property_{g, s}et_ptr
2017-02-24 16:54 ` Eric Blake
@ 2017-02-24 17:12 ` Paolo Bonzini
0 siblings, 0 replies; 13+ messages in thread
From: Paolo Bonzini @ 2017-02-24 17:12 UTC (permalink / raw)
To: Eric Blake, Markus Armbruster; +Cc: marcandre.lureau, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 970 bytes --]
On 24/02/2017 17:54, Eric Blake wrote:
>> If you're setting UserDefOne from UserDefOneMore, some of the values are
>> going to be lost. Presumably there was a reason why you used
>> UserDefOneMore, and therefore an error is the safe bet.
>>
>> If you're getting UserDefOne from UserDefOneMore, some of the values are
>> going to be lost. However, it's reasonable that you didn't even know
>> that UserDefOneMore existed, which makes it sensible to allow reading
>> into a covariant type.
>
> How often to we add qapi subtypes, but not adjust the rest of the code
> base to cope with it existing? Is it going to be less of a maintenance
> burden just patching all the uses of the property getters to deal with
> the new type than it is to keep the non-strict visitor?
It's not about adjusting the rest the code, it's about the other code
not caring about the data in the subtype. Why should it be using it
rather than the supertype?
Paolo
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH 1/3] qom-qobject: introduce object_property_{g, s}et_ptr
2017-02-24 15:29 ` Paolo Bonzini
2017-02-24 16:54 ` Eric Blake
@ 2017-02-24 19:18 ` Markus Armbruster
1 sibling, 0 replies; 13+ messages in thread
From: Markus Armbruster @ 2017-02-24 19:18 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: marcandre.lureau, qemu-devel
Paolo Bonzini <pbonzini@redhat.com> writes:
[...]
> I'm leaving these three patches out of the pull request (and 2.9) while
> the discussion goes on.
Appreciated! Please remind me to come back to this discussion once the
freeze madness has died down.
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2017-02-24 19:18 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-02-22 18:04 [Qemu-devel] [PATCH v2 0/3] simplify struct QOM properties and use the result for GUEST_PANICKED Paolo Bonzini
2017-02-22 18:04 ` [Qemu-devel] [PATCH 1/3] qom-qobject: introduce object_property_{g, s}et_ptr Paolo Bonzini
2017-02-22 23:34 ` Eric Blake
2017-02-23 8:33 ` Marc-André Lureau
2017-02-24 14:54 ` Markus Armbruster
2017-02-24 15:29 ` Paolo Bonzini
2017-02-24 16:54 ` Eric Blake
2017-02-24 17:12 ` Paolo Bonzini
2017-02-24 19:18 ` Markus Armbruster
2017-02-22 18:04 ` [Qemu-devel] [PATCH 2/3] cpu: implement get_crash_info through QOM properties Paolo Bonzini
2017-02-22 18:04 ` [Qemu-devel] [PATCH 3/3] vl: pass CPUState to qemu_system_guest_panicked Paolo Bonzini
2017-02-22 18:13 ` [Qemu-devel] [PATCH v2 0/3] simplify struct QOM properties and use the result for GUEST_PANICKED no-reply
2017-02-23 8:34 ` Marc-André Lureau
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.