[RFC PATCH 00/12] QOM/QAPI integration part 1

[RFC PATCH 00/12] QOM/QAPI integration part 1

[Kevin Wolf]

This series adds QOM class definitions to the QAPI schema, introduces
a new TypeInfo.instance_config() callback that configures the object at
creation time (instead of setting properties individually) and is
separate from runtime property setters (which often used to be not
really tested for runtime use), and finally generates a marshalling
function for .instance_config() from the QAPI schema that makes this a
natural C interface rather than a visitor based one.

This is loosely based on Paolo's old proposal in the wiki:
https://wiki.qemu.org/Features/QOM-QAPI_integration

The series is in a rather early stage and I don't really have any
automated tests or documentation in this series yet. I'm also only
converting the class hierarchy for the random number generator backends
to show what the result looks like, the other objects still need to be
done.

So the question to you isn't whether this is mergeable (it isn't), but
whether you think this is the right approach for starting to integrate
QOM and QAPI better.

You'll also see that this doesn't really remove the duplication between
property definitions in the code and configuration struct definitions in
the QAPI schema yet (because we want to keep at least a read-only
runtime property for every configuration option), but at least they mean
somewhat different things now (creation vs. runtime) instead of being
completely redundant.

Possible future steps:

* Define at least those properties to the schema that correspond to a
  config option. For both setters and getters for each option, we'll
  probably want to select in the schema between 'not available',
  'automatically generated function' and 'manually implemented'.

  Other runtime properties could be either left in the code or added to
  the schema as well. Either way, we need to figure out how to best
  describe these things in the schema.

* Getting rid of the big 'object-add' union: While the union is not too
  bad for the rather small number of user-creatable objects, it
  wouldn't scale at all for devices.

  My idea there is that we could define something like this:

  { 'struct': 'ObjectOptions',
    'data': {
        'id': 'str',
        'config': { 'type': 'qom-config-any:user-creatable',
                    'embed': true } } }

  Obviously this would be an extension of the schema language to add an
  'embed' option (another hopefully more acceptable attempt to flatten
  things...), so I'd like to hear opinions on this first before I go to
  implement it.

  Also note that 'qom-config-any:user-creatable' is new, too. The
  'qom-config:...' types introduced by this series don't work for
  subclasses, but only for the exact class.

  On the external interface, the new 'qom-config-any:...' type including
  subclasses would basically behave (and be introspected) like the union
  we have today, just without being defined explicitly.

  As for the C representation for configurations that include
  subclasses, I'm imagining a struct that just contains the qom_type
  string (so we can call the right handler) and a pointer to the real
  config (that is treated as opaque by the generic code).

I could probably add more, but let's just start with this for discussion
now.

Kevin Wolf (12):
  qapi: Add visit_next_struct_member()
  qom: Create object_configure()
  qom: Make object_configure() public
  qom: Add instance_config() to TypeInfo
  rng-random: Implement .instance_config
  rng-backend: Implement .instance_config
  qapi: Allow defining QOM classes
  qapi: Create qom-config:... type for classes
  qapi/qom: Convert rng-backend/random to class
  qapi: Generate QOM config marshalling code
  qapi/qom: Add class definition for rng-builtin
  qapi/qom: Add class definition for rng-egd

 qapi/qom.json                |  46 ++++++++++-----
 include/qapi/visitor-impl.h  |   3 +
 include/qapi/visitor.h       |   2 +
 include/qom/object.h         |   7 +++
 backends/rng-egd.c           |  18 +++---
 backends/rng-random.c        |  18 +++---
 backends/rng.c               |  22 ++++++--
 qapi/qapi-visit-core.c       |   6 ++
 qapi/qobject-input-visitor.c |  16 ++++++
 qom/object.c                 |  32 +++++++++++
 qom/object_interfaces.c      |  22 +-------
 scripts/qapi/expr.py         |  28 ++++++++-
 scripts/qapi/main.py         |   2 +
 scripts/qapi/qom.py          |  91 ++++++++++++++++++++++++++++++
 scripts/qapi/schema.py       | 106 +++++++++++++++++++++++++++++++++++
 qapi/meson.build             |   3 +
 qapi/trace-events            |   1 +
 17 files changed, 362 insertions(+), 61 deletions(-)
 create mode 100644 scripts/qapi/qom.py

-- 
2.31.1

[RFC PATCH 01/12] qapi: Add visit_next_struct_member()

[Kevin Wolf]

This adds a way to generically deal with an unknown set of members in
input visitors by just getting the name of the next unvisited member.
QOM object creation code will use it to have generic code that calls
property setters which then in turn have the more specific knowledge how
to visit the respective member.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 include/qapi/visitor-impl.h  |  3 +++
 include/qapi/visitor.h       |  2 ++
 qapi/qapi-visit-core.c       |  6 ++++++
 qapi/qobject-input-visitor.c | 16 ++++++++++++++++
 qapi/trace-events            |  1 +
 5 files changed, 28 insertions(+)

diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h
index 2badec5ba4..af66a850ed 100644
--- a/include/qapi/visitor-impl.h
+++ b/include/qapi/visitor-impl.h
@@ -57,6 +57,9 @@ struct Visitor
     /* Must be set to visit structs */
     void (*end_struct)(Visitor *v, void **obj);
 
+    /* Must be set for input visitors to visit structs */
+    const char *(*next_struct_member)(Visitor *v);
+
     /* Must be set; implementations may require @list to be non-null,
      * but must document it. */
     bool (*start_list)(Visitor *v, const char *name, GenericList **list,
diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
index d53a84c9ba..5a1a28f9ad 100644
--- a/include/qapi/visitor.h
+++ b/include/qapi/visitor.h
@@ -326,6 +326,8 @@ bool visit_check_struct(Visitor *v, Error **errp);
  */
 void visit_end_struct(Visitor *v, void **obj);
 
+/* TODO */
+const char *visit_next_struct_member(Visitor *v);
 
 /*** Visiting lists ***/
 
diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c
index 6c13510a2b..82fb63e459 100644
--- a/qapi/qapi-visit-core.c
+++ b/qapi/qapi-visit-core.c
@@ -70,6 +70,12 @@ void visit_end_struct(Visitor *v, void **obj)
     v->end_struct(v, obj);
 }
 
+const char *visit_next_struct_member(Visitor *v)
+{
+    trace_visit_next_struct_member(v);
+    return v->next_struct_member(v);
+}
+
 bool visit_start_list(Visitor *v, const char *name, GenericList **list,
                       size_t size, Error **errp)
 {
diff --git a/qapi/qobject-input-visitor.c b/qapi/qobject-input-visitor.c
index f0b4c7ca9d..a409b841af 100644
--- a/qapi/qobject-input-visitor.c
+++ b/qapi/qobject-input-visitor.c
@@ -310,6 +310,21 @@ static void qobject_input_end_struct(Visitor *v, void **obj)
     qobject_input_pop(v, obj);
 }
 
+static const char *qobject_input_next_struct_member(Visitor *v)
+{
+    QObjectInputVisitor *qiv = to_qiv(v);
+    StackObject *tos = QSLIST_FIRST(&qiv->stack);
+    GHashTableIter iter;
+    const char *key;
+
+    assert(qobject_type(tos->obj) == QTYPE_QDICT && tos->h);
+    g_hash_table_iter_init(&iter, tos->h);
+    if (g_hash_table_iter_next(&iter, (void **)&key, NULL)) {
+        return key;
+    }
+    return false;
+}
+
 
 static bool qobject_input_start_list(Visitor *v, const char *name,
                                      GenericList **list, size_t size,
@@ -700,6 +715,7 @@ static QObjectInputVisitor *qobject_input_visitor_base_new(QObject *obj)
     v->visitor.start_struct = qobject_input_start_struct;
     v->visitor.check_struct = qobject_input_check_struct;
     v->visitor.end_struct = qobject_input_end_struct;
+    v->visitor.next_struct_member = qobject_input_next_struct_member;
     v->visitor.start_list = qobject_input_start_list;
     v->visitor.next_list = qobject_input_next_list;
     v->visitor.check_list = qobject_input_check_list;
diff --git a/qapi/trace-events b/qapi/trace-events
index ab108c4f0e..2d91bb6ae3 100644
--- a/qapi/trace-events
+++ b/qapi/trace-events
@@ -7,6 +7,7 @@ visit_complete(void *v, void *opaque) "v=%p opaque=%p"
 visit_start_struct(void *v, const char *name, void *obj, size_t size) "v=%p name=%s obj=%p size=%zu"
 visit_check_struct(void *v) "v=%p"
 visit_end_struct(void *v, void *obj) "v=%p obj=%p"
+visit_next_struct_member(void *v) "v=%p"
 
 visit_start_list(void *v, const char *name, void *obj, size_t size) "v=%p name=%s obj=%p size=%zu"
 visit_next_list(void *v, void *tail, size_t size) "v=%p tail=%p size=%zu"
-- 
2.31.1

[RFC PATCH 02/12] qom: Create object_configure()

[Kevin Wolf]

This renames object_set_properties_from_qdict() to object_configure()
and removes the QDict parameter from it: With visit_next_struct_member()
it can set all properties without looking at the keys of the QDict.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 qom/object_interfaces.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c
index 3b61c195c5..f9f5608194 100644
--- a/qom/object_interfaces.c
+++ b/qom/object_interfaces.c
@@ -42,16 +42,15 @@ bool user_creatable_can_be_deleted(UserCreatable *uc)
     }
 }
 
