[Qemu-devel] [PATCH v2 0/3] simplify struct QOM properties and use the result for GUEST_PANICKED

[Qemu-devel] [PATCH v2 0/3] simplify struct QOM properties and use the result for GUEST_PANICKED

[Paolo Bonzini]

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

[Qemu-devel] [PATCH 1/3] qom-qobject: introduce object_property_{g, s}et_ptr

[Paolo Bonzini]

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

Re: [Qemu-devel] [PATCH 1/3] qom-qobject: introduce object_property_{g, s}et_ptr

[Eric Blake]

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

Re: [Qemu-devel] [PATCH 1/3] qom-qobject: introduce object_property_{g, s}et_ptr

[Marc-André Lureau]

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

Re: [Qemu-devel] [PATCH 1/3] qom-qobject: introduce object_property_{g, s}et_ptr

[Markus Armbruster]

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')

Re: [Qemu-devel] [PATCH 1/3] qom-qobject: introduce object_property_{g, s}et_ptr

[Paolo Bonzini]

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

Re: [Qemu-devel] [PATCH 1/3] qom-qobject: introduce object_property_{g, s}et_ptr

[Eric Blake]

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

Re: [Qemu-devel] [PATCH 1/3] qom-qobject: introduce object_property_{g, s}et_ptr

[Paolo Bonzini]

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

Re: [Qemu-devel] [PATCH 1/3] qom-qobject: introduce object_property_{g, s}et_ptr

[Markus Armbruster]

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.

[Qemu-devel] [PATCH 2/3] cpu: implement get_crash_info through QOM properties

[Paolo Bonzini]

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

[Qemu-devel] [PATCH 3/3] vl: pass CPUState to qemu_system_guest_panicked

[Paolo Bonzini]

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

Re: [Qemu-devel] [PATCH v2 0/3] simplify struct QOM properties and use the result for GUEST_PANICKED

[no-reply]

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

Re: [Qemu-devel] [PATCH v2 0/3] simplify struct QOM properties and use the result for GUEST_PANICKED

[Marc-André Lureau]

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

[Qemu-devel] [PATCH 0/3] simplify struct QOM properties and use the result for GUEST_PANICKED

[Paolo Bonzini]

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

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                       |  49 +++++++++
 target/i386/cpu.c                       |   2 +-
 tests/Makefile.include                  |   2 +-
 tests/check-qom-proplist.c              | 185 +++++++++++++++++++++++++++++++-
 tests/qapi-schema/qapi-schema-test.json |   8 ++
 vl.c                                    |  13 ++-
 11 files changed, 326 insertions(+), 17 deletions(-)

-- 
2.9.3

[Qemu-devel] [PATCH 1/3] qom-qobject: introduce object_property_{g, s}et_ptr

[Paolo Bonzini]

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                       |  49 +++++++++
 tests/Makefile.include                  |   2 +-
 tests/check-qom-proplist.c              | 185 +++++++++++++++++++++++++++++++-
 tests/qapi-schema/qapi-schema-test.json |   8 ++
 5 files changed, 309 insertions(+), 3 deletions(-)

diff --git a/include/qom/qom-qobject.h b/include/qom/qom-qobject.h
index 77cd717..ff1d307 100644
--- a/include/qom/qom-qobject.h
+++ b/include/qom/qom-qobject.h
@@ -39,4 +39,72 @@ 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
+ * @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
+ *
+ * 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
+ * @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
+ *
+ * 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 object.
+ */
+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 object.
+ */
+#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..09a12e0 100644
--- a/qom/qom-qobject.c
+++ b/qom/qom-qobject.c
@@ -44,3 +44,52 @@ 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);
+    return ptr;
+}
diff --git a/tests/Makefile.include b/tests/Makefile.include
index 634394a..9de910b 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -515,7 +515,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 qemu-timer.o \
 	$(test-util-obj-y) $(qtest-obj-y) $(test-block-obj-y) $(chardev-obj-y)
