Paolo Bonzini <pbonz...@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 <pbonz...@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 <arm...@redhat.com>