-static void object_set_properties_from_qdict(Object *obj, const QDict *qdict,
-                                             Visitor *v, Error **errp)
+static void object_configure(Object *obj, Visitor *v, Error **errp)
 {
-    const QDictEntry *e;
+    const char *key;
 
     if (!visit_start_struct(v, NULL, NULL, 0, errp)) {
         return;
     }
-    for (e = qdict_first(qdict); e; e = qdict_next(qdict, e)) {
-        if (!object_property_set(obj, e->key, v, errp)) {
+    while ((key = visit_next_struct_member(v))) {
+        if (!object_property_set(obj, key, v, errp)) {
             goto out;
         }
     }
@@ -69,7 +68,7 @@ void object_set_properties_from_keyval(Object *obj, const QDict *qdict,
     } else {
         v = qobject_input_visitor_new_keyval(QOBJECT(qdict));
     }
-    object_set_properties_from_qdict(obj, qdict, v, errp);
+    object_configure(obj, v, errp);
     visit_free(v);
 }
 
@@ -108,7 +107,7 @@ Object *user_creatable_add_type(const char *type, const char *id,
 
     assert(qdict);
     obj = object_new(type);
-    object_set_properties_from_qdict(obj, qdict, v, &local_err);
+    object_configure(obj, v, &local_err);
     if (local_err) {
         goto out;
     }
-- 
2.31.1

[RFC PATCH 03/12] qom: Make object_configure() public

[Kevin Wolf]

This makes object_configure() public and moves it to qom/object.c
because it will need to access TypeImpl in the future.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 include/qom/object.h    |  3 +++
 qom/object.c            | 17 +++++++++++++++++
 qom/object_interfaces.c | 17 -----------------
 3 files changed, 20 insertions(+), 17 deletions(-)

diff --git a/include/qom/object.h b/include/qom/object.h
index faae0d841f..d67ba2411d 100644
--- a/include/qom/object.h
+++ b/include/qom/object.h
@@ -770,6 +770,9 @@ void object_initialize_child_internal(Object *parent, const char *propname,
                                       void *child, size_t size,
                                       const char *type);
 
+/** TODO */
+void object_configure(Object *obj, Visitor *v, Error **errp);
+
 /**
  * object_dynamic_cast:
  * @obj: The object to cast.
diff --git a/qom/object.c b/qom/object.c
index 6be710bc40..d8da362987 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -605,6 +605,23 @@ void object_initialize_child_internal(Object *parent,
                                        &error_abort, NULL);
 }
 
+void object_configure(Object *obj, Visitor *v, Error **errp)
+{
+    const char *key;
+
+    if (!visit_start_struct(v, NULL, NULL, 0, errp)) {
+        return;
+    }
+    while ((key = visit_next_struct_member(v))) {
+        if (!object_property_set(obj, key, v, errp)) {
+            goto out;
+        }
+    }
+    visit_check_struct(v, errp);
+out:
+    visit_end_struct(v, NULL);
+}
+
 static inline bool object_property_is_child(ObjectProperty *prop)
 {
     return strstart(prop->type, "child<", NULL);
diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c
index f9f5608194..0e8875bf04 100644
--- a/qom/object_interfaces.c
+++ b/qom/object_interfaces.c
@@ -42,23 +42,6 @@ bool user_creatable_can_be_deleted(UserCreatable *uc)
     }
 }
 
-static void object_configure(Object *obj, Visitor *v, Error **errp)
-{
-    const char *key;
-
-    if (!visit_start_struct(v, NULL, NULL, 0, errp)) {
-        return;
-    }
-    while ((key = visit_next_struct_member(v))) {
-        if (!object_property_set(obj, key, v, errp)) {
-            goto out;
-        }
-    }
-    visit_check_struct(v, errp);
-out:
-    visit_end_struct(v, NULL);
-}
-
 void object_set_properties_from_keyval(Object *obj, const QDict *qdict,
                                        bool from_json, Error **errp)
 {
-- 
2.31.1

[RFC PATCH 04/12] qom: Add instance_config() to TypeInfo

[Kevin Wolf]

Instead of providing the whole configuration through property setters,
object_config() can now use the instance_config() callback. Options that
are not consumed by the callback (e.g. because they belong to a parent
class that hasn't been converted to the new callback yet) are still set
as properties.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 include/qom/object.h |  4 ++++
 qom/object.c         | 15 +++++++++++++++
 2 files changed, 19 insertions(+)

diff --git a/include/qom/object.h b/include/qom/object.h
index d67ba2411d..e60cacb54b 100644
--- a/include/qom/object.h
+++ b/include/qom/object.h
@@ -384,6 +384,9 @@ struct Object
  *   for initializing its own members.
  * @instance_post_init: This function is called to finish initialization of
  *   an object, after all @instance_init functions were called.
+ * @instance_config: This function is called to set the initial configuration
+ *   of an object.  If not provided, configuration is done through property
+ *   setters.
  * @instance_finalize: This function is called during object destruction.  This
  *   is called before the parent @instance_finalize function has been called.
  *   An object should only free the members that are unique to its type in this
@@ -419,6 +422,7 @@ struct TypeInfo
     size_t instance_align;
     void (*instance_init)(Object *obj);
     void (*instance_post_init)(Object *obj);
+    bool (*instance_config)(Object *obj, Visitor *v, Error **errp);
     void (*instance_finalize)(Object *obj);
 
     bool abstract;
diff --git a/qom/object.c b/qom/object.c
index d8da362987..6cacfa9ab1 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -60,6 +60,7 @@ struct TypeImpl
 
     void (*instance_init)(Object *obj);
     void (*instance_post_init)(Object *obj);
+    bool (*instance_config)(Object *obj, Visitor *v, Error **errp);
     void (*instance_finalize)(Object *obj);
 
     bool abstract;
@@ -124,6 +125,7 @@ static TypeImpl *type_new(const TypeInfo *info)
 
     ti->instance_init = info->instance_init;
     ti->instance_post_init = info->instance_post_init;
+    ti->instance_config = info->instance_config;
     ti->instance_finalize = info->instance_finalize;
 
     ti->abstract = info->abstract;
@@ -303,6 +305,7 @@ static void type_initialize(TypeImpl *ti)
         assert(ti->abstract);
         assert(!ti->instance_init);
         assert(!ti->instance_post_init);
+        assert(!ti->instance_config);
         assert(!ti->instance_finalize);
         assert(!ti->num_interfaces);
     }
@@ -607,11 +610,23 @@ void object_initialize_child_internal(Object *parent,
 
 void object_configure(Object *obj, Visitor *v, Error **errp)
 {
+    TypeImpl *ti;
     const char *key;
 
     if (!visit_start_struct(v, NULL, NULL, 0, errp)) {
         return;
     }
+
+    /* Call .instance_config, including for all parent classes */
+    for (ti = obj->class->type; ti; ti = ti->parent_type) {
+        if (ti->instance_config) {
+            if (!ti->instance_config(obj, v, errp)) {
+                goto out;
+            }
+        }
+    }
+
+    /* Set options not consumed by .instance_config as properties */
     while ((key = visit_next_struct_member(v))) {
         if (!object_property_set(obj, key, v, errp)) {
             goto out;
-- 
2.31.1

[RFC PATCH 05/12] rng-random: Implement .instance_config

[Kevin Wolf]

This adds an implementation for .instance_config to the rng-random
object. It is split into a function doing the actual configuration and
providing a native C interface and a marshalling function with a visitor
interface that we want to generate from the QAPI schema later.

rng-random objects are not created internally, so all callers use the
.instance_config interface, and setting the 'filename' property fails
after object creation, so the property setter for it can be removed now.

This commit demonstrates that you can have subclasses implementing
.instance_config while the parent class still uses properties for
initialisation.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 backends/rng-random.c | 27 +++++++++++++++++----------
 1 file changed, 17 insertions(+), 10 deletions(-)

diff --git a/backends/rng-random.c b/backends/rng-random.c
index 7add272edd..b221308091 100644
--- a/backends/rng-random.c
+++ b/backends/rng-random.c
@@ -14,6 +14,7 @@
 #include "sysemu/rng-random.h"
 #include "sysemu/rng.h"
 #include "qapi/error.h"
+#include "qapi/visitor.h"
 #include "qapi/qmp/qerror.h"
 #include "qemu/main-loop.h"
 #include "qemu/module.h"
@@ -89,19 +90,25 @@ static char *rng_random_get_filename(Object *obj, Error **errp)
     return g_strdup(s->filename);
 }
 
-static void rng_random_set_filename(Object *obj, const char *filename,
-                                 Error **errp)
+static bool rng_random_config(Object *obj, const char *filename, Error **errp)
 {
-    RngBackend *b = RNG_BACKEND(obj);
     RngRandom *s = RNG_RANDOM(obj);
 
-    if (b->opened) {
-        error_setg(errp, QERR_PERMISSION_DENIED);
-        return;
-    }
-
     g_free(s->filename);
     s->filename = g_strdup(filename);
+
+    return true;
+}
+
+static bool rng_random_marshal_config(Object *obj, Visitor *v, Error **errp)
+{
+    g_autofree char *filename = NULL;
+
+    if (!visit_type_str(v, "filename", &filename, errp)) {
+        return false;
+    }
+
+    return rng_random_config(obj, filename, errp);
 }
 
 static void rng_random_init(Object *obj)
@@ -131,8 +138,7 @@ static void rng_random_class_init(ObjectClass *klass, void *data)
     rbc->request_entropy = rng_random_request_entropy;
     rbc->opened = rng_random_opened;
     object_class_property_add_str(klass, "filename",
-                                  rng_random_get_filename,
-                                  rng_random_set_filename);
+                                  rng_random_get_filename, NULL);
 
 }
 
@@ -142,6 +148,7 @@ static const TypeInfo rng_random_info = {
     .instance_size = sizeof(RngRandom),
     .class_init = rng_random_class_init,
     .instance_init = rng_random_init,
+    .instance_config = rng_random_marshal_config,
     .instance_finalize = rng_random_finalize,
 };
 
-- 
2.31.1

[RFC PATCH 06/12] rng-backend: Implement .instance_config

[Kevin Wolf]

This commit really only serves as an illustration because the only
property in rng-backend is 'opened', which is useless and deprecated.

Implement .instance_config in order to show that .instance_config is
working both when parent class and subclass implement it (rng-random)
and when only the parent class implements it (rng-egd).

'opened' cannot be set after creation any more with this change. This is
an incompatible change, but its deprecation period is up anyway.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 backends/rng.c | 31 +++++++++++++++++++++++++------
 1 file changed, 25 insertions(+), 6 deletions(-)

diff --git a/backends/rng.c b/backends/rng.c
index 3757b04485..840daf0392 100644
--- a/backends/rng.c
+++ b/backends/rng.c
@@ -46,11 +46,6 @@ static bool rng_backend_prop_get_opened(Object *obj, Error **errp)
     return s->opened;
 }
 
-static void rng_backend_complete(UserCreatable *uc, Error **errp)
-{
-    object_property_set_bool(OBJECT(uc), "opened", true, errp);
-}
-
 static void rng_backend_prop_set_opened(Object *obj, bool value, Error **errp)
 {
     RngBackend *s = RNG_BACKEND(obj);
@@ -77,6 +72,29 @@ static void rng_backend_prop_set_opened(Object *obj, bool value, Error **errp)
     s->opened = true;
 }
 
+static void rng_backend_complete(UserCreatable *uc, Error **errp)
+{
+    rng_backend_prop_set_opened(OBJECT(uc), true, errp);
+}
+
+static bool rng_backend_config(Object *obj, bool opened, Error **errp)
+{
+    ERRP_GUARD();
+    rng_backend_prop_set_opened(obj, opened, errp);
+    return *errp == NULL;
+}
+
+static bool rng_backend_marshal_config(Object *obj, Visitor *v, Error **errp)
+{
+    bool opened;
+
+    if (!visit_type_bool(v, "opened", &opened, errp)) {
+        return false;
+    }
+
+    return rng_backend_config(obj, opened, errp);
+}
+
 static void rng_backend_free_request(RngRequest *req)
 {
     g_free(req->data);
@@ -122,7 +140,7 @@ static void rng_backend_class_init(ObjectClass *oc, void *data)
 
     object_class_property_add_bool(oc, "opened",
                                    rng_backend_prop_get_opened,
-                                   rng_backend_prop_set_opened);
+                                   NULL);
 }
 
 static const TypeInfo rng_backend_info = {
@@ -130,6 +148,7 @@ static const TypeInfo rng_backend_info = {
     .parent = TYPE_OBJECT,
     .instance_size = sizeof(RngBackend),
     .instance_init = rng_backend_init,
+    .instance_config = rng_backend_marshal_config,
     .instance_finalize = rng_backend_finalize,
     .class_size = sizeof(RngBackendClass),
     .class_init = rng_backend_class_init,
-- 
2.31.1

[RFC PATCH 07/12] qapi: Allow defining QOM classes

[Kevin Wolf]

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 scripts/qapi/expr.py   | 28 +++++++++++++++++-
 scripts/qapi/schema.py | 66 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 93 insertions(+), 1 deletion(-)

diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
index 3cb389e875..77550629f3 100644
--- a/scripts/qapi/expr.py
+++ b/scripts/qapi/expr.py
@@ -181,6 +181,8 @@ def check_defn_name_str(name: str, info: QAPISourceInfo, meta: str) -> None:
     """
     if meta == 'event':
         check_name_upper(name, info, meta)
+    elif meta == 'class':
+        check_name_str(name, info, meta)
     elif meta == 'command':
         check_name_lower(
             name, info, meta,
@@ -557,6 +559,24 @@ def check_alternate(expr: _JSONObject, info: QAPISourceInfo) -> None:
         check_type(value['type'], info, source)
 
 
+def check_class(expr: _JSONObject, info: QAPISourceInfo) -> None:
+    """
+    Normalize and validate this expression as a ``class`` definition.
+
+    :param expr: The expression to validate.
+    :param info: QAPI schema source file information.
+
+    :raise QAPISemError: When ``expr`` is not a valid ``class``.
+    :return: None, ``expr`` is normalized in-place as needed.
+    """
+    config = expr.get('config')
+    config_boxed = expr.get('config-boxed', False)
+
+    if config_boxed and config is None:
+        raise QAPISemError(info, "'boxed': true requires 'config'")
+    check_type(config, info, "'config'", allow_dict=not config_boxed)
+
+
 def check_command(expr: _JSONObject, info: QAPISourceInfo) -> None:
     """
     Normalize and validate this expression as a ``command`` definition.
@@ -627,7 +647,7 @@ def check_exprs(exprs: List[_JSONObject]) -> List[_JSONObject]:
             continue
 
         metas = expr.keys() & {'enum', 'struct', 'union', 'alternate',
-                               'command', 'event'}
+                               'class', 'command', 'event'}
         if len(metas) != 1:
             raise QAPISemError(
                 info,
@@ -671,6 +691,12 @@ def check_exprs(exprs: List[_JSONObject]) -> List[_JSONObject]:
                        ['struct', 'data'], ['base', 'if', 'features'])
             normalize_members(expr['data'])
             check_struct(expr, info)
+        elif meta == 'class':
+            check_keys(expr, info, meta,
+                       ['class'], ['if', 'features', 'parent', 'config',
+                        'config-boxed'])
+            normalize_members(expr.get('config'))
+            check_class(expr, info)
         elif meta == 'command':
             check_keys(expr, info, meta,
                        ['command'],
diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index b7b3fc0ce4..ebf69341d7 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -155,6 +155,9 @@ def visit_object_type_flat(self, name, info, ifcond, features,
     def visit_alternate_type(self, name, info, ifcond, features, variants):
         pass
 
+    def visit_class(self, entity):
+        pass
+
     def visit_command(self, name, info, ifcond, features,
                       arg_type, ret_type, gen, success_response, boxed,
                       allow_oob, allow_preconfig, coroutine):
@@ -766,6 +769,50 @@ def __init__(self, name, info, typ, ifcond=None):
         super().__init__(name, info, typ, False, ifcond)
 
 
+class QAPISchemaClass(QAPISchemaEntity):
+    meta = 'class'
+
+    def __init__(self, name, info, doc, ifcond, features, parent,
+                 config_type, config_boxed):
+        super().__init__(name, info, doc, ifcond, features)
+
+        assert not parent or isinstance(parent, str)
+        assert not config_type or isinstance(config_type, str)
+        self._parent_name = parent
+        self.parent = None
+        self._config_type_name = config_type
+        self.config_type = None
+        self.config_boxed = config_boxed
+
+    def check(self, schema):
+        super().check(schema)
+
+        if self._parent_name:
+            self.parent = schema.lookup_entity(self._parent_name,
+                                               QAPISchemaClass)
+            if not self.parent:
+                raise QAPISemError(
+                    self.info,
+                    "Unknown parent class '%s'" % self._parent_name)
+
+        if self._config_type_name:
+            self.config_type = schema.resolve_type(
+                self._config_type_name, self.info, "class 'config'")
+            if not isinstance(self.config_type, QAPISchemaObjectType):
+                raise QAPISemError(
+                    self.info,
+                    "class 'config' cannot take %s"
+                    % self.config_type.describe())
+            if self.config_type.variants and not self.boxed:
+                raise QAPISemError(
+                    self.info,
+                    "class 'config' can take %s only with 'boxed': true"
+                    % self.config_type.describe())
+
+    def visit(self, visitor):
+        super().visit(visitor)
+        visitor.visit_class(self)
+
 class QAPISchemaCommand(QAPISchemaEntity):
     meta = 'command'
 
@@ -1110,6 +1157,23 @@ def _def_alternate_type(self, expr, info, doc):
                                     QAPISchemaVariants(
                                         None, info, tag_member, variants)))
 
+    def _def_class(self, expr, info, doc):
+        name = expr['class']
+        ifcond = QAPISchemaIfCond(expr.get('if'))
+        features = self._make_features(expr.get('features'), info)
+        parent = expr.get('parent')
+        config_type = expr.get('config')
+        config_boxed = expr.get('config-boxed')
+
+        if isinstance(config_type, OrderedDict):
+            config_type = self._make_implicit_object_type(
+                name, info, ifcond,
+                'config', self._make_members(config_type, info))
+
+        self._def_entity(QAPISchemaClass(
+            name, info, doc, ifcond, features, parent, config_type,
+            config_boxed))
+
     def _def_command(self, expr, info, doc):
         name = expr['command']
         data = expr.get('data')
@@ -1161,6 +1225,8 @@ def _def_exprs(self, exprs):
                 self._def_union_type(expr, info, doc)
             elif 'alternate' in expr:
                 self._def_alternate_type(expr, info, doc)
+            elif 'class' in expr:
+                self._def_class(expr, info, doc)
             elif 'command' in expr:
                 self._def_command(expr, info, doc)
             elif 'event' in expr:
-- 
2.31.1

[RFC PATCH 08/12] qapi: Create qom-config:... type for classes

[Kevin Wolf]

For every class that has a 'config' definition, a corresponding
'qom-config:$QOM_TYPE' type is automatically created that contains the
configuration for the class and all of its parent classes.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 scripts/qapi/schema.py | 60 +++++++++++++++++++++++++++++++++++-------
 1 file changed, 50 insertions(+), 10 deletions(-)

diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index ebf69341d7..79db42b810 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -761,6 +761,13 @@ def connect_doc(self, doc):
             for f in self.features:
                 doc.connect_feature(f)
 
+    def clone(self):
+        features = [QAPISchemaFeature(f.name, f.info, f.ifcond)
+                    for f in self.features]
+        return QAPISchemaObjectTypeMember(
+            self.name, self.info, self._type_name, self.optional,
+            self.ifcond, features)
+
 
 class QAPISchemaVariant(QAPISchemaObjectTypeMember):
     role = 'branch'
@@ -783,17 +790,11 @@ def __init__(self, name, info, doc, ifcond, features, parent,
         self._config_type_name = config_type
         self.config_type = None
         self.config_boxed = config_boxed
+        self.full_config_type = None
 
-    def check(self, schema):
-        super().check(schema)
-
-        if self._parent_name:
-            self.parent = schema.lookup_entity(self._parent_name,
-                                               QAPISchemaClass)
-            if not self.parent:
-                raise QAPISemError(
-                    self.info,
-                    "Unknown parent class '%s'" % self._parent_name)
+    def get_qom_config_type(self, schema):
+        if self.full_config_type:
+            return self.full_config_type
 
         if self._config_type_name:
             self.config_type = schema.resolve_type(
@@ -809,6 +810,40 @@ def check(self, schema):
                     "class 'config' can take %s only with 'boxed': true"
                     % self.config_type.describe())
 
+            # FIXME That's a bit ugly
+            self.config_type.check(schema)
+            members = [m.clone() for m in self.config_type.members]
+        else:
+            members = []
+
+        if self._parent_name:
+            self.parent = schema.lookup_entity(self._parent_name,
+                                               QAPISchemaClass)
+            if not self.parent:
+                raise QAPISemError(
+                    self.info,
+                    "Unknown parent class '%s'" % self._parent_name)
+
+            self.parent.get_qom_config_type(schema)
+            members += [m.clone() for m in self.parent.config_type.members]
+
+        self.full_config_type = QAPISchemaObjectType(
+            f"qom-config:{self.name}", self.info, None, self._ifcond,
+            self.features, None, members, None)
+
+        return self.full_config_type
+
+    def check(self, schema):
+        super().check(schema)
+        assert self.full_config_type
+
+    def connect_doc(self, doc=None):
+        super().connect_doc(doc)
+        doc = doc or self.doc
+        if doc:
+            if self.config_type and self.config_type.is_implicit():
+                self.config_type.connect_doc(doc)
+
     def visit(self, visitor):
         super().visit(visitor)
         visitor.visit_class(self)
@@ -1236,6 +1271,11 @@ def _def_exprs(self, exprs):
             else:
                 assert False
 
+        classes = [c for c in self._entity_list
+                   if isinstance(c,QAPISchemaClass)]
+        for c in classes:
+            self._def_entity(c.get_qom_config_type(self))
+
     def check(self):
         for ent in self._entity_list:
             ent.check(self)
-- 
2.31.1

[RFC PATCH 09/12] qapi/qom: Convert rng-backend/random to class

[Kevin Wolf]

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 qapi/qom.json | 22 ++++++++++++++++------
 1 file changed, 16 insertions(+), 6 deletions(-)

diff --git a/qapi/qom.json b/qapi/qom.json
index ccd1167808..a167e91f67 100644
--- a/qapi/qom.json
+++ b/qapi/qom.json
@@ -721,6 +721,16 @@
 { 'struct': 'RngProperties',
   'data': { '*opened': { 'type': 'bool', 'features': ['deprecated'] } } }
 
+##
+# @rng-backend:
+#
+# Base class for random number generator backends
+#
+# Since: 1.3
+##
+{ 'class': 'rng-backend',
+  'config': 'RngProperties' }
+
 ##
 # @RngEgdProperties:
 #
@@ -736,18 +746,18 @@
   'data': { 'chardev': 'str' } }
 
 ##
-# @RngRandomProperties:
+# @rng-random:
 #
-# Properties for rng-random objects.
+# Random number generator backend using a host random number device
 #
 # @filename: the filename of the device on the host to obtain entropy from
 #            (default: "/dev/urandom")
 #
 # Since: 1.3
 ##
-{ 'struct': 'RngRandomProperties',
-  'base': 'RngProperties',
-  'data': { '*filename': 'str' } }
+{ 'class': 'rng-random',
+  'parent': 'rng-backend',
+  'config': { '*filename': 'str' } }
 
 ##
 # @SevGuestProperties:
@@ -889,7 +899,7 @@
       'qtest':                      'QtestProperties',
       'rng-builtin':                'RngProperties',
       'rng-egd':                    'RngEgdProperties',
-      'rng-random':                 { 'type': 'RngRandomProperties',
+      'rng-random':                 { 'type': 'qom-config:rng-random',
                                       'if': 'CONFIG_POSIX' },
       'secret':                     'SecretProperties',
       'secret_keyring':             { 'type': 'SecretKeyringProperties',
-- 
2.31.1

[RFC PATCH 10/12] qapi: Generate QOM config marshalling code

[Kevin Wolf]

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 backends/rng-random.c | 17 ++------
 backends/rng.c        | 17 ++------
 scripts/qapi/main.py  |  2 +
 scripts/qapi/qom.py   | 91 +++++++++++++++++++++++++++++++++++++++++++
 qapi/meson.build      |  3 ++
 5 files changed, 104 insertions(+), 26 deletions(-)
 create mode 100644 scripts/qapi/qom.py

diff --git a/backends/rng-random.c b/backends/rng-random.c
index b221308091..35738df3c6 100644
--- a/backends/rng-random.c
+++ b/backends/rng-random.c
@@ -14,6 +14,7 @@
 #include "sysemu/rng-random.h"
 #include "sysemu/rng.h"
 #include "qapi/error.h"
+#include "qapi/qapi-qom-qom.h"
 #include "qapi/visitor.h"
 #include "qapi/qmp/qerror.h"
 #include "qemu/main-loop.h"
@@ -90,7 +91,8 @@ static char *rng_random_get_filename(Object *obj, Error **errp)
     return g_strdup(s->filename);
 }
 
-static bool rng_random_config(Object *obj, const char *filename, Error **errp)
+bool qom_rng_random_config(Object *obj, bool has_filename,
+                           const char *filename, Error **errp)
 {
     RngRandom *s = RNG_RANDOM(obj);
 
@@ -100,17 +102,6 @@ static bool rng_random_config(Object *obj, const char *filename, Error **errp)
     return true;
 }
 
-static bool rng_random_marshal_config(Object *obj, Visitor *v, Error **errp)
-{
-    g_autofree char *filename = NULL;
-
-    if (!visit_type_str(v, "filename", &filename, errp)) {
-        return false;
-    }
-
-    return rng_random_config(obj, filename, errp);
-}
-
 static void rng_random_init(Object *obj)
 {
     RngRandom *s = RNG_RANDOM(obj);
@@ -148,7 +139,7 @@ static const TypeInfo rng_random_info = {
     .instance_size = sizeof(RngRandom),
     .class_init = rng_random_class_init,
     .instance_init = rng_random_init,
-    .instance_config = rng_random_marshal_config,
+    .instance_config = qom_rng_random_marshal_config,
     .instance_finalize = rng_random_finalize,
 };
 
diff --git a/backends/rng.c b/backends/rng.c
index 840daf0392..d560bd7b5d 100644
--- a/backends/rng.c
+++ b/backends/rng.c
@@ -13,6 +13,7 @@
 #include "qemu/osdep.h"
 #include "sysemu/rng.h"
 #include "qapi/error.h"
+#include "qapi/qapi-qom-qom.h"
 #include "qapi/qmp/qerror.h"
 #include "qemu/module.h"
 #include "qom/object_interfaces.h"
@@ -77,24 +78,14 @@ static void rng_backend_complete(UserCreatable *uc, Error **errp)
     rng_backend_prop_set_opened(OBJECT(uc), true, errp);
 }
 
-static bool rng_backend_config(Object *obj, bool opened, Error **errp)
+bool qom_rng_backend_config(Object *obj, bool has_opened, bool opened,
+                            Error **errp)
 {
     ERRP_GUARD();
     rng_backend_prop_set_opened(obj, opened, errp);
     return *errp == NULL;
 }
 
-static bool rng_backend_marshal_config(Object *obj, Visitor *v, Error **errp)
-{
-    bool opened;
-
-    if (!visit_type_bool(v, "opened", &opened, errp)) {
-        return false;
-    }
-
-    return rng_backend_config(obj, opened, errp);
-}
-
 static void rng_backend_free_request(RngRequest *req)
 {
     g_free(req->data);
@@ -148,7 +139,7 @@ static const TypeInfo rng_backend_info = {
     .parent = TYPE_OBJECT,
     .instance_size = sizeof(RngBackend),
     .instance_init = rng_backend_init,
-    .instance_config = rng_backend_marshal_config,
+    .instance_config = qom_rng_backend_marshal_config,
     .instance_finalize = rng_backend_finalize,
     .class_size = sizeof(RngBackendClass),
     .class_init = rng_backend_class_init,
diff --git a/scripts/qapi/main.py b/scripts/qapi/main.py
index f2ea6e0ce4..b6b4a4a74d 100644
--- a/scripts/qapi/main.py
+++ b/scripts/qapi/main.py
@@ -16,6 +16,7 @@
 from .error import QAPIError
 from .events import gen_events
 from .introspect import gen_introspect
+from .qom import gen_qom
 from .schema import QAPISchema
 from .types import gen_types
 from .visit import gen_visit
@@ -52,6 +53,7 @@ def generate(schema_file: str,
     gen_commands(schema, output_dir, prefix)
     gen_events(schema, output_dir, prefix)
     gen_introspect(schema, output_dir, prefix, unmask)
+    gen_qom(schema, output_dir, prefix)
 
 
 def main() -> int:
diff --git a/scripts/qapi/qom.py b/scripts/qapi/qom.py
new file mode 100644
index 0000000000..1cca94890f
--- /dev/null
+++ b/scripts/qapi/qom.py
@@ -0,0 +1,91 @@
+"""
+QAPI QOM boilerplate generator
+
+Copyright (c) 2021 Red Hat Inc.
+
+Authors:
+ Kevin Wolf <kwolf@redhat.com>
+
+This work is licensed under the terms of the GNU GPL, version 2.
+See the COPYING file in the top-level directory.
+"""
+
+from .common import c_name, mcgen
+from .gen import (
+    build_params,
+    QAPISchemaModularCVisitor,
+)
+from .schema import (
+    QAPISchema,
+    QAPISchemaClass,
+)
+
+
+def gen_config_decl(c: QAPISchemaClass) -> str:
+    params = build_params(c.config_type, c.config_boxed,  'Error **errp')
+    return mcgen('''
+bool qom_%(name)s_marshal_config(Object *obj, Visitor *v, Error **errp);
+bool qom_%(name)s_config(Object *obj, %(params)s);
+''', name=c.c_name(), params=params)
+
+
+def gen_config_call(c: QAPISchemaClass) -> str:
+    if c.config_boxed:
+        argstr = '&config, '
+    else:
+        assert not c.config_type.variants
+        argstr = ''
+        for m in c.config_type.members:
+            if m.optional:
+                argstr += 'config.has_%s, ' % c_name(m.name)
+            argstr += 'config.%s, ' % c_name(m.name)
+
+    return f'qom_{c.c_name()}_config(obj, {argstr}errp)'
+
+def gen_config(c: QAPISchemaClass) -> str:
+    return mcgen('''
+bool qom_%(qom_type)s_marshal_config(Object *obj, Visitor *v, Error **errp)
+{
+    %(config_name)s config = {0};
+
+    if (!visit_type_%(config_name)s_members(v, &config, errp)) {
+        return false;
+    }
+
+    return %(call)s;
+}
+
+''', qom_type=c.c_name(), config_name=c.config_type.c_name(),
+     call=gen_config_call(c))
+
+
+class QAPISchemaGenQOMVisitor(QAPISchemaModularCVisitor):
+
+    def __init__(self, prefix: str):
+        super().__init__(
+            prefix, 'qapi-qom', ' * Schema-defined QOM types',
+            None, __doc__)
+
+    def _begin_user_module(self, name: str) -> None:
+        qom = self._module_basename('qapi-qom', name)
+        types = self._module_basename('qapi-types', name)
+        visit = self._module_basename('qapi-visit', name)
+        self._genc.add(mcgen('''
+#include "qemu/osdep.h"
+#include "qapi/error.h"
+#include "%(qom)s.h"
+#include "%(types)s.h"
+#include "%(visit)s.h"
+
+''', types=types, visit=visit, qom=qom))
+
+    def visit_class(self, c: QAPISchemaClass) -> None:
+        if c.config_type:
+            self._genh.add(gen_config_decl(c))
+            self._genc.add(gen_config(c))
+
+
+def gen_qom(schema: QAPISchema, output_dir: str, prefix: str) -> None:
+    vis = QAPISchemaGenQOMVisitor(prefix)
+    schema.visit(vis)
+    vis.write(output_dir)
diff --git a/qapi/meson.build b/qapi/meson.build
index c356a385e3..10c412f1ad 100644
--- a/qapi/meson.build
+++ b/qapi/meson.build
@@ -87,6 +87,7 @@ qapi_nonmodule_outputs = [
   'qapi-init-commands.h', 'qapi-init-commands.c',
   'qapi-events.h', 'qapi-events.c',
   'qapi-emit-events.c', 'qapi-emit-events.h',
+  'qapi-qom.h', 'qapi-qom.c',
 ]
 
 # First build all sources
@@ -111,6 +112,8 @@ foreach module : qapi_all_modules
       'qapi-events-@0@.h'.format(module),
       'qapi-commands-@0@.c'.format(module),
       'qapi-commands-@0@.h'.format(module),
+      'qapi-qom-@0@.c'.format(module),
+      'qapi-qom-@0@.h'.format(module),
     ]
   endif
   if module.endswith('-target')
-- 
2.31.1

[RFC PATCH 11/12] qapi/qom: Add class definition for rng-builtin

[Kevin Wolf]

This doesn't add any configuration options compared to its parent class,
so this just makes the schema a little more descriptive with no other
code changes.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 qapi/qom.json | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/qapi/qom.json b/qapi/qom.json
index a167e91f67..864c6a658b 100644
--- a/qapi/qom.json
+++ b/qapi/qom.json
@@ -731,6 +731,16 @@
 { 'class': 'rng-backend',
   'config': 'RngProperties' }
 
+##
+# @rng-builtin:
+#
+# Built-in random number generator backend
+#
+# Since: 1.3
+##
+{ 'class': 'rng-builtin',
+  'parent': 'rng-backend' }
+
 ##
 # @RngEgdProperties:
 #
@@ -897,7 +907,7 @@
       'pr-manager-helper':          { 'type': 'PrManagerHelperProperties',
                                       'if': 'CONFIG_LINUX' },
       'qtest':                      'QtestProperties',
-      'rng-builtin':                'RngProperties',
+      'rng-builtin':                'qom-config:rng-builtin',
       'rng-egd':                    'RngEgdProperties',
       'rng-random':                 { 'type': 'qom-config:rng-random',
                                       'if': 'CONFIG_POSIX' },
-- 
2.31.1

[RFC PATCH 12/12] qapi/qom: Add class definition for rng-egd

[Kevin Wolf]

Switch object creation to .instance_config and remove the property
setter that would only return an error after creation anyway.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 qapi/qom.json      | 12 ++++++------
 backends/rng-egd.c | 18 +++++++-----------
 2 files changed, 13 insertions(+), 17 deletions(-)

diff --git a/qapi/qom.json b/qapi/qom.json
index 864c6a658b..fce24428f8 100644
--- a/qapi/qom.json
+++ b/qapi/qom.json
@@ -742,18 +742,18 @@
   'parent': 'rng-backend' }
 
 ##
-# @RngEgdProperties:
+# @rng-egd:
 #
-# Properties for rng-egd objects.
+# Random number generator backend connecting to an EGD-compatible daemon
 #
 # @chardev: the name of a character device backend that provides the connection
 #           to the RNG daemon
 #
 # Since: 1.3
 ##
-{ 'struct': 'RngEgdProperties',
-  'base': 'RngProperties',
-  'data': { 'chardev': 'str' } }
+{ 'class': 'rng-egd',
+  'parent': 'rng-backend',
+  'config': { 'chardev': 'str' } }
 
 ##
 # @rng-random:
@@ -908,7 +908,7 @@
                                       'if': 'CONFIG_LINUX' },
       'qtest':                      'QtestProperties',
       'rng-builtin':                'qom-config:rng-builtin',
-      'rng-egd':                    'RngEgdProperties',
+      'rng-egd':                    'qom-config:rng-egd',
       'rng-random':                 { 'type': 'qom-config:rng-random',
                                       'if': 'CONFIG_POSIX' },
       'secret':                     'SecretProperties',
diff --git a/backends/rng-egd.c b/backends/rng-egd.c
index 4de142b9dc..89255dc6fa 100644
--- a/backends/rng-egd.c
+++ b/backends/rng-egd.c
@@ -15,6 +15,7 @@
 #include "chardev/char-fe.h"
 #include "qapi/error.h"
 #include "qapi/qmp/qerror.h"
+#include "qapi/qapi-qom-qom.h"
 #include "qemu/module.h"
 #include "qom/object.h"
 
@@ -110,17 +111,12 @@ static void rng_egd_opened(RngBackend *b, Error **errp)
                              rng_egd_chr_read, NULL, NULL, s, NULL, true);
 }
 
