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

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

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

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.

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

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

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

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.

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