[PATCH 0/3] vl: QAPIfy -object

[PATCH 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 | 50 ++------------------
 qom/object_interfaces.c         | 57 +----------------------
 softmmu/vl.c                    | 82 +++++++++++++++++++++++++--------
 tests/check-qom-proplist.c      | 74 ++++++++++++++++++++---------
 4 files changed, 121 insertions(+), 142 deletions(-)

-- 
2.26.2

[PATCH 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.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 tests/check-qom-proplist.c | 74 ++++++++++++++++++++++++++------------
 1 file changed, 52 insertions(+), 22 deletions(-)

diff --git a/tests/check-qom-proplist.c b/tests/check-qom-proplist.c
index 1b76581980..73472944a0 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,71 @@ 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 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));
+    g_assert(test_create_obj(qdict, &err));
     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);
+    g_assert(!test_create_obj(qdict, &err));
+    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"));
+    g_assert(test_create_obj(qdict, &err));
+    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 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.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 include/qom/object_interfaces.h | 50 ++--------------------
 qom/object_interfaces.c         | 57 +------------------------
 softmmu/vl.c                    | 73 +++++++++++++++++++++++++--------
 3 files changed, 60 insertions(+), 120 deletions(-)

diff --git a/include/qom/object_interfaces.h b/include/qom/object_interfaces.h
index fb32330901..454fe4435b 100644
--- a/include/qom/object_interfaces.h
+++ b/include/qom/object_interfaces.h
@@ -95,54 +95,10 @@ 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.
+ * Returns: true when an object was successfully created, false when an error
+ * occurred
  */
-int user_creatable_add_opts_foreach(void *opaque,
-                                    QemuOpts *opts, Error **errp);
+bool user_creatable_add_qapi(ObjectOptions *options, Error **errp);
 
 /**
  * user_creatable_parse_str:
diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c
index 62d7db7629..2e50698075 100644
--- a/qom/object_interfaces.c
+++ b/qom/object_interfaces.c
@@ -117,7 +117,7 @@ out:
     return obj;
 }
 
-void user_creatable_add_qapi(ObjectOptions *options, Error **errp)
+bool user_creatable_add_qapi(ObjectOptions *options, Error **errp)
 {
     Visitor *v;
     QObject *qobj;
@@ -138,60 +138,7 @@ void user_creatable_add_qapi(ObjectOptions *options, Error **errp)
                                   options->id, props, v, errp);
     object_unref(obj);
     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;
+    return obj != NULL;
 }
 
 char *object_property_help(const char *name, const char *type,
diff --git a/softmmu/vl.c b/softmmu/vl.c
index ff488ea3e7..b245e912e5 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,48 @@ 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 *), Error **errp)
+{
+    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)) {
+            if (!user_creatable_add_qapi(opt->opts, errp)) {
+                return;
+            }
+            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 (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 +1740,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 +1860,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, &error_fatal);
 
     /* spice needs the timers to be initialized by this point */
     /* spice must initialize before audio as it changes the default auiodev */
@@ -1846,9 +1889,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 +1902,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, &error_fatal);
 
     if (tpm_init() < 0) {
         exit(1);
@@ -3398,11 +3439,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 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.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 softmmu/vl.c | 27 ++++++++++++++++++---------
 1 file changed, 18 insertions(+), 9 deletions(-)

diff --git a/softmmu/vl.c b/softmmu/vl.c
index b245e912e5..7b07f19de7 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"
@@ -1714,19 +1715,27 @@ 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 (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 (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 0/3] vl: QAPIfy -object

[no-reply]

Patchew URL: https://patchew.org/QEMU/20210311172459.990281-1-pbonzini@redhat.com/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 20210311172459.990281-1-pbonzini@redhat.com
Subject: [PATCH 0/3] vl: QAPIfy -object

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 - [tag update]      patchew/20210310154526.463850-1-pbonzini@redhat.com -> patchew/20210310154526.463850-1-pbonzini@redhat.com
 * [new tag]         patchew/20210311172459.990281-1-pbonzini@redhat.com -> patchew/20210311172459.990281-1-pbonzini@redhat.com
Switched to a new branch 'test'
56d1575 vl: allow passing JSON to -object
320d5b3 qom: move user_creatable_add_opts logic to vl.c and QAPIfy it
645f9df tests: convert check-qom-proplist to keyval

=== OUTPUT BEGIN ===
1/3 Checking commit 645f9dfaea05 (tests: convert check-qom-proplist to keyval)
2/3 Checking commit 320d5b301968 (qom: move user_creatable_add_opts logic to vl.c and QAPIfy it)
WARNING: line over 80 characters
#192: FILE: softmmu/vl.c:145:
+static QTAILQ_HEAD(, ObjectOption) object_opts = QTAILQ_HEAD_INITIALIZER(object_opts);

ERROR: line over 90 characters
#200: FILE: softmmu/vl.c:1694:
+static void object_option_foreach_add(bool (*type_opt_predicate)(const char *), Error **errp)

total: 1 errors, 1 warnings, 253 lines checked

Patch 2/3 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

3/3 Checking commit 56d1575bce1e (vl: allow passing JSON to -object)
=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20210311172459.990281-1-pbonzini@redhat.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

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

[Eric Blake]

On 3/11/21 11:24 AM, Paolo Bonzini wrote:
> 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.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  tests/check-qom-proplist.c | 74 ++++++++++++++++++++++++++------------
>  1 file changed, 52 insertions(+), 22 deletions(-)
> 

>  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 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));
> +    g_assert(test_create_obj(qdict, &err));