-static void rng_egd_set_chardev(Object *obj, const char *value, Error **errp)
+bool qom_rng_egd_config(Object *obj, const char *chardev, Error **errp)
 {
-    RngBackend *b = RNG_BACKEND(obj);
-    RngEgd *s = RNG_EGD(b);
+    RngEgd *s = RNG_EGD(obj);
 
-    if (b->opened) {
-        error_setg(errp, QERR_PERMISSION_DENIED);
-    } else {
-        g_free(s->chr_name);
-        s->chr_name = g_strdup(value);
-    }
+    s->chr_name = g_strdup(chardev);
+    return true;
 }
 
 static char *rng_egd_get_chardev(Object *obj, Error **errp)
@@ -149,8 +145,7 @@ static void rng_egd_class_init(ObjectClass *klass, void *data)
 
     rbc->request_entropy = rng_egd_request_entropy;
     rbc->opened = rng_egd_opened;
-    object_class_property_add_str(klass, "chardev",
-                                  rng_egd_get_chardev, rng_egd_set_chardev);
+    object_class_property_add_str(klass, "chardev", rng_egd_get_chardev, NULL);
 }
 
 static const TypeInfo rng_egd_info = {
@@ -158,6 +153,7 @@ static const TypeInfo rng_egd_info = {
     .parent = TYPE_RNG_BACKEND,
     .instance_size = sizeof(RngEgd),
     .class_init = rng_egd_class_init,
+    .instance_config = qom_rng_egd_marshal_config,
     .instance_finalize = rng_egd_finalize,
 };
 
-- 
2.31.1

Re: [RFC PATCH 00/12] QOM/QAPI integration part 1

[Paolo Bonzini]

On 11/3/21 18:29, Kevin Wolf wrote:
> This series adds QOM class definitions to the QAPI schema, introduces
> a new TypeInfo.instance_config() callback that configures the object at
> creation time (instead of setting properties individually) and is
> separate from runtime property setters (which often used to be not
> really tested for runtime use), and finally generates a marshalling
> function for .instance_config() from the QAPI schema that makes this a
> natural C interface rather than a visitor based one.

That's pretty cool!

Just one question: why not always use boxed configuration?  It should 
not make the instance_config types any larger, and it avoids unwieldy 
argument lists.

Also, for the obligatory bikeshedding remark, do you have any other 
plans or ideas for the colon-separated auto generated typenames?  Having 
both the "namespace" (qom) and the more specific use (config) before the 
classname is a bit weird, compared to the existing structs like 
RngRandomProperties.  Especially if boxed config is more used (or 
becomes the only possibility), it might be that qom:class-name:config, 
or even just class-name:config, make for nicer code in the object 
implementation.

Paolo

Re: [RFC PATCH 00/12] QOM/QAPI integration part 1

[Kevin Wolf]

Am 03.11.2021 um 22:26 hat Paolo Bonzini geschrieben:
> On 11/3/21 18:29, Kevin Wolf wrote:
> > This series adds QOM class definitions to the QAPI schema, introduces
> > a new TypeInfo.instance_config() callback that configures the object at
> > creation time (instead of setting properties individually) and is
> > separate from runtime property setters (which often used to be not
> > really tested for runtime use), and finally generates a marshalling
> > function for .instance_config() from the QAPI schema that makes this a
> > natural C interface rather than a visitor based one.
> 
> That's pretty cool!
> 
> Just one question: why not always use boxed configuration?  It should not
> make the instance_config types any larger, and it avoids unwieldy argument
> lists.

Basically for the same reason as for commands (and for consistency with
commands): If you have only one or two options, then creating a separate
type for them feels just a little over the top, and boxing doesn't work
with implicit types.

I really like the concise definitions without a separate struct like in:

{ 'class': 'rng-egd',
  'parent': 'rng-backend',
  'config': { 'chardev': 'str' } }

Though maybe we could make it work by giving the implicit type another
prefixed name?

> Also, for the obligatory bikeshedding remark, do you have any other plans or
> ideas for the colon-separated auto generated typenames?  Having both the
> "namespace" (qom) and the more specific use (config) before the classname is
> a bit weird, compared to the existing structs like RngRandomProperties.
> Especially if boxed config is more used (or becomes the only possibility),
> it might be that qom:class-name:config, or even just class-name:config, make
> for nicer code in the object implementation.

'qom-config:classname' isn't a type that is useful for the object
implementations at all. Its only use is really storing the whole
configuration temporarily in a QAPI C struct before applying it.

The class implementations always want to store only their "local" config
options, but 'qom-config:classname' contains those of the parent class
as well.

In my example conversion, I left RngProperties as a separate type that
is just referenced in the class definition:

{ 'class': 'rng-backend',
  'config': 'RngProperties' }

This is what you would do when rng-backend wants to store its options in
a single struct, and then it's still RngProperties.

As for the naming, I chose a prefix because I think QOM doesn't restrict
the characters used in class names, so suffixes could become ambiguous.
The colon is just a character that is invalid in QAPI user types and
looked nice to me, any other reserved character would do.

Oh, and I also wanted to say something about why not just directly using
the class name, which was my first idea: 'foo': 'iothread' looks more
like referencing an existing iothread rather than the configuration for
a new one. I wanted to leave us the option that we could possibly later
take a string for such options (a QOM path) and then pass the referenced
object to QMP commands as the proper QOM type.

(Maybe there aren't currently many commands that take specific QOM
objects, but I'm dreaming of a future state with QMP commands that take
'block-node' arguments etc.)

Kevin

Re: [RFC PATCH 00/12] QOM/QAPI integration part 1

[Paolo Bonzini]

On 11/4/21 10:07, Kevin Wolf wrote:
>> Also, for the obligatory bikeshedding remark, do you have any other plans or
>> ideas for the colon-separated auto generated typenames?  Having both the
>> "namespace" (qom) and the more specific use (config) before the classname is
>> a bit weird, compared to the existing structs like RngRandomProperties.
>> Especially if boxed config is more used (or becomes the only possibility),
>> it might be that qom:class-name:config, or even just class-name:config, make
>> for nicer code in the object implementation.
> 
> 'qom-config:classname' isn't a type that is useful for the object
> implementations at all. Its only use is really storing the whole
> configuration temporarily in a QAPI C struct before applying it.

<rubbish>
It would be useful as the (auto-boxed) argument of the configuration 
code.  So basically you'd not need the RngProperties function anymore 
because you use something like QomConfigRngBackend (or 
RngBackendQomConfig - hence the question on how to name the 
auto-generated types).
</rubbish>

> The class implementations always want to store only their "local" config
> options, but 'qom-config:classname' contains those of the parent class
> as well.

Ah, I didn't understand that (hence the rubbish tag above).  It makes 
sense given that instance_config is called per-class while ObjectOptions 
stores all the info in one class.  That's a major change from my sketch, 
which planned to call the base class config function by hand (and handle 
the marshalling via QAPI 'base': '...').

> Oh, and I also wanted to say something about why not just directly using
> the class name, which was my first idea: 'foo': 'iothread' looks more
> like referencing an existing iothread rather than the configuration for
> a new one. I wanted to leave us the option that we could possibly later
> take a string for such options (a QOM path) and then pass the referenced
> object to QMP commands as the proper QOM type.

I agree that 'iothread' is going to be confusing when you're referring 
to the configuration.

Anyway I'm totally fine with 'qom-config:classname'.  Given this 
explanation, however, one alternative that makes sense could be 
'classname:full-config'. Then you could use 'classname:config' for the 
autoboxed configs---and reserve 'classname' to mean the pointer to an 
object.  Classes are (generally) lowercase and QAPI structs are 
CamelCase, so there is not much potential for collisions.

Paolo

Re: [RFC PATCH 00/12] QOM/QAPI integration part 1

[Kevin Wolf]

Am 04.11.2021 um 13:39 hat Paolo Bonzini geschrieben:
> On 11/4/21 10:07, Kevin Wolf wrote:
> > The class implementations always want to store only their "local" config
> > options, but 'qom-config:classname' contains those of the parent class
> > as well.
> 
> Ah, I didn't understand that (hence the rubbish tag above).  It makes sense
> given that instance_config is called per-class while ObjectOptions stores
> all the info in one class.  That's a major change from my sketch, which
> planned to call the base class config function by hand (and handle the
> marshalling via QAPI 'base': '...').

Yeah, handling inheritance and how to represent things in the schema is
probably the two more interesting things this series changes compared to
your proposal.

I started with your model, but it just didn't work out nicely, because I
always had the full configuration in the child class and apart from just
being ugly, having all options of the parent class duplicated, but
ignored, would certainly be a source for a lot of confusion and bugs.

It took me a while to figure out how to deal with this, but I'm quite
happy with the result.

> > Oh, and I also wanted to say something about why not just directly using
> > the class name, which was my first idea: 'foo': 'iothread' looks more
> > like referencing an existing iothread rather than the configuration for
> > a new one. I wanted to leave us the option that we could possibly later
> > take a string for such options (a QOM path) and then pass the referenced
> > object to QMP commands as the proper QOM type.
> 
> I agree that 'iothread' is going to be confusing when you're referring to
> the configuration.
> 
> Anyway I'm totally fine with 'qom-config:classname'.  Given this
> explanation, however, one alternative that makes sense could be
> 'classname:full-config'. Then you could use 'classname:config' for the
> autoboxed configs---and reserve 'classname' to mean the pointer to an
> object.  Classes are (generally) lowercase and QAPI structs are
> CamelCase, so there is not much potential for collisions.

Makes sense to me, too.

I just checked and I actually already forbid class names with colons in
them (check_name_str() takes care of this), so yes, suffixes actually
work on the QAPI level.

If we actually want to use these types in manually written C code, we
might have to convert the name to CamelCase, though, for consistency
with the coding style.

We already have a function camel_to_upper(), we'd need a new
lower_to_camel(), so that from a class 'rng-random', you would get types
'RngRandomConfig' (the local ones) and 'RngRandomFullConfig' (with
parent options).

Kevin

Re: [RFC PATCH 00/12] QOM/QAPI integration part 1

[Paolo Bonzini]

On 11/4/21 15:26, Kevin Wolf wrote:
> It took me a while to figure out how to deal with this, but I'm quite
> happy with the result.

I like it too.

>>> Oh, and I also wanted to say something about why not just directly using
>>> the class name, which was my first idea: 'foo': 'iothread' looks more
>>> like referencing an existing iothread rather than the configuration for
>>> a new one. I wanted to leave us the option that we could possibly later
>>> take a string for such options (a QOM path) and then pass the referenced
>>> object to QMP commands as the proper QOM type.
>>
>> I agree that 'iothread' is going to be confusing when you're referring to
>> the configuration.
>>
>> Anyway I'm totally fine with 'qom-config:classname'.  Given this
>> explanation, however, one alternative that makes sense could be
>> 'classname:full-config'. Then you could use 'classname:config' for the
>> autoboxed configs---and reserve 'classname' to mean the pointer to an
>> object.  Classes are (generally) lowercase and QAPI structs are
>> CamelCase, so there is not much potential for collisions.
> 
> Makes sense to me, too.
> 
> I just checked and I actually already forbid class names with colons in
> them (check_name_str() takes care of this), so yes, suffixes actually
> work on the QAPI level.
> 
> If we actually want to use these types in manually written C code, we
> might have to convert the name to CamelCase, though, for consistency
> with the coding style.
> 
> We already have a function camel_to_upper(), we'd need a new
> lower_to_camel(), so that from a class 'rng-random', you would get types
> 'RngRandomConfig' (the local ones) and 'RngRandomFullConfig' (with
> parent options).

That's nice.  IMO with these changes the autoboxing becomes again more 
appealing.  With the auto-generated local config struct,

     { 'class': 'rng-egd',
       'parent': 'rng-backend',
       'config': { 'chardev': 'str' } }

now maps to

     bool qom_rng_egd_config(Object *obj, RngEgdConfig *config,
                             Error **errp)
     {
         RngEgd *s = RNG_EGD(obj);

         s->chr_name = g_strdup(config->chardev);
         return true;
     }

The three arguments follow the same prototype as .instance_config:

     bool (*instance_config)(Object *obj, Visitor *v, Error **errp);

just with the visitor replaced by a nice C struct.  You started 
(obviously) with the simplest cases, and it's good to check whether easy 
things remain easy, but it seems to me that all but the simplest objects 
would end up using boxed config anyway.

Also (and this is something Markus and I have discussed in the past, but 
I'm not sure if we have actually reached an agreement), I would make 
instance_config return void.  The usual convention *is* to return bool 
from functions that have an Error** and no other return value; however, 
that's because in general there will be more calls to the function than 
definitions.

In this case, there will be just one call to the ti->instance_config 
function pointer, in object_configure, and N definitions of the 
function, so the ratio and the rationale are reversed.  See 
object_property_get for an example in qom/object.c.

Thanks,

Paolo

Re: [RFC PATCH 00/12] QOM/QAPI integration part 1

[Kevin Wolf]

Am 04.11.2021 um 15:49 hat Paolo Bonzini geschrieben:
> On 11/4/21 15:26, Kevin Wolf wrote:
> > It took me a while to figure out how to deal with this, but I'm quite
> > happy with the result.
> 
> I like it too.
> 
> > > > Oh, and I also wanted to say something about why not just directly using
> > > > the class name, which was my first idea: 'foo': 'iothread' looks more
> > > > like referencing an existing iothread rather than the configuration for
> > > > a new one. I wanted to leave us the option that we could possibly later
> > > > take a string for such options (a QOM path) and then pass the referenced
> > > > object to QMP commands as the proper QOM type.
> > > 
> > > I agree that 'iothread' is going to be confusing when you're referring to
> > > the configuration.
> > > 
> > > Anyway I'm totally fine with 'qom-config:classname'.  Given this
> > > explanation, however, one alternative that makes sense could be
> > > 'classname:full-config'. Then you could use 'classname:config' for the
> > > autoboxed configs---and reserve 'classname' to mean the pointer to an
> > > object.  Classes are (generally) lowercase and QAPI structs are
> > > CamelCase, so there is not much potential for collisions.
> > 
> > Makes sense to me, too.
> > 
> > I just checked and I actually already forbid class names with colons in
> > them (check_name_str() takes care of this), so yes, suffixes actually
> > work on the QAPI level.
> > 
> > If we actually want to use these types in manually written C code, we
> > might have to convert the name to CamelCase, though, for consistency
> > with the coding style.
> > 
> > We already have a function camel_to_upper(), we'd need a new
> > lower_to_camel(), so that from a class 'rng-random', you would get types
> > 'RngRandomConfig' (the local ones) and 'RngRandomFullConfig' (with
> > parent options).
> 
> That's nice.  IMO with these changes the autoboxing becomes again more
> appealing.  With the auto-generated local config struct,
> 
>     { 'class': 'rng-egd',
>       'parent': 'rng-backend',
>       'config': { 'chardev': 'str' } }
> 
> now maps to
> 
>     bool qom_rng_egd_config(Object *obj, RngEgdConfig *config,
>                             Error **errp)
>     {
>         RngEgd *s = RNG_EGD(obj);
> 
>         s->chr_name = g_strdup(config->chardev);
>         return true;
>     }
> 
> The three arguments follow the same prototype as .instance_config:
> 
>     bool (*instance_config)(Object *obj, Visitor *v, Error **errp);
> 
> just with the visitor replaced by a nice C struct.  You started (obviously)
> with the simplest cases, and it's good to check whether easy things remain
> easy, but it seems to me that all but the simplest objects would end up
> using boxed config anyway.

I think I'd still like to have the option of unboxed arguments for the
simpler objects (most user creatable objects are relatively simple), but
flipping the default would make sense if we just automatically create a
named type.

But before I implement anything like this, I'd first like to hear
Markus's opinion because I would prefer to avoid -EMAGIC during review.

> Also (and this is something Markus and I have discussed in the past, but I'm
> not sure if we have actually reached an agreement), I would make
> instance_config return void.  The usual convention *is* to return bool from
> functions that have an Error** and no other return value; however, that's
> because in general there will be more calls to the function than
> definitions.
> 
> In this case, there will be just one call to the ti->instance_config
> function pointer, in object_configure, and N definitions of the function, so
> the ratio and the rationale are reversed.  See object_property_get for an
> example in qom/object.c.

Good point. Though not necessarily one for ti->instance_config because
that's the automatically generated marshaller and returning a bool from
there isn't a problem at all. The function that should definitely return
void is the idiomatic C one that is manually written and called by the
marshaller.

Kevin

Re: [RFC PATCH 00/12] QOM/QAPI integration part 1

[Damien Hedde]

On 11/4/21 10:07, Kevin Wolf wrote:
> Am 03.11.2021 um 22:26 hat Paolo Bonzini geschrieben:
>> On 11/3/21 18:29, Kevin Wolf wrote:
>>> This series adds QOM class definitions to the QAPI schema, introduces
>>> a new TypeInfo.instance_config() callback that configures the object at
>>> creation time (instead of setting properties individually) and is
>>> separate from runtime property setters (which often used to be not
>>> really tested for runtime use), and finally generates a marshalling
>>> function for .instance_config() from the QAPI schema that makes this a
>>> natural C interface rather than a visitor based one.
>>
>> That's pretty cool!

Hi,

Just to be sure I understand. Is this config a generalization of the 
qdev-properties we have to describe the parameter used to create a device ?

>>
>> Just one question: why not always use boxed configuration?  It should not
>> make the instance_config types any larger, and it avoids unwieldy argument
>> lists.
> 
> Basically for the same reason as for commands (and for consistency with
> commands): If you have only one or two options, then creating a separate
> type for them feels just a little over the top, and boxing doesn't work
> with implicit types.
> 
> I really like the concise definitions without a separate struct like in:
> 
> { 'class': 'rng-egd',
>    'parent': 'rng-backend',
>    'config': { 'chardev': 'str' } }
> 
> Though maybe we could make it work by giving the implicit type another
> prefixed name?

What's an implicit type in this context ?

> 
>> Also, for the obligatory bikeshedding remark, do you have any other plans or
>> ideas for the colon-separated auto generated typenames?  Having both the
>> "namespace" (qom) and the more specific use (config) before the classname is
>> a bit weird, compared to the existing structs like RngRandomProperties.
>> Especially if boxed config is more used (or becomes the only possibility),
>> it might be that qom:class-name:config, or even just class-name:config, make
>> for nicer code in the object implementation.
> 
> 'qom-config:classname' isn't a type that is useful for the object
> implementations at all. Its only use is really storing the whole
> configuration temporarily in a QAPI C struct before applying it.
> 

Would not this type be useful to generate read-only properties (and 
store the values) corresponding to the config if we expect to always 
have such properties ?

--
Damien

Re: [RFC PATCH 00/12] QOM/QAPI integration part 1

[Kevin Wolf]

Am 04.11.2021 um 16:52 hat Damien Hedde geschrieben:
> On 11/4/21 10:07, Kevin Wolf wrote:
> > Am 03.11.2021 um 22:26 hat Paolo Bonzini geschrieben:
> > > On 11/3/21 18:29, Kevin Wolf wrote:
> > > > This series adds QOM class definitions to the QAPI schema, introduces
> > > > a new TypeInfo.instance_config() callback that configures the object at
> > > > creation time (instead of setting properties individually) and is
> > > > separate from runtime property setters (which often used to be not
> > > > really tested for runtime use), and finally generates a marshalling
> > > > function for .instance_config() from the QAPI schema that makes this a
> > > > natural C interface rather than a visitor based one.
> > > 
> > > That's pretty cool!
> 
> Hi,
> 
> Just to be sure I understand. Is this config a generalization of the
> qdev-properties we have to describe the parameter used to create a
> device ?

Everything I'm doing here is on the QOM level. But qdev is just a layer
on top of QOM, so yes, eventually qdev properties should be declared in
the schema, too. It's useful to keep this long-term goal in mind, but
for now, the immediate goal is making it work for user-creatable
objects.

> > > Just one question: why not always use boxed configuration?  It should not
> > > make the instance_config types any larger, and it avoids unwieldy argument
> > > lists.
> > 
> > Basically for the same reason as for commands (and for consistency with
> > commands): If you have only one or two options, then creating a separate
> > type for them feels just a little over the top, and boxing doesn't work
> > with implicit types.
> > 
> > I really like the concise definitions without a separate struct like in:
> > 
> > { 'class': 'rng-egd',
> >    'parent': 'rng-backend',
> >    'config': { 'chardev': 'str' } }
> > 
> > Though maybe we could make it work by giving the implicit type another
> > prefixed name?
> 
> What's an implicit type in this context ?

The value of 'config' creates an implicit struct type that is considered
anonymous on the QAPI level. With an explicit type, you wouldn't put an
object containing all the options there, but just a type name, like
this:

{ 'struct': 'RngEgdConfig',
  'data': { 'chardev': 'str' } }

{ 'class': 'rng-egd',
   'parent': 'rng-backend',
   'config': 'RngEgdConfig' }

> > 
> > > Also, for the obligatory bikeshedding remark, do you have any other plans or
> > > ideas for the colon-separated auto generated typenames?  Having both the
> > > "namespace" (qom) and the more specific use (config) before the classname is
> > > a bit weird, compared to the existing structs like RngRandomProperties.
> > > Especially if boxed config is more used (or becomes the only possibility),
> > > it might be that qom:class-name:config, or even just class-name:config, make
> > > for nicer code in the object implementation.
> > 
> > 'qom-config:classname' isn't a type that is useful for the object
> > implementations at all. Its only use is really storing the whole
> > configuration temporarily in a QAPI C struct before applying it.
> 
> Would not this type be useful to generate read-only properties (and store
> the values) corresponding to the config if we expect to always have such
> properties ?

No, that's what I explained in the following paragraph. That would be a
different type, because 'qom-config:classname' contains the options for
the parent class, too, but you only want to store the options for the
class itself.

Kevin

Re: [RFC PATCH 07/12] qapi: Allow defining QOM classes

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  scripts/qapi/expr.py   | 28 +++++++++++++++++-
>  scripts/qapi/schema.py | 66 ++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 93 insertions(+), 1 deletion(-)

Missing: docs/devel/qapi-code-gen.rst update.  I understand why, but it
does make review harder.

>
> diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
> index 3cb389e875..77550629f3 100644
> --- a/scripts/qapi/expr.py
> +++ b/scripts/qapi/expr.py
> @@ -181,6 +181,8 @@ def check_defn_name_str(name: str, info: QAPISourceInfo, meta: str) -> None:
>      """
>      if meta == 'event':
>          check_name_upper(name, info, meta)
> +    elif meta == 'class':
> +        check_name_str(name, info, meta)

This permits arbitrary QAPI names.  I figure we'll want to define and
enforce a suitable naming convention.

>      elif meta == 'command':
>          check_name_lower(
>              name, info, meta,
> @@ -557,6 +559,24 @@ def check_alternate(expr: _JSONObject, info: QAPISourceInfo) -> None:
>          check_type(value['type'], info, source)
>  
>  
> +def check_class(expr: _JSONObject, info: QAPISourceInfo) -> None:
> +    """
> +    Normalize and validate this expression as a ``class`` definition.
> +
> +    :param expr: The expression to validate.
> +    :param info: QAPI schema source file information.
> +
> +    :raise QAPISemError: When ``expr`` is not a valid ``class``.
> +    :return: None, ``expr`` is normalized in-place as needed.
> +    """
> +    config = expr.get('config')
> +    config_boxed = expr.get('config-boxed', False)
> +
> +    if config_boxed and config is None:
> +        raise QAPISemError(info, "'boxed': true requires 'config'")

You check 'config-boxed', but the error message talks about 'boxed'.

> +    check_type(config, info, "'config'", allow_dict=not config_boxed)
> +
> +
>  def check_command(expr: _JSONObject, info: QAPISourceInfo) -> None:
>      """
>      Normalize and validate this expression as a ``command`` definition.
> @@ -627,7 +647,7 @@ def check_exprs(exprs: List[_JSONObject]) -> List[_JSONObject]:
>              continue
>  
>          metas = expr.keys() & {'enum', 'struct', 'union', 'alternate',
> -                               'command', 'event'}
> +                               'class', 'command', 'event'}
>          if len(metas) != 1:
>              raise QAPISemError(
>                  info,
> @@ -671,6 +691,12 @@ def check_exprs(exprs: List[_JSONObject]) -> List[_JSONObject]:
>                         ['struct', 'data'], ['base', 'if', 'features'])
>              normalize_members(expr['data'])
>              check_struct(expr, info)
> +        elif meta == 'class':
> +            check_keys(expr, info, meta,
> +                       ['class'], ['if', 'features', 'parent', 'config',
> +                        'config-boxed'])
> +            normalize_members(expr.get('config'))
> +            check_class(expr, info)
>          elif meta == 'command':
>              check_keys(expr, info, meta,
>                         ['command'],
> diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
> index b7b3fc0ce4..ebf69341d7 100644
> --- a/scripts/qapi/schema.py
> +++ b/scripts/qapi/schema.py
> @@ -155,6 +155,9 @@ def visit_object_type_flat(self, name, info, ifcond, features,
>      def visit_alternate_type(self, name, info, ifcond, features, variants):
>          pass
>  
> +    def visit_class(self, entity):
> +        pass
> +
>      def visit_command(self, name, info, ifcond, features,
>                        arg_type, ret_type, gen, success_response, boxed,
>                        allow_oob, allow_preconfig, coroutine):
> @@ -766,6 +769,50 @@ def __init__(self, name, info, typ, ifcond=None):
>          super().__init__(name, info, typ, False, ifcond)
>  
>  
> +class QAPISchemaClass(QAPISchemaEntity):
> +    meta = 'class'
> +
> +    def __init__(self, name, info, doc, ifcond, features, parent,
> +                 config_type, config_boxed):
> +        super().__init__(name, info, doc, ifcond, features)
> +
> +        assert not parent or isinstance(parent, str)

I can't see what ensures this.

> +        assert not config_type or isinstance(config_type, str)
> +        self._parent_name = parent
> +        self.parent = None
> +        self._config_type_name = config_type
> +        self.config_type = None
> +        self.config_boxed = config_boxed
> +
> +    def check(self, schema):
> +        super().check(schema)
> +
> +        if self._parent_name:
> +            self.parent = schema.lookup_entity(self._parent_name,
> +                                               QAPISchemaClass)
> +            if not self.parent:
> +                raise QAPISemError(
> +                    self.info,
> +                    "Unknown parent class '%s'" % self._parent_name)
> +
> +        if self._config_type_name:
> +            self.config_type = schema.resolve_type(
> +                self._config_type_name, self.info, "class 'config'")

"class's 'config'" for consistency with other uses of .resolve_type().

> +            if not isinstance(self.config_type, QAPISchemaObjectType):
> +                raise QAPISemError(
> +                    self.info,
> +                    "class 'config' cannot take %s"
> +                    % self.config_type.describe())
> +            if self.config_type.variants and not self.boxed:

self.config_boxed?

> +                raise QAPISemError(
> +                    self.info,
> +                    "class 'config' can take %s only with 'boxed': true"

'config-boxed'?

> +                    % self.config_type.describe())
> +
> +    def visit(self, visitor):
> +        super().visit(visitor)
> +        visitor.visit_class(self)
> +
>  class QAPISchemaCommand(QAPISchemaEntity):
>      meta = 'command'
>  
> @@ -1110,6 +1157,23 @@ def _def_alternate_type(self, expr, info, doc):
>                                      QAPISchemaVariants(
>                                          None, info, tag_member, variants)))
>  
> +    def _def_class(self, expr, info, doc):
> +        name = expr['class']
> +        ifcond = QAPISchemaIfCond(expr.get('if'))
> +        features = self._make_features(expr.get('features'), info)
> +        parent = expr.get('parent')
> +        config_type = expr.get('config')
> +        config_boxed = expr.get('config-boxed')
> +
> +        if isinstance(config_type, OrderedDict):
> +            config_type = self._make_implicit_object_type(
> +                name, info, ifcond,
> +                'config', self._make_members(config_type, info))

Does QAPISchemaMember.describe() need an update for this?

> +
> +        self._def_entity(QAPISchemaClass(
> +            name, info, doc, ifcond, features, parent, config_type,
> +            config_boxed))
> +
>      def _def_command(self, expr, info, doc):
>          name = expr['command']
>          data = expr.get('data')
> @@ -1161,6 +1225,8 @@ def _def_exprs(self, exprs):
>                  self._def_union_type(expr, info, doc)
>              elif 'alternate' in expr:
>                  self._def_alternate_type(expr, info, doc)
> +            elif 'class' in expr:
> +                self._def_class(expr, info, doc)
>              elif 'command' in expr:
>                  self._def_command(expr, info, doc)
>              elif 'event' in expr:

Re: [RFC PATCH 08/12] qapi: Create qom-config:... type for classes

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> For every class that has a 'config' definition, a corresponding
> 'qom-config:$QOM_TYPE' type is automatically created that contains the
> configuration for the class and all of its parent classes.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>

I assume $QOM_TYPE stands for the class's name.

What kind of type does this define?  Peeking at the code below... looks
like it's a struct type.

I wonder how it's related to the the type we already use or create for
the the class's 'config' member.  Which is either the name of a struct
or union type to use, or a dictionary that tells us what struct type to
create.  Let's see below.

> ---
>  scripts/qapi/schema.py | 60 +++++++++++++++++++++++++++++++++++-------
>  1 file changed, 50 insertions(+), 10 deletions(-)
>
> diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
> index ebf69341d7..79db42b810 100644
> --- a/scripts/qapi/schema.py
> +++ b/scripts/qapi/schema.py
> @@ -761,6 +761,13 @@ def connect_doc(self, doc):
>              for f in self.features:
>                  doc.connect_feature(f)
>  
> +    def clone(self):
> +        features = [QAPISchemaFeature(f.name, f.info, f.ifcond)
> +                    for f in self.features]
> +        return QAPISchemaObjectTypeMember(
> +            self.name, self.info, self._type_name, self.optional,
> +            self.ifcond, features)
> +

A copy that's somewhere between shallow and deep.  Reserving judgement.

>  
>  class QAPISchemaVariant(QAPISchemaObjectTypeMember):
>      role = 'branch'
> @@ -783,17 +790,11 @@ def __init__(self, name, info, doc, ifcond, features, parent,
>          self._config_type_name = config_type
>          self.config_type = None
>          self.config_boxed = config_boxed
> +        self.full_config_type = None
>  
> -    def check(self, schema):
> -        super().check(schema)
> -
> -        if self._parent_name:
> -            self.parent = schema.lookup_entity(self._parent_name,
> -                                               QAPISchemaClass)
> -            if not self.parent:
> -                raise QAPISemError(
> -                    self.info,
> -                    "Unknown parent class '%s'" % self._parent_name)
> +    def get_qom_config_type(self, schema):
> +        if self.full_config_type:
> +            return self.full_config_type
>  
>          if self._config_type_name:
>              self.config_type = schema.resolve_type(
> @@ -809,6 +810,40 @@ def check(self, schema):
>                      "class 'config' can take %s only with 'boxed': true"
>                      % self.config_type.describe())
>  
> +            # FIXME That's a bit ugly
> +            self.config_type.check(schema)
> +            members = [m.clone() for m in self.config_type.members]
> +        else:
> +            members = []

Have you considered defaulting ._config_type_name to 'q_empty'?

> +
> +        if self._parent_name:
> +            self.parent = schema.lookup_entity(self._parent_name,
> +                                               QAPISchemaClass)
> +            if not self.parent:
> +                raise QAPISemError(
> +                    self.info,
> +                    "Unknown parent class '%s'" % self._parent_name)
> +
> +            self.parent.get_qom_config_type(schema)
> +            members += [m.clone() for m in self.parent.config_type.members]

@members is the list of common members of the class's and all its
parents' 'config' types.

Any variant members of 'config' types get silently ignored.  Should we
reject them instead?

> +
> +        self.full_config_type = QAPISchemaObjectType(
> +            f"qom-config:{self.name}", self.info, None, self._ifcond,
> +            self.features, None, members, None)

The "full config type" inherits conditional and features from the class.
Its (common) members are the members of the class's and all its parents'
'config' types.

Could we use the parent's full config type as the base type here?

Do we need the non-full config type (the type named by or implicitly
defined by the 'config' member') for anything other than a source of
local members for the full config type?

> +
> +        return self.full_config_type
> +
> +    def check(self, schema):
> +        super().check(schema)
> +        assert self.full_config_type

New constraint: must call .get_qom_config_type() before .check().

This side effect makes me unsure sure about the "get" in the name.
Let's worry about that later.

> +
> +    def connect_doc(self, doc=None):
> +        super().connect_doc(doc)
> +        doc = doc or self.doc
> +        if doc:
> +            if self.config_type and self.config_type.is_implicit():
> +                self.config_type.connect_doc(doc)

Brain cramp.

> +
>      def visit(self, visitor):
>          super().visit(visitor)
>          visitor.visit_class(self)
> @@ -1236,6 +1271,11 @@ def _def_exprs(self, exprs):
>              else:
>                  assert False
>  
> +        classes = [c for c in self._entity_list
> +                   if isinstance(c,QAPISchemaClass)]
> +        for c in classes:
> +            self._def_entity(c.get_qom_config_type(self))
> +

Either use a generator expression

           classes = (c for c in self._entity_list if ... )
           for c in classes:

Or do it the old-fashioned way

           for ent in self._entity_list:
               if isinstance(ent, QAPISchemaClass):

>      def check(self):
>          for ent in self._entity_list:
>              ent.check(self)

Re: [RFC PATCH 09/12] qapi/qom: Convert rng-backend/random to class

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  qapi/qom.json | 22 ++++++++++++++++------
>  1 file changed, 16 insertions(+), 6 deletions(-)
>
> diff --git a/qapi/qom.json b/qapi/qom.json
> index ccd1167808..a167e91f67 100644
> --- a/qapi/qom.json
> +++ b/qapi/qom.json
> @@ -721,6 +721,16 @@
>  { 'struct': 'RngProperties',
>    'data': { '*opened': { 'type': 'bool', 'features': ['deprecated'] } } }
>  
> +##
> +# @rng-backend:
> +#
> +# Base class for random number generator backends
> +#
> +# Since: 1.3
> +##
> +{ 'class': 'rng-backend',
> +  'config': 'RngProperties' }
> +
>  ##
>  # @RngEgdProperties:
>  #
> @@ -736,18 +746,18 @@
>    'data': { 'chardev': 'str' } }
>  
>  ##
> -# @RngRandomProperties:
> +# @rng-random:
>  #
> -# Properties for rng-random objects.
> +# Random number generator backend using a host random number device
>  #
>  # @filename: the filename of the device on the host to obtain entropy from
>  #            (default: "/dev/urandom")
>  #
>  # Since: 1.3
>  ##
> -{ 'struct': 'RngRandomProperties',
> -  'base': 'RngProperties',
> -  'data': { '*filename': 'str' } }
> +{ 'class': 'rng-random',
> +  'parent': 'rng-backend',
> +  'config': { '*filename': 'str' } }
>  
>  ##
>  # @SevGuestProperties:
> @@ -889,7 +899,7 @@
>        'qtest':                      'QtestProperties',
>        'rng-builtin':                'RngProperties',
>        'rng-egd':                    'RngEgdProperties',
> -      'rng-random':                 { 'type': 'RngRandomProperties',
> +      'rng-random':                 { 'type': 'qom-config:rng-random',
>                                        'if': 'CONFIG_POSIX' },
>        'secret':                     'SecretProperties',
>        'secret_keyring':             { 'type': 'SecretKeyringProperties',

This generates struct q_obj_rng_random_config and struct
qom_config_rng_random.  Their names violate coding style.

The former struct appears to be unused.  Hmm, the next patch will use
it.  Okay.

Re: [RFC PATCH 10/12] qapi: Generate QOM config marshalling code

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  backends/rng-random.c | 17 ++------
>  backends/rng.c        | 17 ++------
>  scripts/qapi/main.py  |  2 +
>  scripts/qapi/qom.py   | 91 +++++++++++++++++++++++++++++++++++++++++++
>  qapi/meson.build      |  3 ++
>  5 files changed, 104 insertions(+), 26 deletions(-)
>  create mode 100644 scripts/qapi/qom.py
>
> diff --git a/backends/rng-random.c b/backends/rng-random.c
> index b221308091..35738df3c6 100644
> --- a/backends/rng-random.c
> +++ b/backends/rng-random.c
> @@ -14,6 +14,7 @@
>  #include "sysemu/rng-random.h"
>  #include "sysemu/rng.h"
>  #include "qapi/error.h"
> +#include "qapi/qapi-qom-qom.h"
>  #include "qapi/visitor.h"
>  #include "qapi/qmp/qerror.h"
>  #include "qemu/main-loop.h"
> @@ -90,7 +91,8 @@ static char *rng_random_get_filename(Object *obj, Error **errp)
>      return g_strdup(s->filename);
>  }
>  
> -static bool rng_random_config(Object *obj, const char *filename, Error **errp)
> +bool qom_rng_random_config(Object *obj, bool has_filename,
> +                           const char *filename, Error **errp)
>  {
>      RngRandom *s = RNG_RANDOM(obj);
>  
> @@ -100,17 +102,6 @@ static bool rng_random_config(Object *obj, const char *filename, Error **errp)
>      return true;
>  }
>  
> -static bool rng_random_marshal_config(Object *obj, Visitor *v, Error **errp)
> -{
> -    g_autofree char *filename = NULL;
> -
> -    if (!visit_type_str(v, "filename", &filename, errp)) {
> -        return false;
> -    }
> -
> -    return rng_random_config(obj, filename, errp);
> -}
> -

Generated replacement:

   bool qom_rng_random_marshal_config(Object *obj, Visitor *v, Error **errp)
   {
       q_obj_rng_random_config config = {0};

       if (!visit_type_q_obj_rng_random_config_members(v, &config, errp)) {
           return false;
       }

       return qom_rng_random_config(obj, config.has_filename, config.filename, errp);
   }

where visit_type_q_obj_rng_random_config_members() is

    bool visit_type_q_obj_rng_random_config_members(Visitor *v, q_obj_rng_random_config *obj, Error **errp)
    {
        if (visit_optional(v, "filename", &obj->has_filename)) {
            if (!visit_type_str(v, "filename", &obj->filename, errp)) {
                return false;
            }
        }
        return true;
    }

The handwritten version lacks a visit_optional(), I think.

>  static void rng_random_init(Object *obj)
>  {
>      RngRandom *s = RNG_RANDOM(obj);
> @@ -148,7 +139,7 @@ static const TypeInfo rng_random_info = {
>      .instance_size = sizeof(RngRandom),
>      .class_init = rng_random_class_init,
>      .instance_init = rng_random_init,
> -    .instance_config = rng_random_marshal_config,
> +    .instance_config = qom_rng_random_marshal_config,
>      .instance_finalize = rng_random_finalize,
>  };
>  
> diff --git a/backends/rng.c b/backends/rng.c
> index 840daf0392..d560bd7b5d 100644
> --- a/backends/rng.c
> +++ b/backends/rng.c
> @@ -13,6 +13,7 @@
>  #include "qemu/osdep.h"
>  #include "sysemu/rng.h"
>  #include "qapi/error.h"
> +#include "qapi/qapi-qom-qom.h"
>  #include "qapi/qmp/qerror.h"
>  #include "qemu/module.h"
>  #include "qom/object_interfaces.h"
> @@ -77,24 +78,14 @@ static void rng_backend_complete(UserCreatable *uc, Error **errp)
>      rng_backend_prop_set_opened(OBJECT(uc), true, errp);
>  }
>  
> -static bool rng_backend_config(Object *obj, bool opened, Error **errp)
> +bool qom_rng_backend_config(Object *obj, bool has_opened, bool opened,
> +                            Error **errp)
>  {
>      ERRP_GUARD();
>      rng_backend_prop_set_opened(obj, opened, errp);
>      return *errp == NULL;
>  }
>  
> -static bool rng_backend_marshal_config(Object *obj, Visitor *v, Error **errp)
> -{
> -    bool opened;
> -
> -    if (!visit_type_bool(v, "opened", &opened, errp)) {
> -        return false;
> -    }
> -
> -    return rng_backend_config(obj, opened, errp);
> -}
> -

Generated replacement:

   bool qom_rng_backend_marshal_config(Object *obj, Visitor *v, Error **errp)
   {
       RngProperties config = {0};

       if (!visit_type_RngProperties_members(v, &config, errp)) {
           return false;
       }

       return qom_rng_backend_config(obj, config.has_opened, config.opened, errp);
   }

where visit_type_RngProperties_members() is

   bool visit_type_RngProperties_members(Visitor *v, RngProperties *obj, Error **errp)
   {
       if (visit_optional(v, "opened", &obj->has_opened)) {
           if (visit_policy_reject(v, "opened", 1u << QAPI_DEPRECATED, errp)) {
               return false;
           }
           if (!visit_policy_skip(v, "opened", 1u << QAPI_DEPRECATED)) {
               if (!visit_type_bool(v, "opened", &obj->opened, errp)) {
                   return false;
               }
           }
       }
       return true;
   }

The handwritten version lacks a visit_optional(), and neglects to check
policy.

>  static void rng_backend_free_request(RngRequest *req)
>  {
>      g_free(req->data);
> @@ -148,7 +139,7 @@ static const TypeInfo rng_backend_info = {
>      .parent = TYPE_OBJECT,
>      .instance_size = sizeof(RngBackend),
>      .instance_init = rng_backend_init,
> -    .instance_config = rng_backend_marshal_config,
> +    .instance_config = qom_rng_backend_marshal_config,
>      .instance_finalize = rng_backend_finalize,
>      .class_size = sizeof(RngBackendClass),
>      .class_init = rng_backend_class_init,
> diff --git a/scripts/qapi/main.py b/scripts/qapi/main.py
> index f2ea6e0ce4..b6b4a4a74d 100644
> --- a/scripts/qapi/main.py
> +++ b/scripts/qapi/main.py
> @@ -16,6 +16,7 @@
>  from .error import QAPIError
>  from .events import gen_events
>  from .introspect import gen_introspect
> +from .qom import gen_qom
>  from .schema import QAPISchema
>  from .types import gen_types
>  from .visit import gen_visit
> @@ -52,6 +53,7 @@ def generate(schema_file: str,
>      gen_commands(schema, output_dir, prefix)
>      gen_events(schema, output_dir, prefix)
>      gen_introspect(schema, output_dir, prefix, unmask)
> +    gen_qom(schema, output_dir, prefix)
>  
>  
>  def main() -> int:
> diff --git a/scripts/qapi/qom.py b/scripts/qapi/qom.py
> new file mode 100644
> index 0000000000..1cca94890f
> --- /dev/null
> +++ b/scripts/qapi/qom.py
> @@ -0,0 +1,91 @@
> +"""
> +QAPI QOM boilerplate generator
> +
> +Copyright (c) 2021 Red Hat Inc.
> +
> +Authors:
> + Kevin Wolf <kwolf@redhat.com>
> +
> +This work is licensed under the terms of the GNU GPL, version 2.
> +See the COPYING file in the top-level directory.
> +"""
> +
> +from .common import c_name, mcgen
> +from .gen import (
> +    build_params,
> +    QAPISchemaModularCVisitor,
> +)
> +from .schema import (
> +    QAPISchema,
> +    QAPISchemaClass,
> +)
> +
> +
> +def gen_config_decl(c: QAPISchemaClass) -> str:
> +    params = build_params(c.config_type, c.config_boxed,  'Error **errp')
> +    return mcgen('''
> +bool qom_%(name)s_marshal_config(Object *obj, Visitor *v, Error **errp);
> +bool qom_%(name)s_config(Object *obj, %(params)s);
> +''', name=c.c_name(), params=params)
> +
> +
> +def gen_config_call(c: QAPISchemaClass) -> str:
> +    if c.config_boxed:
> +        argstr = '&config, '
> +    else:
> +        assert not c.config_type.variants
> +        argstr = ''
> +        for m in c.config_type.members:
> +            if m.optional:
> +                argstr += 'config.has_%s, ' % c_name(m.name)
> +            argstr += 'config.%s, ' % c_name(m.name)
> +
> +    return f'qom_{c.c_name()}_config(obj, {argstr}errp)'

Duplicates part of gen_call() from commands.py.  Let's not worry about
that right now.

> +
> +def gen_config(c: QAPISchemaClass) -> str:
> +    return mcgen('''
> +bool qom_%(qom_type)s_marshal_config(Object *obj, Visitor *v, Error **errp)
> +{
> +    %(config_name)s config = {0};
> +
> +    if (!visit_type_%(config_name)s_members(v, &config, errp)) {
> +        return false;
> +    }
> +
> +    return %(call)s;
> +}
> +
> +''', qom_type=c.c_name(), config_name=c.config_type.c_name(),
> +     call=gen_config_call(c))
> +
> +
> +class QAPISchemaGenQOMVisitor(QAPISchemaModularCVisitor):
> +
> +    def __init__(self, prefix: str):
> +        super().__init__(
> +            prefix, 'qapi-qom', ' * Schema-defined QOM types',
> +            None, __doc__)
> +
> +    def _begin_user_module(self, name: str) -> None:
> +        qom = self._module_basename('qapi-qom', name)
> +        types = self._module_basename('qapi-types', name)
> +        visit = self._module_basename('qapi-visit', name)
> +        self._genc.add(mcgen('''
> +#include "qemu/osdep.h"
> +#include "qapi/error.h"
> +#include "%(qom)s.h"
> +#include "%(types)s.h"
> +#include "%(visit)s.h"
> +
> +''', types=types, visit=visit, qom=qom))
> +
> +    def visit_class(self, c: QAPISchemaClass) -> None:
> +        if c.config_type:
> +            self._genh.add(gen_config_decl(c))
> +            self._genc.add(gen_config(c))
> +
> +
> +def gen_qom(schema: QAPISchema, output_dir: str, prefix: str) -> None:
> +    vis = QAPISchemaGenQOMVisitor(prefix)
> +    schema.visit(vis)
> +    vis.write(output_dir)
> diff --git a/qapi/meson.build b/qapi/meson.build
> index c356a385e3..10c412f1ad 100644
> --- a/qapi/meson.build
> +++ b/qapi/meson.build
> @@ -87,6 +87,7 @@ qapi_nonmodule_outputs = [
>    'qapi-init-commands.h', 'qapi-init-commands.c',
>    'qapi-events.h', 'qapi-events.c',
>    'qapi-emit-events.c', 'qapi-emit-events.h',
> +  'qapi-qom.h', 'qapi-qom.c',
>  ]
>  
>  # First build all sources
> @@ -111,6 +112,8 @@ foreach module : qapi_all_modules
>        'qapi-events-@0@.h'.format(module),
>        'qapi-commands-@0@.c'.format(module),
>        'qapi-commands-@0@.h'.format(module),
> +      'qapi-qom-@0@.c'.format(module),
> +      'qapi-qom-@0@.h'.format(module),
>      ]
>    endif
>    if module.endswith('-target')

Re: [RFC PATCH 02/12] qom: Create object_configure()

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> This renames object_set_properties_from_qdict() to object_configure()
> and removes the QDict parameter from it: With visit_next_struct_member()
> it can set all properties without looking at the keys of the QDict.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  qom/object_interfaces.c | 13 ++++++-------
>  1 file changed, 6 insertions(+), 7 deletions(-)
>
> diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c
> index 3b61c195c5..f9f5608194 100644
> --- a/qom/object_interfaces.c
> +++ b/qom/object_interfaces.c
> @@ -42,16 +42,15 @@ bool user_creatable_can_be_deleted(UserCreatable *uc)
>      }
>  }
>  
> -static void object_set_properties_from_qdict(Object *obj, const QDict *qdict,
> -                                             Visitor *v, Error **errp)
> +static void object_configure(Object *obj, Visitor *v, Error **errp)
>  {
> -    const QDictEntry *e;
> +    const char *key;
>  
>      if (!visit_start_struct(v, NULL, NULL, 0, errp)) {
>          return;
>      }
> -    for (e = qdict_first(qdict); e; e = qdict_next(qdict, e)) {
> -        if (!object_property_set(obj, e->key, v, errp)) {
> +    while ((key = visit_next_struct_member(v))) {
> +        if (!object_property_set(obj, key, v, errp)) {
>              goto out;
>          }
>      }
> @@ -69,7 +68,7 @@ void object_set_properties_from_keyval(Object *obj, const QDict *qdict,
>      } else {
>          v = qobject_input_visitor_new_keyval(QOBJECT(qdict));
>      }
> -    object_set_properties_from_qdict(obj, qdict, v, errp);
> +    object_configure(obj, v, errp);
>      visit_free(v);
>  }
>  
> @@ -108,7 +107,7 @@ Object *user_creatable_add_type(const char *type, const char *id,
>  
>      assert(qdict);

This is the only remaining use of parameter @qdict.  Let's drop it.

>      obj = object_new(type);
> -    object_set_properties_from_qdict(obj, qdict, v, &local_err);
> +    object_configure(obj, v, &local_err);
>      if (local_err) {
>          goto out;
>      }

Brief recap how configuration data flows trough QMP object-add to QOM:

* QMP object-add is fully typed: union ObjectOptions.  QMP core parses
  input via QDict to ObjectOptions (JSON parser + QObject input
  visitor), and passes that to qmp_object_add().

* qmp_object_add() passes it on to user_creatable_add_qapi().

  Aside: getting rid of the wrapper would be as easy as renaming
  user_creatable_add_qapi() to qmp_object_add().

* user_creatable_add_qapi() converts right back to QDict (QObject output
  visitor).  It extracts the non-properties "qom-type" and "id", and
  passes the properties (wrapped in a QObject input visitor) to
  user_creatable_add_type().

* user_creatable_add_type() feeds the properties to object_configure(),
  formerly object_set_properties_from_qdict().

Your patch simplifies one detail of the last step.  Small
simplifications are welcome, too.

The visitor ping-pong remains: input -> output -> input.

We play visitor ping-pong because we reduce the problem "for each member
of ObjectOptions: set property" to the solved problem "set properties
for an input visitor".

Straight solution of the problem: a QOM property output visitor.

Observation, not demand.

Re: [RFC PATCH 00/12] QOM/QAPI integration part 1

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> This series adds QOM class definitions to the QAPI schema, introduces
> a new TypeInfo.instance_config() callback that configures the object at
> creation time (instead of setting properties individually) and is
> separate from runtime property setters (which often used to be not
> really tested for runtime use), and finally generates a marshalling
> function for .instance_config() from the QAPI schema that makes this a
> natural C interface rather than a visitor based one.
>
> This is loosely based on Paolo's old proposal in the wiki:
> https://wiki.qemu.org/Features/QOM-QAPI_integration
>
> The series is in a rather early stage and I don't really have any
> automated tests or documentation in this series yet. I'm also only
> converting the class hierarchy for the random number generator backends
> to show what the result looks like, the other objects still need to be
> done.
>
> So the question to you isn't whether this is mergeable (it isn't), but
> whether you think this is the right approach for starting to integrate
> QOM and QAPI better.
>
> You'll also see that this doesn't really remove the duplication between
> property definitions in the code and configuration struct definitions in
> the QAPI schema yet (because we want to keep at least a read-only
> runtime property for every configuration option), but at least they mean
> somewhat different things now (creation vs. runtime) instead of being
> completely redundant.
>
> Possible future steps:
>
> * Define at least those properties to the schema that correspond to a
>   config option. For both setters and getters for each option, we'll
>   probably want to select in the schema between 'not available',
>   'automatically generated function' and 'manually implemented'.
>
>   Other runtime properties could be either left in the code or added to
>   the schema as well. Either way, we need to figure out how to best
>   describe these things in the schema.

Permit me a diversion of sorts.

With QOM, we have properties.  A property is readable if it has a
getter, writable if it has a setter.  There is no real concept of
configuration vs. state.  Writable properties can be written at any
time.

In practice, some properties are to be used only like configuration, and
we check configuration at realize time (for devices), or by a surrogate
like qemu_add_machine_init_done_notifier().  If you set them later,
things may break, and you get to keep the pieces.

In this "QOM/QAPI integration part 1", configuration (expressed in QAPI
schema) makes it into QOM.

Now we have configuration *and* properties.

Do we need the properties?

Note I'm not asking whether we need setters.  I'm asking whether we need
to expose configuration bits via qom-set & friends in addition to the
QAPI schema and query-qmp-schema.

> * Getting rid of the big 'object-add' union: While the union is not too
>   bad for the rather small number of user-creatable objects, it
>   wouldn't scale at all for devices.
>
>   My idea there is that we could define something like this:
>
>   { 'struct': 'ObjectOptions',
>     'data': {
>         'id': 'str',
>         'config': { 'type': 'qom-config-any:user-creatable',
>                     'embed': true } } }
>
>   Obviously this would be an extension of the schema language to add an
>   'embed' option (another hopefully more acceptable attempt to flatten
>   things...), so I'd like to hear opinions on this first before I go to
>   implement it.

'embed': true would splice in the members of a struct type instead of a
single member of that struct type.  Correct?

Stretch goal: make it work for union types, too :)

I've thought of this before.  Plenty of nesting in the wire format
exists pretty much only to let us have the C structs we want.  Right
now, the only way to "splice in" such a struct is the base type.
General splicing could be useful.  It may take an introspection flag
day.

>   Also note that 'qom-config-any:user-creatable' is new, too. The
>   'qom-config:...' types introduced by this series don't work for
>   subclasses, but only for the exact class.
>
>   On the external interface, the new 'qom-config-any:...' type including
>   subclasses would basically behave (and be introspected) like the union
>   we have today, just without being defined explicitly.

I'm not sure I follow.  How is the qom-config-any:user-creatable to be
defined?  QAPI collects all the qom-config:* types into a union
automatically?

>   As for the C representation for configurations that include
>   subclasses, I'm imagining a struct that just contains the qom_type
>   string (so we can call the right handler) and a pointer to the real
>   config (that is treated as opaque by the generic code).

Now you lost me.

> I could probably add more, but let's just start with this for discussion
> now.

I wish we could fill a whiteboard together...

Re: [RFC PATCH 10/12] qapi: Generate QOM config marshalling code

[Kevin Wolf]

Am 23.11.2021 um 15:16 hat Markus Armbruster geschrieben:
> Kevin Wolf <kwolf@redhat.com> writes:
> 
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >  backends/rng-random.c | 17 ++------
> >  backends/rng.c        | 17 ++------
> >  scripts/qapi/main.py  |  2 +
> >  scripts/qapi/qom.py   | 91 +++++++++++++++++++++++++++++++++++++++++++
> >  qapi/meson.build      |  3 ++
> >  5 files changed, 104 insertions(+), 26 deletions(-)
> >  create mode 100644 scripts/qapi/qom.py
> >
> > diff --git a/backends/rng-random.c b/backends/rng-random.c
> > index b221308091..35738df3c6 100644
> > --- a/backends/rng-random.c
> > +++ b/backends/rng-random.c
> > @@ -14,6 +14,7 @@
> >  #include "sysemu/rng-random.h"
> >  #include "sysemu/rng.h"
> >  #include "qapi/error.h"
> > +#include "qapi/qapi-qom-qom.h"
> >  #include "qapi/visitor.h"
> >  #include "qapi/qmp/qerror.h"
> >  #include "qemu/main-loop.h"
> > @@ -90,7 +91,8 @@ static char *rng_random_get_filename(Object *obj, Error **errp)
> >      return g_strdup(s->filename);
> >  }
> >  
> > -static bool rng_random_config(Object *obj, const char *filename, Error **errp)
> > +bool qom_rng_random_config(Object *obj, bool has_filename,
> > +                           const char *filename, Error **errp)
> >  {
> >      RngRandom *s = RNG_RANDOM(obj);
> >  
> > @@ -100,17 +102,6 @@ static bool rng_random_config(Object *obj, const char *filename, Error **errp)
> >      return true;
> >  }
> >  
> > -static bool rng_random_marshal_config(Object *obj, Visitor *v, Error **errp)
> > -{
> > -    g_autofree char *filename = NULL;
> > -
> > -    if (!visit_type_str(v, "filename", &filename, errp)) {
> > -        return false;
> > -    }
> > -
> > -    return rng_random_config(obj, filename, errp);
> > -}
> > -
> 
> Generated replacement:
> 
>    bool qom_rng_random_marshal_config(Object *obj, Visitor *v, Error **errp)
>    {
>        q_obj_rng_random_config config = {0};
> 
>        if (!visit_type_q_obj_rng_random_config_members(v, &config, errp)) {
>            return false;
>        }
> 
>        return qom_rng_random_config(obj, config.has_filename, config.filename, errp);
>    }
> 
> where visit_type_q_obj_rng_random_config_members() is
> 
>     bool visit_type_q_obj_rng_random_config_members(Visitor *v, q_obj_rng_random_config *obj, Error **errp)
>     {
>         if (visit_optional(v, "filename", &obj->has_filename)) {
>             if (!visit_type_str(v, "filename", &obj->filename, errp)) {
>                 return false;
>             }
>         }
>         return true;
>     }
> 
> The handwritten version lacks a visit_optional(), I think.

Yes, this looks like a bug in the handwritten version from patch 5.

The generated version seems to have a bug, too: It doesn't free the
members of @config after calling qom_rng_random_config().

Kevin

Re: [RFC PATCH 08/12] qapi: Create qom-config:... type for classes

[Kevin Wolf]

Am 23.11.2021 um 14:00 hat Markus Armbruster geschrieben:
> Kevin Wolf <kwolf@redhat.com> writes:
> 
> > For every class that has a 'config' definition, a corresponding
> > 'qom-config:$QOM_TYPE' type is automatically created that contains the
> > configuration for the class and all of its parent classes.
> >
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> 
> I assume $QOM_TYPE stands for the class's name.
> 
> What kind of type does this define?  Peeking at the code below... looks
> like it's a struct type.
> 
> I wonder how it's related to the the type we already use or create for
> the the class's 'config' member.  Which is either the name of a struct
> or union type to use, or a dictionary that tells us what struct type to
> create.  Let's see below.

The members of 'config' (i.e. the local configuration options of the
class) are a subset of the member of this new type (i.e. the
configuration options of this class and all of its parent classes).

The new type is really just needed internally in the generated QAPI code
so that the full input can be stored in a C struct temporarily. All of
the manually written code only uses the 'config' type.

> > ---
> >  scripts/qapi/schema.py | 60 +++++++++++++++++++++++++++++++++++-------
> >  1 file changed, 50 insertions(+), 10 deletions(-)
> >
> > diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
> > index ebf69341d7..79db42b810 100644
> > --- a/scripts/qapi/schema.py
> > +++ b/scripts/qapi/schema.py
> > @@ -761,6 +761,13 @@ def connect_doc(self, doc):
> >              for f in self.features:
> >                  doc.connect_feature(f)
> >  
> > +    def clone(self):
> > +        features = [QAPISchemaFeature(f.name, f.info, f.ifcond)
> > +                    for f in self.features]
> > +        return QAPISchemaObjectTypeMember(
> > +            self.name, self.info, self._type_name, self.optional,
> > +            self.ifcond, features)
> > +
> 
> A copy that's somewhere between shallow and deep.  Reserving judgement.
> 
> >  
> >  class QAPISchemaVariant(QAPISchemaObjectTypeMember):
> >      role = 'branch'
> > @@ -783,17 +790,11 @@ def __init__(self, name, info, doc, ifcond, features, parent,
> >          self._config_type_name = config_type
> >          self.config_type = None
> >          self.config_boxed = config_boxed
> > +        self.full_config_type = None
> >  
> > -    def check(self, schema):
> > -        super().check(schema)
> > -
> > -        if self._parent_name:
> > -            self.parent = schema.lookup_entity(self._parent_name,
> > -                                               QAPISchemaClass)
> > -            if not self.parent:
> > -                raise QAPISemError(
> > -                    self.info,
> > -                    "Unknown parent class '%s'" % self._parent_name)
> > +    def get_qom_config_type(self, schema):
> > +        if self.full_config_type:
> > +            return self.full_config_type
> >  
> >          if self._config_type_name:
> >              self.config_type = schema.resolve_type(
> > @@ -809,6 +810,40 @@ def check(self, schema):
> >                      "class 'config' can take %s only with 'boxed': true"
> >                      % self.config_type.describe())
> >  
> > +            # FIXME That's a bit ugly
> > +            self.config_type.check(schema)
> > +            members = [m.clone() for m in self.config_type.members]
> > +        else:
> > +            members = []
> 
> Have you considered defaulting ._config_type_name to 'q_empty'?

No. Sounds like a minor simplification here because the if condition
would always become true and therefore the code can be unconditional.

The more important point to talk about in this context is the FIXME
comment. We need to define the right order in which things should be
done.

Before this series, we have a split into two phases: First process all
type definitions in the schema and create the objects representing them,
and then check whether all of the created objects are valid. Among
others, these two phases are needed because we allow definitions to
refer to types that are defined only later.

I'm trying to insert a third phase in the middle that creates new
implicit types based on the schema definitions.

The thing that makes trouble here is that .check() doesn't really only
check, but it's in fact the thing that finalises the initialisation of
object types. Before it, we don't have a member list, but we need it in
order to create the derived type. So I ended up calling .check() for
some types earlier than it should be called, basically intertwining
these two phases.

This can probably fail when .check() tries to access an implicit type
that hasn't been created yet. Not sure if this is a practically relevant
limitation, but mixing the phases this way is definitely ugly.

Any ideas how to solve this cleanly?

> > +
> > +        if self._parent_name:
> > +            self.parent = schema.lookup_entity(self._parent_name,
> > +                                               QAPISchemaClass)
> > +            if not self.parent:
> > +                raise QAPISemError(
> > +                    self.info,
> > +                    "Unknown parent class '%s'" % self._parent_name)
> > +
> > +            self.parent.get_qom_config_type(schema)
> > +            members += [m.clone() for m in self.parent.config_type.members]
> 
> @members is the list of common members of the class's and all its
> parents' 'config' types.
> 
> Any variant members of 'config' types get silently ignored.  Should we
> reject them instead?

We (try to) allow them for boxed 'config' types currently. I think this
was only because commands do the same, but I don't see any reason why
variants would be needed at all for 'config'. So we can just reject them
for both boxed and non-boxed configs.

> > +
> > +        self.full_config_type = QAPISchemaObjectType(
> > +            f"qom-config:{self.name}", self.info, None, self._ifcond,
> > +            self.features, None, members, None)
> 
> The "full config type" inherits conditional and features from the class.
> Its (common) members are the members of the class's and all its parents'
> 'config' types.
> 
> Could we use the parent's full config type as the base type here?

Hmm, good question. I guess that could work.

> Do we need the non-full config type (the type named by or implicitly
> defined by the 'config' member') for anything other than a source of
> local members for the full config type?

Yes, this is the practically relevant type in manually written C code.
The implementation of .instance_config in a QOM object has no use for
all the properties that are really meant for its parent class. It might
even commonly include the local config type in its runtime state, and
having options of the parent class duplicated both in the child class
and in the parent class would be a sure way to introduce bugs.

This is the reason why the non-full config type has a nice C type name
when using an explicit type for 'config', whereas the full config type
is a QAPI implementation detail that can have an ugly name in C without
bothering anyone.

> > +
> > +        return self.full_config_type
> > +
> > +    def check(self, schema):
> > +        super().check(schema)
> > +        assert self.full_config_type
> 
> New constraint: must call .get_qom_config_type() before .check().
> 
> This side effect makes me unsure sure about the "get" in the name.
> Let's worry about that later.

Aye, related to the (theoretical) three phases mentioned above.

> > +
> > +    def connect_doc(self, doc=None):
> > +        super().connect_doc(doc)
> > +        doc = doc or self.doc
> > +        if doc:
> > +            if self.config_type and self.config_type.is_implicit():
> > +                self.config_type.connect_doc(doc)
> 
> Brain cramp.

Apart from the attribute name for the config type, it's an exact copy
from QAPISchemaCommand.

Kevin

Re: [RFC PATCH 07/12] qapi: Allow defining QOM classes

[Kevin Wolf]

Am 23.11.2021 um 11:02 hat Markus Armbruster geschrieben:
> Kevin Wolf <kwolf@redhat.com> writes:
> 
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >  scripts/qapi/expr.py   | 28 +++++++++++++++++-
> >  scripts/qapi/schema.py | 66 ++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 93 insertions(+), 1 deletion(-)
> 
> Missing: docs/devel/qapi-code-gen.rst update.  I understand why, but it
> does make review harder.
> 
> >
> > diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
> > index 3cb389e875..77550629f3 100644
> > --- a/scripts/qapi/expr.py
> > +++ b/scripts/qapi/expr.py
> > @@ -181,6 +181,8 @@ def check_defn_name_str(name: str, info: QAPISourceInfo, meta: str) -> None:
> >      """
> >      if meta == 'event':
> >          check_name_upper(name, info, meta)
> > +    elif meta == 'class':
> > +        check_name_str(name, info, meta)
> 
> This permits arbitrary QAPI names.  I figure we'll want to define and
> enforce a suitable naming convention.

It's the QOM type name. So if you want to enforce something stricter, it
needs to be defined for QOM first. I seem to remember that arbitrary
QAPI names is already stricter than arbitrary QOM names, but I was
hoping that it is close enough to what QOM classes actually use in
practice. I fully expect to have a list of exceptions at some point when
converting devices.

> >      elif meta == 'command':
> >          check_name_lower(
> >              name, info, meta,
> > @@ -557,6 +559,24 @@ def check_alternate(expr: _JSONObject, info: QAPISourceInfo) -> None:
> >          check_type(value['type'], info, source)
> >  
> >  
> > +def check_class(expr: _JSONObject, info: QAPISourceInfo) -> None:
> > +    """
> > +    Normalize and validate this expression as a ``class`` definition.
> > +
> > +    :param expr: The expression to validate.
> > +    :param info: QAPI schema source file information.
> > +
> > +    :raise QAPISemError: When ``expr`` is not a valid ``class``.
> > +    :return: None, ``expr`` is normalized in-place as needed.
> > +    """
> > +    config = expr.get('config')
> > +    config_boxed = expr.get('config-boxed', False)
> > +
> > +    if config_boxed and config is None:
> > +        raise QAPISemError(info, "'boxed': true requires 'config'")
> 
> You check 'config-boxed', but the error message talks about 'boxed'.
> 
> > +    check_type(config, info, "'config'", allow_dict=not config_boxed)
> > +
> > +
> >  def check_command(expr: _JSONObject, info: QAPISourceInfo) -> None:
> >      """
> >      Normalize and validate this expression as a ``command`` definition.
> > @@ -627,7 +647,7 @@ def check_exprs(exprs: List[_JSONObject]) -> List[_JSONObject]:
> >              continue
> >  
> >          metas = expr.keys() & {'enum', 'struct', 'union', 'alternate',
> > -                               'command', 'event'}
> > +                               'class', 'command', 'event'}
> >          if len(metas) != 1:
> >              raise QAPISemError(
> >                  info,
> > @@ -671,6 +691,12 @@ def check_exprs(exprs: List[_JSONObject]) -> List[_JSONObject]:
> >                         ['struct', 'data'], ['base', 'if', 'features'])
> >              normalize_members(expr['data'])
> >              check_struct(expr, info)
> > +        elif meta == 'class':
> > +            check_keys(expr, info, meta,
> > +                       ['class'], ['if', 'features', 'parent', 'config',
> > +                        'config-boxed'])
> > +            normalize_members(expr.get('config'))
> > +            check_class(expr, info)
> >          elif meta == 'command':
> >              check_keys(expr, info, meta,
> >                         ['command'],
> > diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
> > index b7b3fc0ce4..ebf69341d7 100644
> > --- a/scripts/qapi/schema.py
> > +++ b/scripts/qapi/schema.py
> > @@ -155,6 +155,9 @@ def visit_object_type_flat(self, name, info, ifcond, features,
> >      def visit_alternate_type(self, name, info, ifcond, features, variants):
> >          pass
> >  
> > +    def visit_class(self, entity):
> > +        pass
> > +
> >      def visit_command(self, name, info, ifcond, features,
> >                        arg_type, ret_type, gen, success_response, boxed,
> >                        allow_oob, allow_preconfig, coroutine):
> > @@ -766,6 +769,50 @@ def __init__(self, name, info, typ, ifcond=None):
> >          super().__init__(name, info, typ, False, ifcond)
> >  
> >  
> > +class QAPISchemaClass(QAPISchemaEntity):
> > +    meta = 'class'
> > +
> > +    def __init__(self, name, info, doc, ifcond, features, parent,
> > +                 config_type, config_boxed):
> > +        super().__init__(name, info, doc, ifcond, features)
> > +
> > +        assert not parent or isinstance(parent, str)
> 
> I can't see what ensures this.

Indeed, check_class() fails to check this. You agree that it is the
place where it should be checked?

> > +        assert not config_type or isinstance(config_type, str)
> > +        self._parent_name = parent
> > +        self.parent = None
> > +        self._config_type_name = config_type
> > +        self.config_type = None
> > +        self.config_boxed = config_boxed
> > +
> > +    def check(self, schema):
> > +        super().check(schema)
> > +
> > +        if self._parent_name:
> > +            self.parent = schema.lookup_entity(self._parent_name,
> > +                                               QAPISchemaClass)
> > +            if not self.parent:
> > +                raise QAPISemError(
> > +                    self.info,
> > +                    "Unknown parent class '%s'" % self._parent_name)
> > +
> > +        if self._config_type_name:
> > +            self.config_type = schema.resolve_type(
> > +                self._config_type_name, self.info, "class 'config'")
> 
> "class's 'config'" for consistency with other uses of .resolve_type().
> 
> > +            if not isinstance(self.config_type, QAPISchemaObjectType):
> > +                raise QAPISemError(
> > +                    self.info,
> > +                    "class 'config' cannot take %s"
> > +                    % self.config_type.describe())
> > +            if self.config_type.variants and not self.boxed:
> 
> self.config_boxed?
> 
> > +                raise QAPISemError(
> > +                    self.info,
> > +                    "class 'config' can take %s only with 'boxed': true"
> 
> 'config-boxed'?

Yes, I renamed it and of course forgot some instances. Can't wait for
the patches that would make mypy catch at least 'self.boxed'.

> > +                    % self.config_type.describe())
> > +
> > +    def visit(self, visitor):
> > +        super().visit(visitor)
> > +        visitor.visit_class(self)
> > +
> >  class QAPISchemaCommand(QAPISchemaEntity):
> >      meta = 'command'
> >  
> > @@ -1110,6 +1157,23 @@ def _def_alternate_type(self, expr, info, doc):
> >                                      QAPISchemaVariants(
> >                                          None, info, tag_member, variants)))
> >  
> > +    def _def_class(self, expr, info, doc):
> > +        name = expr['class']
> > +        ifcond = QAPISchemaIfCond(expr.get('if'))
> > +        features = self._make_features(expr.get('features'), info)
> > +        parent = expr.get('parent')
> > +        config_type = expr.get('config')
> > +        config_boxed = expr.get('config-boxed')
> > +
> > +        if isinstance(config_type, OrderedDict):
> > +            config_type = self._make_implicit_object_type(
> > +                name, info, ifcond,
> > +                'config', self._make_members(config_type, info))
> 
> Does QAPISchemaMember.describe() need an update for this?

Looks like it.

Kevin

Re: [RFC PATCH 09/12] qapi/qom: Convert rng-backend/random to class

[Kevin Wolf]

Am 23.11.2021 um 14:15 hat Markus Armbruster geschrieben:
> Kevin Wolf <kwolf@redhat.com> writes:
> 
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >  qapi/qom.json | 22 ++++++++++++++++------
> >  1 file changed, 16 insertions(+), 6 deletions(-)
> >
> > diff --git a/qapi/qom.json b/qapi/qom.json
> > index ccd1167808..a167e91f67 100644
> > --- a/qapi/qom.json
> > +++ b/qapi/qom.json
> > @@ -721,6 +721,16 @@
> >  { 'struct': 'RngProperties',
> >    'data': { '*opened': { 'type': 'bool', 'features': ['deprecated'] } } }
> >  
> > +##
> > +# @rng-backend:
> > +#
> > +# Base class for random number generator backends
> > +#
> > +# Since: 1.3
> > +##
> > +{ 'class': 'rng-backend',
> > +  'config': 'RngProperties' }
> > +
> >  ##
> >  # @RngEgdProperties:
> >  #
> > @@ -736,18 +746,18 @@
> >    'data': { 'chardev': 'str' } }
> >  
> >  ##
> > -# @RngRandomProperties:
> > +# @rng-random:
> >  #
> > -# Properties for rng-random objects.
> > +# Random number generator backend using a host random number device
> >  #
> >  # @filename: the filename of the device on the host to obtain entropy from
> >  #            (default: "/dev/urandom")
> >  #
> >  # Since: 1.3
> >  ##
> > -{ 'struct': 'RngRandomProperties',
> > -  'base': 'RngProperties',
> > -  'data': { '*filename': 'str' } }
> > +{ 'class': 'rng-random',
> > +  'parent': 'rng-backend',
> > +  'config': { '*filename': 'str' } }
> >  
> >  ##
> >  # @SevGuestProperties:
> > @@ -889,7 +899,7 @@
> >        'qtest':                      'QtestProperties',
> >        'rng-builtin':                'RngProperties',
> >        'rng-egd':                    'RngEgdProperties',
> > -      'rng-random':                 { 'type': 'RngRandomProperties',
> > +      'rng-random':                 { 'type': 'qom-config:rng-random',
> >                                        'if': 'CONFIG_POSIX' },
> >        'secret':                     'SecretProperties',
> >        'secret_keyring':             { 'type': 'SecretKeyringProperties',
> 
> This generates struct q_obj_rng_random_config and struct
> qom_config_rng_random.  Their names violate coding style.
> 
> The former struct appears to be unused.  Hmm, the next patch will use
> it.  Okay.

They are just for internal use of QAPI generated code. QMP command
marshallers are precedence for this.

Kevin

Re: [RFC PATCH 02/12] qom: Create object_configure()

[Kevin Wolf]

Am 23.11.2021 um 16:23 hat Markus Armbruster geschrieben:
> Kevin Wolf <kwolf@redhat.com> writes:
> 
> > This renames object_set_properties_from_qdict() to object_configure()
> > and removes the QDict parameter from it: With visit_next_struct_member()
> > it can set all properties without looking at the keys of the QDict.
> >
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >  qom/object_interfaces.c | 13 ++++++-------
> >  1 file changed, 6 insertions(+), 7 deletions(-)
> >
> > diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c
> > index 3b61c195c5..f9f5608194 100644
> > --- a/qom/object_interfaces.c
> > +++ b/qom/object_interfaces.c
> > @@ -42,16 +42,15 @@ bool user_creatable_can_be_deleted(UserCreatable *uc)
> >      }
> >  }
> >  
> > -static void object_set_properties_from_qdict(Object *obj, const QDict *qdict,
> > -                                             Visitor *v, Error **errp)
> > +static void object_configure(Object *obj, Visitor *v, Error **errp)
> >  {
> > -    const QDictEntry *e;
> > +    const char *key;
> >  
> >      if (!visit_start_struct(v, NULL, NULL, 0, errp)) {
> >          return;
> >      }
> > -    for (e = qdict_first(qdict); e; e = qdict_next(qdict, e)) {
> > -        if (!object_property_set(obj, e->key, v, errp)) {
> > +    while ((key = visit_next_struct_member(v))) {
> > +        if (!object_property_set(obj, key, v, errp)) {
> >              goto out;
> >          }
> >      }
> > @@ -69,7 +68,7 @@ void object_set_properties_from_keyval(Object *obj, const QDict *qdict,
> >      } else {
> >          v = qobject_input_visitor_new_keyval(QOBJECT(qdict));
> >      }
> > -    object_set_properties_from_qdict(obj, qdict, v, errp);
> > +    object_configure(obj, v, errp);
> >      visit_free(v);
> >  }
> >  
> > @@ -108,7 +107,7 @@ Object *user_creatable_add_type(const char *type, const char *id,
> >  
> >      assert(qdict);
> 
> This is the only remaining use of parameter @qdict.  Let's drop it.

Indeed, very good. I've never liked this interface that needs a QDict in
addition to the visitor.

> >      obj = object_new(type);
> > -    object_set_properties_from_qdict(obj, qdict, v, &local_err);
> > +    object_configure(obj, v, &local_err);
> >      if (local_err) {
> >          goto out;
> >      }
> 
> Brief recap how configuration data flows trough QMP object-add to QOM:
> 
> * QMP object-add is fully typed: union ObjectOptions.  QMP core parses
>   input via QDict to ObjectOptions (JSON parser + QObject input
>   visitor), and passes that to qmp_object_add().
> 
> * qmp_object_add() passes it on to user_creatable_add_qapi().
> 
>   Aside: getting rid of the wrapper would be as easy as renaming
>   user_creatable_add_qapi() to qmp_object_add().
> 
> * user_creatable_add_qapi() converts right back to QDict (QObject output
>   visitor).  It extracts the non-properties "qom-type" and "id", and
>   passes the properties (wrapped in a QObject input visitor) to
>   user_creatable_add_type().
> 
> * user_creatable_add_type() feeds the properties to object_configure(),
>   formerly object_set_properties_from_qdict().
> 
> Your patch simplifies one detail of the last step.  Small
> simplifications are welcome, too.
> 
> The visitor ping-pong remains: input -> output -> input.
> 
> We play visitor ping-pong because we reduce the problem "for each member
> of ObjectOptions: set property" to the solved problem "set properties
> for an input visitor".
> 
> Straight solution of the problem: a QOM property output visitor.

I'm not sure how that should work? You can't drive a visit from both
sides and QOM properties already take an input visitor. The only way I
see to make this work is converting to QObjects and creating input
visitors internally in the QOM property output visitor, which would kind
of defeat the purpose.

What could in theory be done is passing a Visitor to qmp_object_add()
instead of ObjectOptions (in the long run, we probably don't want a big
union of all existing QOM types anyway). But if directly pass this on to
QOM, we lose all of the QAPI validation, which we don't want either.

Kevin

Re: [RFC PATCH 00/12] QOM/QAPI integration part 1

[Kevin Wolf]

Am 23.11.2021 um 17:05 hat Markus Armbruster geschrieben:
> Kevin Wolf <kwolf@redhat.com> writes:
> 
> > This series adds QOM class definitions to the QAPI schema, introduces
> > a new TypeInfo.instance_config() callback that configures the object at
> > creation time (instead of setting properties individually) and is
> > separate from runtime property setters (which often used to be not
> > really tested for runtime use), and finally generates a marshalling
> > function for .instance_config() from the QAPI schema that makes this a
> > natural C interface rather than a visitor based one.
> >
> > This is loosely based on Paolo's old proposal in the wiki:
> > https://wiki.qemu.org/Features/QOM-QAPI_integration
> >
> > The series is in a rather early stage and I don't really have any
> > automated tests or documentation in this series yet. I'm also only
> > converting the class hierarchy for the random number generator backends
> > to show what the result looks like, the other objects still need to be
> > done.
> >
> > So the question to you isn't whether this is mergeable (it isn't), but
> > whether you think this is the right approach for starting to integrate
> > QOM and QAPI better.
> >
> > You'll also see that this doesn't really remove the duplication between
> > property definitions in the code and configuration struct definitions in
> > the QAPI schema yet (because we want to keep at least a read-only
> > runtime property for every configuration option), but at least they mean
> > somewhat different things now (creation vs. runtime) instead of being
> > completely redundant.
> >
> > Possible future steps:
> >
> > * Define at least those properties to the schema that correspond to a
> >   config option. For both setters and getters for each option, we'll
> >   probably want to select in the schema between 'not available',
> >   'automatically generated function' and 'manually implemented'.
> >
> >   Other runtime properties could be either left in the code or added to
> >   the schema as well. Either way, we need to figure out how to best
> >   describe these things in the schema.
> 
> Permit me a diversion of sorts.
> 
> With QOM, we have properties.  A property is readable if it has a
> getter, writable if it has a setter.  There is no real concept of
> configuration vs. state.  Writable properties can be written at any
> time.
> 
> In practice, some properties are to be used only like configuration, and
> we check configuration at realize time (for devices), or by a surrogate
> like qemu_add_machine_init_done_notifier().  If you set them later,
> things may break, and you get to keep the pieces.
> 
> In this "QOM/QAPI integration part 1", configuration (expressed in QAPI
> schema) makes it into QOM.
> 
> Now we have configuration *and* properties.
> 
> Do we need the properties?

Configuration is for creating objects, properties are for runtime after
the creation. So for the practical answer, as long as you can find a QOM
type that wants to allow either changing an option at runtime or just
exposing its current value, I would say, yes, we need both. And I can
easily list some QOM types that do.

The theoretical answer is that of course you can replace properties with
custom query-* and set-* QMP commands, but that's not only hardly an
improvment, but also a compatibility problem.

The approach I'm taking here with QAPIfication of objects (and planning
to take for future conversions) is to drop setters that can't work at
runtime (which might be the majority of properties), but keep properties
around otherwise. Everything else would be a per-object decision, not
part of the infrastructure work.

> Note I'm not asking whether we need setters.  I'm asking whether we
> need to expose configuration bits via qom-set & friends in addition to
> the QAPI schema and query-qmp-schema.

I'm not sure I follow here. How is querying or changing option values
redundant with querying which options exist?

Maybe qom-list could become obsolete if we move all properties (and not
just the configuration) into the QAPI schema, but I don't see qom-get
and qom-set going away.

> > * Getting rid of the big 'object-add' union: While the union is not too
> >   bad for the rather small number of user-creatable objects, it
> >   wouldn't scale at all for devices.
> >
> >   My idea there is that we could define something like this:
> >
> >   { 'struct': 'ObjectOptions',
> >     'data': {
> >         'id': 'str',
> >         'config': { 'type': 'qom-config-any:user-creatable',
> >                     'embed': true } } }
> >
> >   Obviously this would be an extension of the schema language to add an
> >   'embed' option (another hopefully more acceptable attempt to flatten
> >   things...), so I'd like to hear opinions on this first before I go to
> >   implement it.
> 
> 'embed': true would splice in the members of a struct type instead of a
> single member of that struct type.  Correct?
> 
> Stretch goal: make it work for union types, too :)
> 
> I've thought of this before.  Plenty of nesting in the wire format
> exists pretty much only to let us have the C structs we want.  Right
> now, the only way to "splice in" such a struct is the base type.
> General splicing could be useful.  It may take an introspection flag
> day.

Base types aren't visible in the introspection either, so probably not
if you continue to just report the resulting structure?

> >   Also note that 'qom-config-any:user-creatable' is new, too. The
> >   'qom-config:...' types introduced by this series don't work for
> >   subclasses, but only for the exact class.
> >
> >   On the external interface, the new 'qom-config-any:...' type including
> >   subclasses would basically behave (and be introspected) like the union
> >   we have today, just without being defined explicitly.
> 
> I'm not sure I follow.  How is the qom-config-any:user-creatable to be
> defined?  QAPI collects all the qom-config:* types into a union
> automatically?

All classes that inherit from user-creatable, but yes, automatically
collected.

For user-creatable, we can either introduce interfaces in QAPI, too, or
we just pretend it's actually the top-level parent class.

Kevin

Re: [RFC PATCH 00/12] QOM/QAPI integration part 1

[Peter Maydell]

On Tue, 14 Dec 2021 at 10:26, Kevin Wolf <kwolf@redhat.com> wrote:
> Configuration is for creating objects, properties are for runtime after
> the creation.

Well, yes and no. In a few places we have some properties which
are morally speaking configuration stuff, but which are runtime
settable. This happens because the property needs to be set after
the device is realized but before the machine is run, and we
don't have a concept of "settable only during the machine creation
phase", only of "settable only before realize". (I can't find an
example of this in the codebase offhand, but I definitely have one
in a patchset I'm working on -- the code which needs to
configure the property of the configured object is far removed
in both location in the codebase and point at which it runs
from the code which is doing the initialize-and-realize.)

-- PMM

Re: [RFC PATCH 00/12] QOM/QAPI integration part 1

[Kevin Wolf]

Am 14.12.2021 um 11:40 hat Peter Maydell geschrieben:
> On Tue, 14 Dec 2021 at 10:26, Kevin Wolf <kwolf@redhat.com> wrote:
> > Configuration is for creating objects, properties are for runtime after
> > the creation.
> 
> Well, yes and no. In a few places we have some properties which
> are morally speaking configuration stuff, but which are runtime
> settable. This happens because the property needs to be set after
> the device is realized but before the machine is run, and we
> don't have a concept of "settable only during the machine creation
> phase", only of "settable only before realize". (I can't find an
> example of this in the codebase offhand, but I definitely have one
> in a patchset I'm working on -- the code which needs to
> configure the property of the configured object is far removed
> in both location in the codebase and point at which it runs
> from the code which is doing the initialize-and-realize.)

Yes, that's fair, but for the infrastructure it doesn't matter much what
something is "morally speaking". These things use properties at
"runtime" (i.e. after realize) today and will keep using them until we
find a different solution. I have no intention to change anything about
it in the context of QAPIfication. The only liberty I'm taking is
removing setters that can't work after realize.

Kevin

Re: [RFC PATCH 00/12] QOM/QAPI integration part 1

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 23.11.2021 um 17:05 hat Markus Armbruster geschrieben:
>> Kevin Wolf <kwolf@redhat.com> writes:
>> 
>> > This series adds QOM class definitions to the QAPI schema, introduces
>> > a new TypeInfo.instance_config() callback that configures the object at
>> > creation time (instead of setting properties individually) and is
>> > separate from runtime property setters (which often used to be not
>> > really tested for runtime use), and finally generates a marshalling
>> > function for .instance_config() from the QAPI schema that makes this a
>> > natural C interface rather than a visitor based one.
>> >
>> > This is loosely based on Paolo's old proposal in the wiki:
>> > https://wiki.qemu.org/Features/QOM-QAPI_integration
>> >
>> > The series is in a rather early stage and I don't really have any
>> > automated tests or documentation in this series yet. I'm also only
>> > converting the class hierarchy for the random number generator backends
>> > to show what the result looks like, the other objects still need to be
>> > done.
>> >
>> > So the question to you isn't whether this is mergeable (it isn't), but
>> > whether you think this is the right approach for starting to integrate
>> > QOM and QAPI better.
>> >
>> > You'll also see that this doesn't really remove the duplication between
>> > property definitions in the code and configuration struct definitions in
>> > the QAPI schema yet (because we want to keep at least a read-only
>> > runtime property for every configuration option), but at least they mean
>> > somewhat different things now (creation vs. runtime) instead of being
>> > completely redundant.
>> >
>> > Possible future steps:
>> >
>> > * Define at least those properties to the schema that correspond to a
>> >   config option. For both setters and getters for each option, we'll
>> >   probably want to select in the schema between 'not available',
>> >   'automatically generated function' and 'manually implemented'.
>> >
>> >   Other runtime properties could be either left in the code or added to
>> >   the schema as well. Either way, we need to figure out how to best
>> >   describe these things in the schema.
>> 
>> Permit me a diversion of sorts.
>> 
>> With QOM, we have properties.  A property is readable if it has a
>> getter, writable if it has a setter.  There is no real concept of
>> configuration vs. state.  Writable properties can be written at any
>> time.
>> 
>> In practice, some properties are to be used only like configuration, and
>> we check configuration at realize time (for devices), or by a surrogate
>> like qemu_add_machine_init_done_notifier().  If you set them later,
>> things may break, and you get to keep the pieces.
>> 
>> In this "QOM/QAPI integration part 1", configuration (expressed in QAPI
>> schema) makes it into QOM.
>> 
>> Now we have configuration *and* properties.
>> 
>> Do we need the properties?
>
> Configuration is for creating objects, properties are for runtime after
> the creation. So for the practical answer, as long as you can find a QOM
> type that wants to allow either changing an option at runtime or just
> exposing its current value, I would say, yes, we need both. And I can
> easily list some QOM types that do.
>
> The theoretical answer is that of course you can replace properties with
> custom query-* and set-* QMP commands, but that's not only hardly an
> improvment, but also a compatibility problem.