diff --git a/tests/check-qom-proplist.c b/tests/check-qom-proplist.c
index a16cefc..e0ad880 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,137 @@ 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);
+    g_assert(local_err);
+    g_assert(!ret);
+    error_free(local_err);
+    local_err = NULL;
+
+    /* 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);
+    g_assert(local_err);
+    error_free(local_err);
+    local_err = NULL;
+
+    /* 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);
+    g_assert(local_err);
+    g_assert(!ret);
+    error_free(local_err);
+    local_err = NULL;
+
+    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);
+    g_assert(local_err);
+    error_free(local_err);
+    local_err = NULL;
+
+    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 +707,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',
-- 
2.9.3

Re: [Qemu-devel] [PATCH 1/3] qom-qobject: introduce object_property_{g, s}et_ptr

[Paolo Bonzini]

On 21/02/2017 11:42, 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                       |  49 +++++++++
>  tests/Makefile.include                  |   2 +-
>  tests/check-qom-proplist.c              | 185 +++++++++++++++++++++++++++++++-
>  tests/qapi-schema/qapi-schema-test.json |   8 ++
>  5 files changed, 309 insertions(+), 3 deletions(-)

Missing hunk:

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')

Paolo

> diff --git a/include/qom/qom-qobject.h b/include/qom/qom-qobject.h
> index 77cd717..ff1d307 100644
> --- a/include/qom/qom-qobject.h
> +++ b/include/qom/qom-qobject.h
> @@ -39,4 +39,72 @@ 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
> + * @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
> + *
> + * 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
> + * @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
> + *
> + * 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 object.
> + */
> +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 object.
> + */
> +#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..09a12e0 100644
> --- a/qom/qom-qobject.c
> +++ b/qom/qom-qobject.c
> @@ -44,3 +44,52 @@ 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);
> +    return ptr;
> +}
> diff --git a/tests/Makefile.include b/tests/Makefile.include
> index 634394a..9de910b 100644
> --- a/tests/Makefile.include
> +++ b/tests/Makefile.include
> @@ -515,7 +515,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 qemu-timer.o \
>  	$(test-util-obj-y) $(qtest-obj-y) $(test-block-obj-y) $(chardev-obj-y)
> diff --git a/tests/check-qom-proplist.c b/tests/check-qom-proplist.c
> index a16cefc..e0ad880 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,137 @@ 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);
> +    g_assert(local_err);
> +    g_assert(!ret);
> +    error_free(local_err);
> +    local_err = NULL;
> +
> +    /* 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);
> +    g_assert(local_err);
> +    error_free(local_err);
> +    local_err = NULL;
> +
> +    /* 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);
> +    g_assert(local_err);
> +    g_assert(!ret);
> +    error_free(local_err);
> +    local_err = NULL;
> +
> +    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);
> +    g_assert(local_err);
> +    error_free(local_err);
> +    local_err = NULL;
> +
> +    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 +707,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',
> 

Re: [Qemu-devel] [PATCH 1/3] qom-qobject: introduce object_property_{g, s}et_ptr

[Marc-André Lureau]

Hi

On Tue, Feb 21, 2017 at 4:17 PM Paolo Bonzini <pbonzini@redhat.com> wrote:

>
>
> On 21/02/2017 11:42, 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                       |  49 +++++++++
> >  tests/Makefile.include                  |   2 +-
> >  tests/check-qom-proplist.c              | 185
> +++++++++++++++++++++++++++++++-
> >  tests/qapi-schema/qapi-schema-test.json |   8 ++
> >  5 files changed, 309 insertions(+), 3 deletions(-)
>
> Missing hunk:
>
> 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')
>
> Paolo
>
> > diff --git a/include/qom/qom-qobject.h b/include/qom/qom-qobject.h
> > index 77cd717..ff1d307 100644
> > --- a/include/qom/qom-qobject.h
> > +++ b/include/qom/qom-qobject.h
> > @@ -39,4 +39,72 @@ 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
> > + * @ptr: The C struct that will be written to the property.
>

drop that line


> > + * @name: the name of the property
> > + * @visit_type: the visitor function for @ptr's type.
> > + * @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
> > + * @ptr: The C struct that will be written to the property.
>

drop that line


> > + * @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
> > + *
> > + * 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 object.
> > + */
> > +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 object.
> > + */
> > +#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..09a12e0 100644
> > --- a/qom/qom-qobject.c
> > +++ b/qom/qom-qobject.c
> > @@ -44,3 +44,52 @@ 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 634394a..9de910b 100644
> > --- a/tests/Makefile.include
> > +++ b/tests/Makefile.include
> > @@ -515,7 +515,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 qemu-timer.o \
> >       $(test-util-obj-y) $(qtest-obj-y) $(test-block-obj-y)
> $(chardev-obj-y)
> > diff --git a/tests/check-qom-proplist.c b/tests/check-qom-proplist.c
> > index a16cefc..e0ad880 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,137 @@ 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);
> > +    g_assert(local_err);
> > +    g_assert(!ret);
> > +    error_free(local_err);
> > +    local_err = NULL;
> > +
> > +    /* 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);
> > +    g_assert(local_err);
> > +    error_free(local_err);
> > +    local_err = NULL;
> > +
> > +    /* 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);
> > +    g_assert(local_err);
> > +    g_assert(!ret);
> > +    error_free(local_err);
> > +    local_err = NULL;
> > +
> > +    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);
> > +    g_assert(local_err);
> > +    error_free(local_err);
> > +    local_err = NULL;
> > +
> > +    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 +707,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',
> >
>
> --
Marc-André Lureau

Re: [Qemu-devel] [PATCH 1/3] qom-qobject: introduce object_property_{g, s}et_ptr

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 10781 bytes --]

On 02/21/2017 04:42 AM, 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.

I'm glad I got QAPI_CLONE working - it has proven to be a nice source of
inspiration for further cleanups.

> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  include/qom/qom-qobject.h               |  68 ++++++++++++
>  qom/qom-qobject.c                       |  49 +++++++++
>  tests/Makefile.include                  |   2 +-
>  tests/check-qom-proplist.c              | 185 +++++++++++++++++++++++++++++++-
>  tests/qapi-schema/qapi-schema-test.json |   8 ++
>  5 files changed, 309 insertions(+), 3 deletions(-)
> 
> diff --git a/include/qom/qom-qobject.h b/include/qom/qom-qobject.h
> index 77cd717..ff1d307 100644
> --- a/include/qom/qom-qobject.h
> +++ b/include/qom/qom-qobject.h
> @@ -39,4 +39,72 @@ 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
> + * @ptr: The C struct that will be written to the property.

Maybe: The C struct that will be set to the property's value

Except there is no ptr parameter in this function signature.
Copy-and-paste leftover?

> + * @name: the name of the property

Inconsistent use of trailing '.'

> + * @visit_type: the visitor function for @ptr's type.

Since there is no @ptr, this needs a touchup.

> + * @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
> + * @ptr: The C struct that will be written to the property.

Again, no ptr parameter.

> + * @name: the name of the property
> + * @type: the name of the C struct type pointed to by @ptr.

and again

> + * @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,       \

Is it worth a typedef somewhere to make it easier to cast between
visitor functions?

> +                             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.

It's a bit weird to separate 'ptr' and 'visit_type' by another
parameter, since they are so integrally related; can we reorder the
parameters to put 'name' before 'ptr', or does that look inconsistent
compared to other existing property manipulations?

> + * @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 object.
> + */
> +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.

