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

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

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.

[...]

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

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>

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

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.