That would be nuts.

> The approach I'm taking here with QAPIfication of objects (and planning
> to take for future conversions) is to drop setters that can't work at
> runtime (which might be the majority of properties), but keep properties
> around otherwise. Everything else would be a per-object decision, not
> part of the infrastructure work.

Getting rid of such setters makes sense.

It's been a while since I reviewed...  I don't remember anymore whether
we can have configuration parameters that are also properties.  If yes,
would it make sense to generate such properties?

>> Note I'm not asking whether we need setters.  I'm asking whether we
>> need to expose configuration bits via qom-set & friends in addition to
>> the QAPI schema and query-qmp-schema.
>
> I'm not sure I follow here. How is querying or changing option values
> redundant with querying which options exist?
>
> Maybe qom-list could become obsolete if we move all properties (and not
> just the configuration) into the QAPI schema, but I don't see qom-get
> and qom-set going away.
>
>> > * Getting rid of the big 'object-add' union: While the union is not too
>> >   bad for the rather small number of user-creatable objects, it
>> >   wouldn't scale at all for devices.
>> >
>> >   My idea there is that we could define something like this:
>> >
>> >   { 'struct': 'ObjectOptions',
>> >     'data': {
>> >         'id': 'str',
>> >         'config': { 'type': 'qom-config-any:user-creatable',
>> >                     'embed': true } } }
>> >
>> >   Obviously this would be an extension of the schema language to add an
>> >   'embed' option (another hopefully more acceptable attempt to flatten
>> >   things...), so I'd like to hear opinions on this first before I go to
>> >   implement it.
>> 
>> 'embed': true would splice in the members of a struct type instead of a
>> single member of that struct type.  Correct?
>> 
>> Stretch goal: make it work for union types, too :)
>> 
>> I've thought of this before.  Plenty of nesting in the wire format
>> exists pretty much only to let us have the C structs we want.  Right
>> now, the only way to "splice in" such a struct is the base type.
>> General splicing could be useful.  It may take an introspection flag
>> day.
>
> Base types aren't visible in the introspection either, so probably not
> if you continue to just report the resulting structure?