This performs a side-effect inside of g_assert().  Do we care?  On the
one hand, this is a testsuite (where disabling g_assert weakens the
test), on the other hand, even if we can guarantee g_assert is not
disabled in the testsuite, it lends itself to poor copy-and-paste
practice to other sites.  Better would be to assign to a bool outside
g_assert(), then assert the variable.

>      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);
> +    g_assert(!test_create_obj(qdict, &err));

And again.

> +    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"));
> +    g_assert(test_create_obj(qdict, &err));

and again

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

Otherwise, this looks like a sane thing to do.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

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

[Eric Blake]

On 3/11/21 11:24 AM, 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.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  include/qom/object_interfaces.h | 50 ++--------------------
>  qom/object_interfaces.c         | 57 +------------------------
>  softmmu/vl.c                    | 73 +++++++++++++++++++++++++--------
>  3 files changed, 60 insertions(+), 120 deletions(-)
> 

> @@ -1684,6 +1691,48 @@ 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 *), Error **errp)

Should this return a bool...

> +{
> +    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)) {
> +            if (!user_creatable_add_qapi(opt->opts, errp)) {
> +                return;

with return false here,

> +            }
> +            qapi_free_ObjectOptions(opt->opts);
> +            QTAILQ_REMOVE(&object_opts, opt, next);
> +        }
> +    }

and true here, to make it easier for callers to detect failure without
having to inspect errp?

> +}
> +
> +static void object_option_parse(const char *optarg)

> @@ -1815,9 +1860,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, &error_fatal);

Then again, since the only callers pass error_fatal, we never reach the
early return.

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

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

[Eric Blake]

