[PATCH v2 0/3] vl: QAPIfy -object

[PATCH v2 0/3] vl: QAPIfy -object

[Paolo Bonzini]

This is a replacement for -object QAPIfication that keeps QemuOpts
in order to not break some of the CLI parsing extensions that OptsVisitor
includes.  Since keyval is not used, support for directly passing
JSON syntax to the option must be added manually, which is what patch
3 does.  However, both the QemuOpts and the JSON paths go through
the new ObjectOptions interface, just with two different visitors,
so we can reuse all the new type-safe code that Kevin has added.

Patch 1 is a patch that I already had lying around, which I included
to be able to remove user_creatable_add_opts completely in patch 2.

Paolo

Based-on: <20210311144811.313451-1-kwolf@redhat.com>

Paolo Bonzini (3):
  tests: convert check-qom-proplist to keyval
  qom: move user_creatable_add_opts logic to vl.c and QAPIfy it
  vl: allow passing JSON to -object

 include/qom/object_interfaces.h | 47 -------------------
 qom/object_interfaces.c         | 54 ---------------------
 softmmu/vl.c                    | 83 ++++++++++++++++++++++++++-------
 tests/check-qom-proplist.c      | 77 +++++++++++++++++++++---------
 4 files changed, 120 insertions(+), 141 deletions(-)

v1->v2: avoid g_assert with side effects, fix -object without qom-type

-- 
2.26.2

[PATCH v2 1/3] tests: convert check-qom-proplist to keyval

[Paolo Bonzini]

The command-line creation test is using QemuOpts.  Switch it to keyval,
since the emulator has some special needs and thus the last user of
user_creatable_add_opts will go away with the next patch.

Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 tests/check-qom-proplist.c | 77 +++++++++++++++++++++++++++-----------
 1 file changed, 55 insertions(+), 22 deletions(-)

diff --git a/tests/check-qom-proplist.c b/tests/check-qom-proplist.c
index 1b76581980..48503e0dff 100644
--- a/tests/check-qom-proplist.c
+++ b/tests/check-qom-proplist.c
@@ -21,6 +21,9 @@
 #include "qemu/osdep.h"
 
 #include "qapi/error.h"
+#include "qapi/qobject-input-visitor.h"
+#include "qapi/qmp/qdict.h"
+#include "qapi/qmp/qobject.h"
 #include "qom/object.h"
 #include "qemu/module.h"
 #include "qemu/option.h"
@@ -398,44 +401,74 @@ static void test_dummy_createlist(void)
     object_unparent(OBJECT(dobj));
 }
 
+static bool test_create_obj(QDict *qdict, Error **errp)
+{
+    Visitor *v = qobject_input_visitor_new_keyval(QOBJECT(qdict));
+    Object *obj = user_creatable_add_type(TYPE_DUMMY, "dev0", qdict, v, errp);
+
+    visit_free(v);
+    object_unref(obj);
+    return !!obj;
+}
+
 static void test_dummy_createcmdl(void)
 {
-    QemuOpts *opts;
+    QDict *qdict;
     DummyObject *dobj;
     Error *err = NULL;
-    const char *params = TYPE_DUMMY \
-                         ",id=dev0," \
-                         "bv=yes,sv=Hiss hiss hiss,av=platypus";
+    bool created, help;
+    const char *params = "bv=yes,sv=Hiss hiss hiss,av=platypus";
 
+    /* Needed for user_creatable_del.  */
     qemu_add_opts(&qemu_object_opts);
-    opts = qemu_opts_parse(&qemu_object_opts, params, true, &err);
+
+    qdict = keyval_parse(params, "qom-type", &help, &err);
     g_assert(err == NULL);
-    g_assert(opts);
+    g_assert(qdict);
+    g_assert(!help);
 
-    dobj = DUMMY_OBJECT(user_creatable_add_opts(opts, &err));
+    created = test_create_obj(qdict, &err);
+    g_assert(created);
     g_assert(err == NULL);
+    qobject_unref(qdict);
+
+    dobj = DUMMY_OBJECT(object_resolve_path_component(object_get_objects_root(),
+                                                      "dev0"));
     g_assert(dobj);
     g_assert_cmpstr(dobj->sv, ==, "Hiss hiss hiss");
     g_assert(dobj->bv == true);
     g_assert(dobj->av == DUMMY_PLATYPUS);
 
+    qdict = keyval_parse(params, "qom-type", &help, &err);
+    created = test_create_obj(qdict, &err);
+    g_assert(!created);
+    g_assert(err);
+    g_assert(object_resolve_path_component(object_get_objects_root(), "dev0")
+             == OBJECT(dobj));
+    qobject_unref(qdict);
+    error_free(err);
+    err = NULL;
+
+    qdict = keyval_parse(params, "qom-type", &help, &err);
     user_creatable_del("dev0", &error_abort);
+    g_assert(object_resolve_path_component(object_get_objects_root(), "dev0")
+             == NULL);
 
-    object_unref(OBJECT(dobj));
-
-    /*
-     * cmdline-parsing via qemu_opts_parse() results in a QemuOpts entry
-     * corresponding to the Object's ID to be added to the QemuOptsList
-     * for objects. To avoid having this entry conflict with future
-     * Objects using the same ID (which can happen in cases where
-     * qemu_opts_parse() is used to parse the object params, such as
-     * with hmp_object_add() at the time of this comment), we need to
-     * check for this in user_creatable_del() and remove the QemuOpts if
-     * it is present.
-     *
-     * The below check ensures this works as expected.
-     */
-    g_assert_null(qemu_opts_find(&qemu_object_opts, "dev0"));
+    created = test_create_obj(qdict, &err);
+    g_assert(created);
+    g_assert(err == NULL);
+    qobject_unref(qdict);
+
+    dobj = DUMMY_OBJECT(object_resolve_path_component(object_get_objects_root(),
+                                                      "dev0"));
+    g_assert(dobj);
+    g_assert_cmpstr(dobj->sv, ==, "Hiss hiss hiss");
+    g_assert(dobj->bv == true);
+    g_assert(dobj->av == DUMMY_PLATYPUS);
+    g_assert(object_resolve_path_component(object_get_objects_root(), "dev0")
+             == OBJECT(dobj));
+
+    object_unparent(OBJECT(dobj));
 }
 
 static void test_dummy_badenum(void)