Yes, this should be feasible, except for splicing a union into a union,
because then you get multiple (tag, variants), which the introspection
schema can't do.  So don't go there, at least for now.

>> >   Also note that 'qom-config-any:user-creatable' is new, too. The
>> >   'qom-config:...' types introduced by this series don't work for
>> >   subclasses, but only for the exact class.
>> >
>> >   On the external interface, the new 'qom-config-any:...' type including
>> >   subclasses would basically behave (and be introspected) like the union
>> >   we have today, just without being defined explicitly.
>> 
>> I'm not sure I follow.  How is the qom-config-any:user-creatable to be
>> defined?  QAPI collects all the qom-config:* types into a union
>> automatically?
>
> All classes that inherit from user-creatable, but yes, automatically
> collected.
>
> For user-creatable, we can either introduce interfaces in QAPI, too, or
> we just pretend it's actually the top-level parent class.

Thanks!

Re: [RFC PATCH 00/12] QOM/QAPI integration part 1

[Kevin Wolf]

Am 14.12.2021 um 15:45 hat Markus Armbruster geschrieben:
> Kevin Wolf <kwolf@redhat.com> writes:
> 
> > Am 23.11.2021 um 17:05 hat Markus Armbruster geschrieben:
> >> Kevin Wolf <kwolf@redhat.com> writes:
> >> 
> >> > This series adds QOM class definitions to the QAPI schema, introduces
> >> > a new TypeInfo.instance_config() callback that configures the object at
> >> > creation time (instead of setting properties individually) and is
> >> > separate from runtime property setters (which often used to be not
> >> > really tested for runtime use), and finally generates a marshalling
> >> > function for .instance_config() from the QAPI schema that makes this a
> >> > natural C interface rather than a visitor based one.
> >> >
> >> > This is loosely based on Paolo's old proposal in the wiki:
> >> > https://wiki.qemu.org/Features/QOM-QAPI_integration
> >> >
> >> > The series is in a rather early stage and I don't really have any
> >> > automated tests or documentation in this series yet. I'm also only
> >> > converting the class hierarchy for the random number generator backends
> >> > to show what the result looks like, the other objects still need to be
> >> > done.
> >> >
> >> > So the question to you isn't whether this is mergeable (it isn't), but
> >> > whether you think this is the right approach for starting to integrate
> >> > QOM and QAPI better.
> >> >
> >> > You'll also see that this doesn't really remove the duplication between
> >> > property definitions in the code and configuration struct definitions in
> >> > the QAPI schema yet (because we want to keep at least a read-only
> >> > runtime property for every configuration option), but at least they mean
> >> > somewhat different things now (creation vs. runtime) instead of being
> >> > completely redundant.
> >> >
> >> > Possible future steps:
> >> >
> >> > * Define at least those properties to the schema that correspond to a
> >> >   config option. For both setters and getters for each option, we'll
> >> >   probably want to select in the schema between 'not available',
> >> >   'automatically generated function' and 'manually implemented'.
> >> >
> >> >   Other runtime properties could be either left in the code or added to
> >> >   the schema as well. Either way, we need to figure out how to best
> >> >   describe these things in the schema.
> >> 
> >> Permit me a diversion of sorts.
> >> 
> >> With QOM, we have properties.  A property is readable if it has a
> >> getter, writable if it has a setter.  There is no real concept of
> >> configuration vs. state.  Writable properties can be written at any
> >> time.
> >> 
> >> In practice, some properties are to be used only like configuration, and
> >> we check configuration at realize time (for devices), or by a surrogate
> >> like qemu_add_machine_init_done_notifier().  If you set them later,
> >> things may break, and you get to keep the pieces.
> >> 
> >> In this "QOM/QAPI integration part 1", configuration (expressed in QAPI
> >> schema) makes it into QOM.
> >> 
> >> Now we have configuration *and* properties.
> >> 
> >> Do we need the properties?
> >
> > Configuration is for creating objects, properties are for runtime after
> > the creation. So for the practical answer, as long as you can find a QOM
> > type that wants to allow either changing an option at runtime or just
> > exposing its current value, I would say, yes, we need both. And I can
> > easily list some QOM types that do.
> >
> > The theoretical answer is that of course you can replace properties with
> > custom query-* and set-* QMP commands, but that's not only hardly an
> > improvment, but also a compatibility problem.
> 
> That would be nuts.
> 
> > The approach I'm taking here with QAPIfication of objects (and planning
> > to take for future conversions) is to drop setters that can't work at
> > runtime (which might be the majority of properties), but keep properties
> > around otherwise. Everything else would be a per-object decision, not
> > part of the infrastructure work.
> 
> Getting rid of such setters makes sense.
> 
> It's been a while since I reviewed...  I don't remember anymore whether
> we can have configuration parameters that are also properties.  If yes,
> would it make sense to generate such properties?