Same ordering question, same trailing '.' observation

> + * @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 object.
> + */
> +#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,        \

and the typedef'd function pointer may again come in handy.

> +                            errp)
> +
>  #endif
> diff --git a/qom/qom-qobject.c b/qom/qom-qobject.c
> index 447e4a0..09a12e0 100644
> --- a/qom/qom-qobject.c
> +++ b/qom/qom-qobject.c
> @@ -44,3 +44,52 @@ 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 **),

Long line; you wrapped it in the .h, so you could do so here, too.

> +                             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.

Interesting observation (and useful comment). I wonder how far we are
from killing off non-strict mode, but that's an independent question not
for this series.

> +     */
> +    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);
> +    return ptr;
> +}

> +++ b/tests/check-qom-proplist.c

> +
> +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);
> +    g_assert(local_err);
> +    g_assert(!ret);
> +    error_free(local_err);

Markus just posted a cleanup patch that uses error_free_or_abort()
instead of multiple lines.

This check makes sense (we can't populate userDefOneMore, because it has
a mandatory field in the subtype that was not stored in the property).

> +    local_err = NULL;
> +
> +    /* 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);

Leaks local_err; use error_free_or_abort().

Makes sense, because the subtype has an extra field that isn't supported
by the property.

> +}
> +
> +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);

You coded a non-strict visitor above to allow this to happen, but is it
really what we want?  It basically means we are grabbing the property
fields we care about, while ignoring the rest of the property.  I guess
it may be okay.

> +
> +    /* But you cannot set one.  */
> +    val.integer = 42;
> +    OBJECT_PROPERTY_SET_PTR(OBJECT(dobj), &val, "qv",
> +                            UserDefZero, &local_err);
> +    g_assert(local_err);
> +    error_free(local_err);
> +    local_err = NULL;

Makes sense that we cannot set something that does not have enough
fields present to match what the property is expecting.

> +
> +    /* 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);
> +}
> +
> +++ b/tests/qapi-schema/qapi-schema-test.json
> @@ -91,6 +91,14 @@
>              '*enum1': 'EnumOne' } }   # intentional forward reference
>  
>  ##
> +# @UserDefOneMore:
> +# for testing nested structs

Is nested the right word here?

> +##
> +{ 'struct': 'UserDefOneMore',
> +  'base': 'UserDefOne',
> +  'data': { 'boolean': 'bool' } }
> +
> +##
>  # @EnumOne:
>  ##
>  { 'enum': 'EnumOne',
> 

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

Re: [Qemu-devel] [PATCH 1/3] qom-qobject: introduce object_property_{g, s}et_ptr

[Paolo Bonzini]

[-- Attachment #1: Type: text/plain, Size: 1587 bytes --]



On 21/02/2017 16:57, Eric Blake wrote:
>> +    /* 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);
> 
> You coded a non-strict visitor above to allow this to happen, but is it
> really what we want?  It basically means we are grabbing the property
> fields we care about, while ignoring the rest of the property.  I guess
> it may be okay.

The tests were very useful to write, because I had hardly gotten any of
the corner cases right. :)  I think these semantics make the most sense.

Yeah, it uses non-strict mode but this test provides a reason (namely,
covariant return types) why non-strict mode is useful.  It makes
backwards-compatibility easier.

>> +
>> +    /* 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);
>> +}
>> +
>> +++ b/tests/qapi-schema/qapi-schema-test.json
>> @@ -91,6 +91,14 @@
>>              '*enum1': 'EnumOne' } }   # intentional forward reference
>>  
>>  ##
>> +# @UserDefOneMore:
>> +# for testing nested structs
> Is nested the right word here?

Just copied from UserDefOne. :)  I'll change both to "derived".

Paolo


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]