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>