-- 
2.26.2

[PATCH v2 2/3] qom: move user_creatable_add_opts logic to vl.c and QAPIfy it

[Paolo Bonzini]

Emulators are currently using OptsVisitor (via user_creatable_add_opts)
to parse the -object command line option.  This has one extra feature,
compared to keyval, which is automatic conversion of integers to lists
as well as support for lists as repeated options:

  -object memory-backend-ram,id=pc.ram,size=1048576000,host-nodes=0,policy=bind

So we cannot replace OptsVisitor with keyval right now.  Still, this
patch moves the user_creatable_add_opts logic to vl.c since it is
not needed anywhere else, and makes it go through user_creatable_add_qapi.

In order to minimize code changes, the predicate still takes a string.
This can be changed later to use the ObjectType QAPI enum directly.

Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 include/qom/object_interfaces.h | 47 ---------------------
 qom/object_interfaces.c         | 54 ------------------------
 softmmu/vl.c                    | 74 +++++++++++++++++++++++++--------
 3 files changed, 56 insertions(+), 119 deletions(-)

diff --git a/include/qom/object_interfaces.h b/include/qom/object_interfaces.h
index fb32330901..81541e2080 100644
--- a/include/qom/object_interfaces.h
+++ b/include/qom/object_interfaces.h
@@ -94,56 +94,9 @@ Object *user_creatable_add_type(const char *type, const char *id,
  *
  * Create an instance of the user creatable object according to the
  * options passed in @opts as described in the QAPI schema documentation.
- *
- * Returns: the newly created object or NULL on error
  */
 void user_creatable_add_qapi(ObjectOptions *options, Error **errp);
 
-/**
- * user_creatable_add_opts:
- * @opts: the object definition
- * @errp: if an error occurs, a pointer to an area to store the error
- *
- * Create an instance of the user creatable object whose type
- * is defined in @opts by the 'qom-type' option, placing it
- * in the object composition tree with name provided by the
- * 'id' field. The remaining options in @opts are used to
- * initialize the object properties.
- *
- * Returns: the newly created object or NULL on error
- */
-Object *user_creatable_add_opts(QemuOpts *opts, Error **errp);
-
-
-/**
- * user_creatable_add_opts_predicate:
- * @type: the QOM type to be added
- *
- * A callback function to determine whether an object
- * of type @type should be created. Instances of this
- * callback should be passed to user_creatable_add_opts_foreach
- */
-typedef bool (*user_creatable_add_opts_predicate)(const char *type);
-
-/**
- * user_creatable_add_opts_foreach:
- * @opaque: a user_creatable_add_opts_predicate callback or NULL
- * @opts: options to create
- * @errp: unused
- *
- * An iterator callback to be used in conjunction with
- * the qemu_opts_foreach() method for creating a list of
- * objects from a set of QemuOpts
- *
- * The @opaque parameter can be passed a user_creatable_add_opts_predicate
- * callback to filter which types of object are created during iteration.
- * When it fails, report the error.
- *
- * Returns: 0 on success, -1 when an error was reported.
- */
-int user_creatable_add_opts_foreach(void *opaque,
-                                    QemuOpts *opts, Error **errp);
-
 /**
  * user_creatable_parse_str:
  * @optarg: the object definition string as passed on the command line
diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c
index 62d7db7629..7074040db1 100644
--- a/qom/object_interfaces.c
+++ b/qom/object_interfaces.c
@@ -140,60 +140,6 @@ void user_creatable_add_qapi(ObjectOptions *options, Error **errp)
     visit_free(v);
 }
 
-Object *user_creatable_add_opts(QemuOpts *opts, Error **errp)
-{
-    Visitor *v;
-    QDict *pdict;
-    Object *obj;
-    const char *id = qemu_opts_id(opts);
-    char *type = qemu_opt_get_del(opts, "qom-type");
-
-    if (!type) {
-        error_setg(errp, QERR_MISSING_PARAMETER, "qom-type");
-        return NULL;
-    }
-    if (!id) {
-        error_setg(errp, QERR_MISSING_PARAMETER, "id");
-        qemu_opt_set(opts, "qom-type", type, &error_abort);
-        g_free(type);
-        return NULL;
-    }
-
-    qemu_opts_set_id(opts, NULL);
-    pdict = qemu_opts_to_qdict(opts, NULL);
-
-    v = opts_visitor_new(opts);
-    obj = user_creatable_add_type(type, id, pdict, v, errp);
-    visit_free(v);
-
-    qemu_opts_set_id(opts, (char *) id);
-    qemu_opt_set(opts, "qom-type", type, &error_abort);
-    g_free(type);
-    qobject_unref(pdict);
-    return obj;
-}
-
-
-int user_creatable_add_opts_foreach(void *opaque, QemuOpts *opts, Error **errp)
-{
-    bool (*type_opt_predicate)(const char *, QemuOpts *) = opaque;
-    Object *obj = NULL;
-    const char *type;
-
-    type = qemu_opt_get(opts, "qom-type");
-    if (type && type_opt_predicate &&
-        !type_opt_predicate(type, opts)) {
-        return 0;
-    }
-
-    obj = user_creatable_add_opts(opts, errp);
-    if (!obj) {
-        return -1;
-    }
-    object_unref(obj);
-    return 0;
-}
-
 char *object_property_help(const char *name, const char *type,
                            QObject *defval, const char *description)
 {
diff --git a/softmmu/vl.c b/softmmu/vl.c
index ff488ea3e7..ae017de46c 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -117,6 +117,7 @@
 #include "qapi/qapi-commands-block-core.h"
 #include "qapi/qapi-commands-migration.h"
 #include "qapi/qapi-commands-misc.h"
+#include "qapi/qapi-visit-qom.h"
 #include "qapi/qapi-commands-ui.h"
 #include "qapi/qmp/qerror.h"
 #include "sysemu/iothread.h"
@@ -132,10 +133,16 @@ typedef struct BlockdevOptionsQueueEntry {
 
 typedef QSIMPLEQ_HEAD(, BlockdevOptionsQueueEntry) BlockdevOptionsQueue;
 
+typedef struct ObjectOption {
+    ObjectOptions *opts;
+    QTAILQ_ENTRY(ObjectOption) next;
+} ObjectOption;
+
 static const char *cpu_option;
 static const char *mem_path;
 static const char *incoming;
 static const char *loadvm;
+static QTAILQ_HEAD(, ObjectOption) object_opts = QTAILQ_HEAD_INITIALIZER(object_opts);
 static ram_addr_t maxram_size;
 static uint64_t ram_slots;
 static int display_remote;
@@ -1684,6 +1691,49 @@ static int machine_set_property(void *opaque,
     return object_parse_property_opt(opaque, name, value, "type", errp);
 }
 
+static void object_option_foreach_add(bool (*type_opt_predicate)(const char *))
+{
+    ObjectOption *opt, *next;
+
+    QTAILQ_FOREACH_SAFE(opt, &object_opts, next, next) {
+        const char *type = ObjectType_str(opt->opts->qom_type);
+        if (type_opt_predicate(type)) {
+            user_creatable_add_qapi(opt->opts, &error_fatal);
+            qapi_free_ObjectOptions(opt->opts);
+            QTAILQ_REMOVE(&object_opts, opt, next);
+        }
+    }
+}
+
+static void object_option_parse(const char *optarg)
+{
+    ObjectOption *opt;
+    QemuOpts *opts;
+    const char *type;
+    Visitor *v;
+
+    opts = qemu_opts_parse_noisily(qemu_find_opts("object"),
+                                   optarg, true);
+    if (!opts) {
+        exit(1);
+    }
+
+    type = qemu_opt_get(opts, "qom-type");
+    if (!type) {
+        error_setg(&error_fatal, QERR_MISSING_PARAMETER, "qom-type");
+    }
+    if (user_creatable_print_help(type, opts)) {
+        exit(0);
+    }
+
+    opt = g_new0(ObjectOption, 1);
+    v = opts_visitor_new(opts);
+    visit_type_ObjectOptions(v, NULL, &opt->opts, &error_fatal);
+    visit_free(v);
+
+    QTAILQ_INSERT_TAIL(&object_opts, opt, next);
+}
+
 /*
  * Initial object creation happens before all other
  * QEMU data types are created. The majority of objects
@@ -1691,12 +1741,8 @@ static int machine_set_property(void *opaque,
  * cannot be created here, as it depends on the chardev
  * already existing.
  */
-static bool object_create_early(const char *type, QemuOpts *opts)
+static bool object_create_early(const char *type)
 {
-    if (user_creatable_print_help(type, opts)) {
-        exit(0);
-    }
-
     /*
      * Objects should not be made "delayed" without a reason.  If you
      * add one, state the reason in a comment!
@@ -1815,9 +1861,7 @@ static void qemu_create_early_backends(void)
         exit(1);
     }
 
-    qemu_opts_foreach(qemu_find_opts("object"),
-                      user_creatable_add_opts_foreach,
-                      object_create_early, &error_fatal);
+    object_option_foreach_add(object_create_early);
 
     /* spice needs the timers to be initialized by this point */
     /* spice must initialize before audio as it changes the default auiodev */
@@ -1846,9 +1890,9 @@ static void qemu_create_early_backends(void)
  * The remainder of object creation happens after the
  * creation of chardev, fsdev, net clients and device data types.
  */
-static bool object_create_late(const char *type, QemuOpts *opts)
+static bool object_create_late(const char *type)
 {
-    return !object_create_early(type, opts);
+    return !object_create_early(type);
 }
 
 static void qemu_create_late_backends(void)
@@ -1859,9 +1903,7 @@ static void qemu_create_late_backends(void)
 
     net_init_clients(&error_fatal);
 
-    qemu_opts_foreach(qemu_find_opts("object"),
-                      user_creatable_add_opts_foreach,
-                      object_create_late, &error_fatal);
+    object_option_foreach_add(object_create_late);
 
     if (tpm_init() < 0) {
         exit(1);
@@ -3398,11 +3440,7 @@ void qemu_init(int argc, char **argv, char **envp)
 #endif
                 break;
             case QEMU_OPTION_object:
-                opts = qemu_opts_parse_noisily(qemu_find_opts("object"),
-                                               optarg, true);
-                if (!opts) {
-                    exit(1);
-                }
+                object_option_parse(optarg);
                 break;
             case QEMU_OPTION_overcommit:
                 opts = qemu_opts_parse_noisily(qemu_find_opts("overcommit"),
-- 
2.26.2

[PATCH v2 3/3] vl: allow passing JSON to -object

[Paolo Bonzini]

Extend the ObjectOption code that was added in the previous patch to
enable passing JSON to -object.  Even though we cannot yet add
non-scalar properties with the human-friendly comma-separated syntax,
they can now be added as JSON.

Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 softmmu/vl.c | 33 +++++++++++++++++++++------------
 1 file changed, 21 insertions(+), 12 deletions(-)

diff --git a/softmmu/vl.c b/softmmu/vl.c
index ae017de46c..0e3fa53057 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -31,6 +31,7 @@
 #include "hw/qdev-properties.h"
 #include "qapi/error.h"
 #include "qapi/qmp/qdict.h"
+#include "qapi/qmp/qjson.h"
 #include "qemu-version.h"
 #include "qemu/cutils.h"
 #include "qemu/help_option.h"
@@ -1712,22 +1713,30 @@ static void object_option_parse(const char *optarg)
     const char *type;
     Visitor *v;
 
-    opts = qemu_opts_parse_noisily(qemu_find_opts("object"),
-                                   optarg, true);
-    if (!opts) {
-        exit(1);
-    }
+    if (optarg[0] == '{') {
+        QObject *obj = qobject_from_json(optarg, &error_fatal);
 
-    type = qemu_opt_get(opts, "qom-type");
-    if (!type) {
-        error_setg(&error_fatal, QERR_MISSING_PARAMETER, "qom-type");
-    }
-    if (user_creatable_print_help(type, opts)) {
-        exit(0);
+        v = qobject_input_visitor_new(obj);
+        qobject_unref(obj);
+    } else {
+        opts = qemu_opts_parse_noisily(qemu_find_opts("object"),
+                                       optarg, true);
+        if (!opts) {
+            exit(1);
+        }
+
+        type = qemu_opt_get(opts, "qom-type");
+        if (!type) {
+            error_setg(&error_fatal, QERR_MISSING_PARAMETER, "qom-type");
+        }
+        if (user_creatable_print_help(type, opts)) {
+            exit(0);
+        }
+
+        v = opts_visitor_new(opts);
     }
 
     opt = g_new0(ObjectOption, 1);
-    v = opts_visitor_new(opts);
     visit_type_ObjectOptions(v, NULL, &opt->opts, &error_fatal);
     visit_free(v);
 
-- 
2.26.2

Re: [PATCH v2 0/3] vl: QAPIfy -object

[Kevin Wolf]

Am 12.03.2021 um 18:35 hat Paolo Bonzini geschrieben:
> This is a replacement for -object QAPIfication that keeps QemuOpts
> in order to not break some of the CLI parsing extensions that OptsVisitor
> includes.  Since keyval is not used, support for directly passing
> JSON syntax to the option must be added manually, which is what patch
> 3 does.  However, both the QemuOpts and the JSON paths go through
> the new ObjectOptions interface, just with two different visitors,
> so we can reuse all the new type-safe code that Kevin has added.
> 
> Patch 1 is a patch that I already had lying around, which I included
> to be able to remove user_creatable_add_opts completely in patch 2.
> 
> Paolo
> 
> Based-on: <20210311144811.313451-1-kwolf@redhat.com>

Thanks, applied to the block branch.

Kevin

Re: [PATCH v2 2/3] qom: move user_creatable_add_opts logic to vl.c and QAPIfy it

[Kevin Wolf]

Am 12.03.2021 um 18:35 hat Paolo Bonzini geschrieben:
> Emulators are currently using OptsVisitor (via user_creatable_add_opts)
> to parse the -object command line option.  This has one extra feature,
> compared to keyval, which is automatic conversion of integers to lists
> as well as support for lists as repeated options:
> 
>   -object memory-backend-ram,id=pc.ram,size=1048576000,host-nodes=0,policy=bind
> 
> So we cannot replace OptsVisitor with keyval right now.  Still, this
> patch moves the user_creatable_add_opts logic to vl.c since it is
> not needed anywhere else, and makes it go through user_creatable_add_qapi.
> 
> In order to minimize code changes, the predicate still takes a string.
> This can be changed later to use the ObjectType QAPI enum directly.
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

> diff --git a/softmmu/vl.c b/softmmu/vl.c
> index ff488ea3e7..ae017de46c 100644
> --- a/softmmu/vl.c
> +++ b/softmmu/vl.c
> @@ -117,6 +117,7 @@
>  #include "qapi/qapi-commands-block-core.h"
>  #include "qapi/qapi-commands-migration.h"
>  #include "qapi/qapi-commands-misc.h"
> +#include "qapi/qapi-visit-qom.h"
>  #include "qapi/qapi-commands-ui.h"
>  #include "qapi/qmp/qerror.h"
>  #include "sysemu/iothread.h"
> @@ -132,10 +133,16 @@ typedef struct BlockdevOptionsQueueEntry {
>  
>  typedef QSIMPLEQ_HEAD(, BlockdevOptionsQueueEntry) BlockdevOptionsQueue;
>  
> +typedef struct ObjectOption {
> +    ObjectOptions *opts;
> +    QTAILQ_ENTRY(ObjectOption) next;
> +} ObjectOption;
> +
>  static const char *cpu_option;
>  static const char *mem_path;
>  static const char *incoming;
>  static const char *loadvm;
> +static QTAILQ_HEAD(, ObjectOption) object_opts = QTAILQ_HEAD_INITIALIZER(object_opts);
>  static ram_addr_t maxram_size;
>  static uint64_t ram_slots;
>  static int display_remote;
> @@ -1684,6 +1691,49 @@ static int machine_set_property(void *opaque,
>      return object_parse_property_opt(opaque, name, value, "type", errp);
>  }
>  
> +static void object_option_foreach_add(bool (*type_opt_predicate)(const char *))
> +{
> +    ObjectOption *opt, *next;
> +
> +    QTAILQ_FOREACH_SAFE(opt, &object_opts, next, next) {
> +        const char *type = ObjectType_str(opt->opts->qom_type);
> +        if (type_opt_predicate(type)) {
> +            user_creatable_add_qapi(opt->opts, &error_fatal);
> +            qapi_free_ObjectOptions(opt->opts);
> +            QTAILQ_REMOVE(&object_opts, opt, next);

I added a g_free(opt) here to fix CI failures (LeakSanitizer error).

> +        }
> +    }
> +}

Kevin

Re: [PATCH v2 2/3] qom: move user_creatable_add_opts logic to vl.c and QAPIfy it

[David Hildenbrand]

On 12.03.21 18:35, Paolo Bonzini wrote:
> Emulators are currently using OptsVisitor (via user_creatable_add_opts)
> to parse the -object command line option.  This has one extra feature,
> compared to keyval, which is automatic conversion of integers to lists
> as well as support for lists as repeated options:
> 
>    -object memory-backend-ram,id=pc.ram,size=1048576000,host-nodes=0,policy=bind
> 
> So we cannot replace OptsVisitor with keyval right now.  Still, this
> patch moves the user_creatable_add_opts logic to vl.c since it is
> not needed anywhere else, and makes it go through user_creatable_add_qapi.
> 
> In order to minimize code changes, the predicate still takes a string.
> This can be changed later to use the ObjectType QAPI enum directly.
> 

Rebasing my "noreserve"[1] series on this, I get weird errors from QEMU 
when specifying the new "reserve=off" option for a memory-backend-ram:

"Invalid parameter 'reserve'"

And it looks like this is the case for any new properties. Poking 
around, I fail to find what's causing this -- or how to unlock new 
properties. What is the magic toggle to make it work?

Thanks!

[1] https://lkml.kernel.org/r/20210319101230.21531-1-david@redhat.com

-- 
Thanks,

David / dhildenb

Re: [PATCH v2 2/3] qom: move user_creatable_add_opts logic to vl.c and QAPIfy it

[Markus Armbruster]

David Hildenbrand <david@redhat.com> writes:

> On 12.03.21 18:35, Paolo Bonzini wrote:
>> Emulators are currently using OptsVisitor (via user_creatable_add_opts)
>> to parse the -object command line option.  This has one extra feature,
>> compared to keyval, which is automatic conversion of integers to lists
>> as well as support for lists as repeated options:
>>    -object
>> memory-backend-ram,id=pc.ram,size=1048576000,host-nodes=0,policy=bind
>> So we cannot replace OptsVisitor with keyval right now.  Still, this
>> patch moves the user_creatable_add_opts logic to vl.c since it is
>> not needed anywhere else, and makes it go through user_creatable_add_qapi.
>> In order to minimize code changes, the predicate still takes a
>> string.
>> This can be changed later to use the ObjectType QAPI enum directly.
>> 
>
> Rebasing my "noreserve"[1] series on this, I get weird errors from
> QEMU when specifying the new "reserve=off" option for a
> memory-backend-ram:
>
> "Invalid parameter 'reserve'"
>
> And it looks like this is the case for any new properties. Poking
> around, I fail to find what's causing this -- or how to unlock new 
> properties. What is the magic toggle to make it work?
>
> Thanks!
>
> [1] https://lkml.kernel.org/r/20210319101230.21531-1-david@redhat.com

Wild guess: you didn't add your new properties in the QAPI schema.

For a not-so-wild-guess, send us a git-fetch argument for your rebased
series.

Re: [PATCH v2 2/3] qom: move user_creatable_add_opts logic to vl.c and QAPIfy it

[David Hildenbrand]

On 13.04.21 06:41, Markus Armbruster wrote:
> David Hildenbrand <david@redhat.com> writes:
> 
>> On 12.03.21 18:35, Paolo Bonzini wrote:
>>> Emulators are currently using OptsVisitor (via user_creatable_add_opts)
>>> to parse the -object command line option.  This has one extra feature,
>>> compared to keyval, which is automatic conversion of integers to lists
>>> as well as support for lists as repeated options:
>>>     -object
>>> memory-backend-ram,id=pc.ram,size=1048576000,host-nodes=0,policy=bind
>>> So we cannot replace OptsVisitor with keyval right now.  Still, this
>>> patch moves the user_creatable_add_opts logic to vl.c since it is
>>> not needed anywhere else, and makes it go through user_creatable_add_qapi.
>>> In order to minimize code changes, the predicate still takes a
>>> string.
>>> This can be changed later to use the ObjectType QAPI enum directly.
>>>
>>
>> Rebasing my "noreserve"[1] series on this, I get weird errors from
>> QEMU when specifying the new "reserve=off" option for a
>> memory-backend-ram:
>>
>> "Invalid parameter 'reserve'"
>>
>> And it looks like this is the case for any new properties. Poking
>> around, I fail to find what's causing this -- or how to unlock new
>> properties. What is the magic toggle to make it work?
>>
>> Thanks!
>>
>> [1] https://lkml.kernel.org/r/20210319101230.21531-1-david@redhat.com
> 
> Wild guess: you didn't add your new properties in the QAPI schema.
> 
> For a not-so-wild-guess, send us a git-fetch argument for your rebased
> series.
> 

Oh, there is qapi/qom.json -- maybe that does the trick.

(I have mixed feelings about having to specify the same thing twice at 
different locations)

I'll have a look if that makes it fly.

-- 
Thanks,

David / dhildenb

Re: [PATCH v2 2/3] qom: move user_creatable_add_opts logic to vl.c and QAPIfy it

[David Hildenbrand]

On 13.04.21 10:13, David Hildenbrand wrote:
> On 13.04.21 06:41, Markus Armbruster wrote:
>> David Hildenbrand <david@redhat.com> writes:
>>
>>> On 12.03.21 18:35, Paolo Bonzini wrote:
>>>> Emulators are currently using OptsVisitor (via user_creatable_add_opts)
>>>> to parse the -object command line option.  This has one extra feature,
>>>> compared to keyval, which is automatic conversion of integers to lists
>>>> as well as support for lists as repeated options:
>>>>      -object
>>>> memory-backend-ram,id=pc.ram,size=1048576000,host-nodes=0,policy=bind
>>>> So we cannot replace OptsVisitor with keyval right now.  Still, this
>>>> patch moves the user_creatable_add_opts logic to vl.c since it is
>>>> not needed anywhere else, and makes it go through user_creatable_add_qapi.
>>>> In order to minimize code changes, the predicate still takes a
>>>> string.
>>>> This can be changed later to use the ObjectType QAPI enum directly.
>>>>
>>>
>>> Rebasing my "noreserve"[1] series on this, I get weird errors from
>>> QEMU when specifying the new "reserve=off" option for a
>>> memory-backend-ram:
>>>
>>> "Invalid parameter 'reserve'"
>>>
>>> And it looks like this is the case for any new properties. Poking
>>> around, I fail to find what's causing this -- or how to unlock new
>>> properties. What is the magic toggle to make it work?
>>>
>>> Thanks!
>>>
>>> [1] https://lkml.kernel.org/r/20210319101230.21531-1-david@redhat.com
>>
>> Wild guess: you didn't add your new properties in the QAPI schema.
>>
>> For a not-so-wild-guess, send us a git-fetch argument for your rebased
>> series.
>>
> 
> Oh, there is qapi/qom.json -- maybe that does the trick.
> 
> (I have mixed feelings about having to specify the same thing twice at
> different locations)
> 
> I'll have a look if that makes it fly.

Yes, works just fine -- thanks!


-- 
Thanks,

David / dhildenb

Re: [PATCH v2 2/3] qom: move user_creatable_add_opts logic to vl.c and QAPIfy it

[Kevin Wolf]

Am 13.04.2021 um 10:13 hat David Hildenbrand geschrieben:
> On 13.04.21 06:41, Markus Armbruster wrote:
> > David Hildenbrand <david@redhat.com> writes:
> > 
> > > On 12.03.21 18:35, Paolo Bonzini wrote:
> > > > Emulators are currently using OptsVisitor (via user_creatable_add_opts)
> > > > to parse the -object command line option.  This has one extra feature,
> > > > compared to keyval, which is automatic conversion of integers to lists
> > > > as well as support for lists as repeated options:
> > > >     -object
> > > > memory-backend-ram,id=pc.ram,size=1048576000,host-nodes=0,policy=bind
> > > > So we cannot replace OptsVisitor with keyval right now.  Still, this
> > > > patch moves the user_creatable_add_opts logic to vl.c since it is
> > > > not needed anywhere else, and makes it go through user_creatable_add_qapi.
> > > > In order to minimize code changes, the predicate still takes a
> > > > string.
> > > > This can be changed later to use the ObjectType QAPI enum directly.
> > > > 
> > > 
> > > Rebasing my "noreserve"[1] series on this, I get weird errors from
> > > QEMU when specifying the new "reserve=off" option for a
> > > memory-backend-ram:
> > > 
> > > "Invalid parameter 'reserve'"
> > > 
> > > And it looks like this is the case for any new properties. Poking
> > > around, I fail to find what's causing this -- or how to unlock new
> > > properties. What is the magic toggle to make it work?
> > > 
> > > Thanks!
> > > 
> > > [1] https://lkml.kernel.org/r/20210319101230.21531-1-david@redhat.com
> > 
> > Wild guess: you didn't add your new properties in the QAPI schema.
> > 
> > For a not-so-wild-guess, send us a git-fetch argument for your rebased
> > series.
> > 
> 
> Oh, there is qapi/qom.json -- maybe that does the trick.
> 
> (I have mixed feelings about having to specify the same thing twice at
> different locations)

The idea is that we'll eventually generate some of the QOM boilerplate
from the QAPI schema and remove the duplication again.

But yes, for the time being, you need to touch both places.

Kevin

Re: [PATCH v2 2/3] qom: move user_creatable_add_opts logic to vl.c and QAPIfy it

[Markus Armbruster]

David Hildenbrand <david@redhat.com> writes:

> On 13.04.21 06:41, Markus Armbruster wrote:
>> David Hildenbrand <david@redhat.com> writes:
>> 
>>> On 12.03.21 18:35, Paolo Bonzini wrote:
>>>> Emulators are currently using OptsVisitor (via user_creatable_add_opts)
>>>> to parse the -object command line option.  This has one extra feature,
>>>> compared to keyval, which is automatic conversion of integers to lists
>>>> as well as support for lists as repeated options:
>>>>     -object
>>>> memory-backend-ram,id=pc.ram,size=1048576000,host-nodes=0,policy=bind
>>>> So we cannot replace OptsVisitor with keyval right now.  Still, this
>>>> patch moves the user_creatable_add_opts logic to vl.c since it is
>>>> not needed anywhere else, and makes it go through user_creatable_add_qapi.
>>>> In order to minimize code changes, the predicate still takes a
>>>> string.
>>>> This can be changed later to use the ObjectType QAPI enum directly.
>>>>
>>>
>>> Rebasing my "noreserve"[1] series on this, I get weird errors from
>>> QEMU when specifying the new "reserve=off" option for a
>>> memory-backend-ram:
>>>
>>> "Invalid parameter 'reserve'"
>>>
>>> And it looks like this is the case for any new properties. Poking
>>> around, I fail to find what's causing this -- or how to unlock new
>>> properties. What is the magic toggle to make it work?
>>>
>>> Thanks!
>>>
>>> [1] https://lkml.kernel.org/r/20210319101230.21531-1-david@redhat.com
>> Wild guess: you didn't add your new properties in the QAPI schema.
>> For a not-so-wild-guess, send us a git-fetch argument for your
>> rebased
>> series.
>> 
>
> Oh, there is qapi/qom.json -- maybe that does the trick.
>
> (I have mixed feelings about having to specify the same thing twice at
> different locations)

With reason.

We've talked about generating QOM boilerplate from the QAPI schema, but
haven't progressed to actual patches.

> I'll have a look if that makes it fly.

Re: [PATCH v2 2/3] qom: move user_creatable_add_opts logic to vl.c and QAPIfy it

[Paolo Bonzini]

On 13/04/21 11:48, Markus Armbruster wrote:
>> Oh, there is qapi/qom.json -- maybe that does the trick.
>>
>> (I have mixed feelings about having to specify the same thing twice at
>> different locations)
> 
> With reason.
> 
> We've talked about generating QOM boilerplate from the QAPI schema, but
> haven't progressed to actual patches.

There is an initial sketch at
https://wiki.qemu.org/Features/QOM-QAPI_integration, covering what can 
be done before touching the code generator.

The basic idea is to keep property getters, which allow to read how an 
object was configured and already vastly reduces the amount of 
boilerplate, and replace the setters with a method that takes a QAPI 
visitor.

Later on the low-level QAPI code can be generated by qapi-gen, similar 
to QMP commands.  However, unlike QMP (where QAPIfication included 
switching from raw QDicts to QAPI structs) here we already start with 
code that uses vistors and QAPI structs.  So it's not like having to 
rewrite things twice.

Paolo