On 3/11/21 11:24 AM, Paolo Bonzini wrote:
> 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.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  softmmu/vl.c | 27 ++++++++++++++++++---------
>  1 file changed, 18 insertions(+), 9 deletions(-)
> 
> diff --git a/softmmu/vl.c b/softmmu/vl.c
> index b245e912e5..7b07f19de7 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"
> @@ -1714,19 +1715,27 @@ 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 (user_creatable_print_help(type, opts)) {
> -        exit(0);
> +        v = qobject_input_visitor_new(obj);
> +        qobject_unref(obj);

Interesting note: the JSON form has no way to access help text.  But
that's not a show-stopper.

> +    } else {
> +        opts = qemu_opts_parse_noisily(qemu_find_opts("object"),
> +                                       optarg, true);
> +        if (!opts) {
> +            exit(1);
> +        }
> +
> +        type = qemu_opt_get(opts, "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);

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

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

[Kevin Wolf]

Am 11.03.2021 um 18:24 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.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

> diff --git a/softmmu/vl.c b/softmmu/vl.c
> index ff488ea3e7..b245e912e5 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,48 @@ 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 *), Error **errp)
> +{
> +    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)) {
> +            if (!user_creatable_add_qapi(opt->opts, errp)) {
> +                return;

Technically, this leaks things, though all callers pass &error_fatal
anyway, so this branch is dead code.

Should we just drop the errp parameter and explicitly use &error_fatal
here?

> +            }
> +            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 (user_creatable_print_help(type, opts)) {

type needs a NULL check.

Before this patch:
$ build/qemu-system-x86_64 -object id=foo
qemu-system-x86_64: -object id=foo: Parameter 'qom-type' is missing

After the patch:
$ build/qemu-system-x86_64 -object id=foo
Segmentation fault (core dumped)

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

Kevin

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

[Kevin Wolf]

Am 11.03.2021 um 18:24 hat Paolo Bonzini geschrieben:
> 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.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

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

[Kevin Wolf]

Am 11.03.2021 um 18:24 hat Paolo Bonzini geschrieben:
> 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.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

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

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> 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 new uses would be quite unwelcome :)

>                           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.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  include/qom/object_interfaces.h | 50 ++--------------------
>  qom/object_interfaces.c         | 57 +------------------------
>  softmmu/vl.c                    | 73 +++++++++++++++++++++++++--------
>  3 files changed, 60 insertions(+), 120 deletions(-)

Nice diffstat.

>
> diff --git a/include/qom/object_interfaces.h b/include/qom/object_interfaces.h
> index fb32330901..454fe4435b 100644
> --- a/include/qom/object_interfaces.h
> +++ b/include/qom/object_interfaces.h
> @@ -95,54 +95,10 @@ 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.
> + * Returns: true when an object was successfully created, false when an error
> + * occurred
>   */
> -int user_creatable_add_opts_foreach(void *opaque,
> -                                    QemuOpts *opts, Error **errp);
> +bool user_creatable_add_qapi(ObjectOptions *options, Error **errp);
>  
>  /**
>   * user_creatable_parse_str:
> diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c
> index 62d7db7629..2e50698075 100644
> --- a/qom/object_interfaces.c
> +++ b/qom/object_interfaces.c
> @@ -117,7 +117,7 @@ out:
>      return obj;
>  }
>  
> -void user_creatable_add_qapi(ObjectOptions *options, Error **errp)
> +bool user_creatable_add_qapi(ObjectOptions *options, Error **errp)
>  {
>      Visitor *v;
>      QObject *qobj;
> @@ -138,60 +138,7 @@ void user_creatable_add_qapi(ObjectOptions *options, Error **errp)
>                                    options->id, props, v, errp);
>      object_unref(obj);
>      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);

These things are now checked by visit_type_ObjectOptions() below.

> -    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;
> +    return obj != NULL;
>  }
>  
>  char *object_property_help(const char *name, const char *type,
> diff --git a/softmmu/vl.c b/softmmu/vl.c
> index ff488ea3e7..b245e912e5 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,48 @@ 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 *), Error **errp)
> +{
> +    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)) {
> +            if (!user_creatable_add_qapi(opt->opts, errp)) {
> +                return;
> +            }
> +            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 (user_creatable_print_help(type, opts)) {
> +        exit(0);
> +    }

You move help printing from creation to parse time.  Makes sense, but
I'd make it a separate patch, or mention it in the commit message.

> +
> +    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 +1740,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 +1860,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, &error_fatal);

Unlike the old code, the new code removes the entries where
object_create_early() is true from the set of stored -object options.

>  
>      /* spice needs the timers to be initialized by this point */
>      /* spice must initialize before audio as it changes the default auiodev */
> @@ -1846,9 +1889,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 +1902,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, &error_fatal);

Likewise.

Before and after the patch, object_create_late(opt) ==
!object_create_early(opt).  Your patch provides us with an opportunity
to assert this: object_opts must be empty now.  Suggestion, not demand.