It's not only possible, but the normal case. These properties are often
read-only after realize, but with existing types, we (almost?) always
allow querying properties later even if they can be set only before
realize (and therefore become part of the configuration after this
series).

As for generating, yes, I think that we do that at least partially
(setters will probably have to be manually implemented in the common
case). This is in fact the very proposal in the original cover letter
that you replied to here.

> >> Note I'm not asking whether we need setters.  I'm asking whether we
> >> need to expose configuration bits via qom-set & friends in addition to
> >> the QAPI schema and query-qmp-schema.
> >
> > I'm not sure I follow here. How is querying or changing option values
> > redundant with querying which options exist?
> >
> > Maybe qom-list could become obsolete if we move all properties (and not
> > just the configuration) into the QAPI schema, but I don't see qom-get
> > and qom-set going away.
> >
> >> > * Getting rid of the big 'object-add' union: While the union is not too
> >> >   bad for the rather small number of user-creatable objects, it
> >> >   wouldn't scale at all for devices.
> >> >
> >> >   My idea there is that we could define something like this:
> >> >
> >> >   { 'struct': 'ObjectOptions',
> >> >     'data': {
> >> >         'id': 'str',
> >> >         'config': { 'type': 'qom-config-any:user-creatable',
> >> >                     'embed': true } } }
> >> >
> >> >   Obviously this would be an extension of the schema language to add an
> >> >   'embed' option (another hopefully more acceptable attempt to flatten
> >> >   things...), so I'd like to hear opinions on this first before I go to
> >> >   implement it.
> >> 
> >> 'embed': true would splice in the members of a struct type instead of a
> >> single member of that struct type.  Correct?
> >> 
> >> Stretch goal: make it work for union types, too :)
> >> 
> >> I've thought of this before.  Plenty of nesting in the wire format
> >> exists pretty much only to let us have the C structs we want.  Right
> >> now, the only way to "splice in" such a struct is the base type.
> >> General splicing could be useful.  It may take an introspection flag
> >> day.
> >
> > Base types aren't visible in the introspection either, so probably not
> > if you continue to just report the resulting structure?
> 
> Yes, this should be feasible, except for splicing a union into a union,
> because then you get multiple (tag, variants), which the introspection
> schema can't do.  So don't go there, at least for now.

Right. Let's think about that when we have a use case for splicing
unions into unions.

Kevin