>  
>      if (tpm_init() < 0) {
>          exit(1);
> @@ -3398,11 +3439,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"),

Only suggestions, so:
Reviewed-by: Markus Armbruster <armbru@redhat.com>

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

[Paolo Bonzini]

On 13/03/21 10:35, Markus Armbruster wrote:
>> +    object_option_foreach_add(object_create_late, &error_fatal);
> Likewise.
> 
> Before and after the patch, object_create_late(opt) ==
> !object_create_early(opt).  Your patch provides us with an opportunity
> to assert this: object_opts must be empty now.  Suggestion, not demand.
> 

Nice idea, I'll include it.

Paolo

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

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> 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.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  softmmu/vl.c | 27 ++++++++++++++++++---------
>  1 file changed, 18 insertions(+), 9 deletions(-)
>
> diff --git a/softmmu/vl.c b/softmmu/vl.c
> index b245e912e5..7b07f19de7 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"
> @@ -1714,19 +1715,27 @@ 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 (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 (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);

Best viewed with whitespace change ignored:

   commit d13ba69a7cf33f583a22c28644c28928b120aff0
   Author: Paolo Bonzini <pbonzini@redhat.com>
   Date:   Thu Mar 11 12:24:59 2021 -0500

       vl: allow passing JSON to -object

       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.

       Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
       Message-Id: <20210311172459.990281-4-pbonzini@redhat.com>

   diff --git a/softmmu/vl.c b/softmmu/vl.c
   index b245e912e5..7b07f19de7 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"
   @@ -1714,6 +1715,12 @@ static void object_option_parse(const char *optarg)
        const char *type;
        Visitor *v;

   +    if (optarg[0] == '{') {
   +        QObject *obj = qobject_from_json(optarg, &error_fatal);
   +
   +        v = qobject_input_visitor_new(obj);
   +        qobject_unref(obj);
   +    } else {
            opts = qemu_opts_parse_noisily(qemu_find_opts("object"),
                                           optarg, true);
            if (!opts) {
   @@ -1725,8 +1732,10 @@ static void object_option_parse(const char *optarg)
                exit(0);
            }

   -    opt = g_new0(ObjectOption, 1);
            v = opts_visitor_new(opts);
   +    }
   +
   +    opt = g_new0(ObjectOption, 1);
        visit_type_ObjectOptions(v, NULL, &opt->opts, &error_fatal);
        visit_free(v);

Reviewed-by: Markus Armbruster <armbru@redhat.com>

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

[Markus Armbruster]

One more little thing...

Paolo Bonzini <pbonzini@redhat.com> writes:

> 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.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
[...]
> diff --git a/softmmu/vl.c b/softmmu/vl.c
> index ff488ea3e7..b245e912e5 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;
> +

The names feel awkward.

ObjectOption represents a -object option.  Fair enough.

ObjectOptions represents the "options" in its option argument.

Confusing.  Calling the whole thing and one of its parts the same is a
bad idea.

I never liked calling the key=value things in option arguments
"options".  They aren't CLI options, they are optional CLI option
parameters.

I also don't like calling so many different things "object" (QObject,
Object, ObjectOption, ObjectOptions), but that feels out of scope here.

Can we please rename ObjectOptions?

A naming convention for CLI option argument types and boxed QMP command
argument types would be nice.

[...]

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

[Paolo Bonzini]

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

I will use ObjectOptionsQueue like the typedef just above.

Paolo


Il sab 13 mar 2021, 10:57 Markus Armbruster <armbru@redhat.com> ha scritto:

> One more little thing...
>
> Paolo Bonzini <pbonzini@redhat.com> writes:
>
> > 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.
> >
> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> [...]
> > diff --git a/softmmu/vl.c b/softmmu/vl.c
> > index ff488ea3e7..b245e912e5 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;
> > +
>
> The names feel awkward.
>
> ObjectOption represents a -object option.  Fair enough.
>
> ObjectOptions represents the "options" in its option argument.
>
> Confusing.  Calling the whole thing and one of its parts the same is a
> bad idea.
>
> I never liked calling the key=value things in option arguments
> "options".  They aren't CLI options, they are optional CLI option
> parameters.
>
> I also don't like calling so many different things "object" (QObject,
> Object, ObjectOption, ObjectOptions), but that feels out of scope here.
>
> Can we please rename ObjectOptions?
>
> A naming convention for CLI option argument types and boxed QMP command
> argument types would be nice.
>
> [...]
>
>

[-- Attachment #2: Type: text/html, Size: 3397 bytes --]

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

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 13/03/21 10:35, Markus Armbruster wrote:
>>> +    object_option_foreach_add(object_create_late, &error_fatal);
>> Likewise.
>> 
>> Before and after the patch, object_create_late(opt) ==
>> !object_create_early(opt).  Your patch provides us with an opportunity
>> to assert this: object_opts must be empty now.  Suggestion, not demand.
>> 
>
> Nice idea, I'll include it.

Possible alternative:

    static bool object_create_late(const char *type)
    {
        return true;
    }

Your choice.