* [PATCH 0/3] vl: QAPIfy -object @ 2021-03-11 17:24 Paolo Bonzini 2021-03-11 17:24 ` [PATCH 1/3] tests: convert check-qom-proplist to keyval Paolo Bonzini ` (3 more replies) 0 siblings, 4 replies; 17+ messages in thread From: Paolo Bonzini @ 2021-03-11 17:24 UTC (permalink / raw) To: qemu-devel; +Cc: kwolf, armbru 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 1/3] tests: convert check-qom-proplist to keyval 2021-03-11 17:24 [PATCH 0/3] vl: QAPIfy -object Paolo Bonzini @ 2021-03-11 17:24 ` Paolo Bonzini 2021-03-11 18:29 ` Eric Blake 2021-03-12 10:21 ` Kevin Wolf 2021-03-11 17:24 ` [PATCH 2/3] qom: move user_creatable_add_opts logic to vl.c and QAPIfy it Paolo Bonzini ` (2 subsequent siblings) 3 siblings, 2 replies; 17+ messages in thread From: Paolo Bonzini @ 2021-03-11 17:24 UTC (permalink / raw) To: qemu-devel; +Cc: kwolf, armbru 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 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 1/3] tests: convert check-qom-proplist to keyval 2021-03-11 17:24 ` [PATCH 1/3] tests: convert check-qom-proplist to keyval Paolo Bonzini @ 2021-03-11 18:29 ` Eric Blake 2021-03-12 10:21 ` Kevin Wolf 1 sibling, 0 replies; 17+ messages in thread From: Eric Blake @ 2021-03-11 18:29 UTC (permalink / raw) To: Paolo Bonzini, qemu-devel; +Cc: kwolf, armbru 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/3] tests: convert check-qom-proplist to keyval 2021-03-11 17:24 ` [PATCH 1/3] tests: convert check-qom-proplist to keyval Paolo Bonzini 2021-03-11 18:29 ` Eric Blake @ 2021-03-12 10:21 ` Kevin Wolf 1 sibling, 0 replies; 17+ messages in thread From: Kevin Wolf @ 2021-03-12 10:21 UTC (permalink / raw) To: Paolo Bonzini; +Cc: qemu-devel, armbru 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> ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 2/3] qom: move user_creatable_add_opts logic to vl.c and QAPIfy it 2021-03-11 17:24 [PATCH 0/3] vl: QAPIfy -object Paolo Bonzini 2021-03-11 17:24 ` [PATCH 1/3] tests: convert check-qom-proplist to keyval Paolo Bonzini @ 2021-03-11 17:24 ` Paolo Bonzini 2021-03-11 18:37 ` Eric Blake ` (3 more replies) 2021-03-11 17:24 ` [PATCH 3/3] vl: allow passing JSON to -object Paolo Bonzini 2021-03-11 17:39 ` [PATCH 0/3] vl: QAPIfy -object no-reply 3 siblings, 4 replies; 17+ messages in thread From: Paolo Bonzini @ 2021-03-11 17:24 UTC (permalink / raw) To: qemu-devel; +Cc: kwolf, armbru 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 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 2/3] qom: move user_creatable_add_opts logic to vl.c and QAPIfy it 2021-03-11 17:24 ` [PATCH 2/3] qom: move user_creatable_add_opts logic to vl.c and QAPIfy it Paolo Bonzini @ 2021-03-11 18:37 ` Eric Blake 2021-03-12 10:18 ` Kevin Wolf ` (2 subsequent siblings) 3 siblings, 0 replies; 17+ messages in thread From: Eric Blake @ 2021-03-11 18:37 UTC (permalink / raw) To: Paolo Bonzini, qemu-devel; +Cc: kwolf, armbru 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/3] qom: move user_creatable_add_opts logic to vl.c and QAPIfy it 2021-03-11 17:24 ` [PATCH 2/3] qom: move user_creatable_add_opts logic to vl.c and QAPIfy it Paolo Bonzini 2021-03-11 18:37 ` Eric Blake @ 2021-03-12 10:18 ` Kevin Wolf 2021-03-13 9:35 ` Markus Armbruster 2021-03-13 9:57 ` Markus Armbruster 3 siblings, 0 replies; 17+ messages in thread From: Kevin Wolf @ 2021-03-12 10:18 UTC (permalink / raw) To: Paolo Bonzini; +Cc: qemu-devel, armbru 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/3] qom: move user_creatable_add_opts logic to vl.c and QAPIfy it 2021-03-11 17:24 ` [PATCH 2/3] qom: move user_creatable_add_opts logic to vl.c and QAPIfy it Paolo Bonzini 2021-03-11 18:37 ` Eric Blake 2021-03-12 10:18 ` Kevin Wolf @ 2021-03-13 9:35 ` Markus Armbruster 2021-03-13 9:40 ` Paolo Bonzini 2021-03-13 9:57 ` Markus Armbruster 3 siblings, 1 reply; 17+ messages in thread From: Markus Armbruster @ 2021-03-13 9:35 UTC (permalink / raw) To: Paolo Bonzini; +Cc: kwolf, qemu-devel 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> ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/3] qom: move user_creatable_add_opts logic to vl.c and QAPIfy it 2021-03-13 9:35 ` Markus Armbruster @ 2021-03-13 9:40 ` Paolo Bonzini 2021-03-13 12:32 ` Markus Armbruster 0 siblings, 1 reply; 17+ messages in thread From: Paolo Bonzini @ 2021-03-13 9:40 UTC (permalink / raw) To: Markus Armbruster; +Cc: kwolf, qemu-devel 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/3] qom: move user_creatable_add_opts logic to vl.c and QAPIfy it 2021-03-13 9:40 ` Paolo Bonzini @ 2021-03-13 12:32 ` Markus Armbruster 0 siblings, 0 replies; 17+ messages in thread From: Markus Armbruster @ 2021-03-13 12:32 UTC (permalink / raw) To: Paolo Bonzini; +Cc: kwolf, qemu-devel 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. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/3] qom: move user_creatable_add_opts logic to vl.c and QAPIfy it 2021-03-11 17:24 ` [PATCH 2/3] qom: move user_creatable_add_opts logic to vl.c and QAPIfy it Paolo Bonzini ` (2 preceding siblings ...) 2021-03-13 9:35 ` Markus Armbruster @ 2021-03-13 9:57 ` Markus Armbruster 2021-03-13 10:05 ` Paolo Bonzini 3 siblings, 1 reply; 17+ messages in thread From: Markus Armbruster @ 2021-03-13 9:57 UTC (permalink / raw) To: Paolo Bonzini; +Cc: kwolf, qemu-devel 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. [...] ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/3] qom: move user_creatable_add_opts logic to vl.c and QAPIfy it 2021-03-13 9:57 ` Markus Armbruster @ 2021-03-13 10:05 ` Paolo Bonzini 0 siblings, 0 replies; 17+ messages in thread From: Paolo Bonzini @ 2021-03-13 10:05 UTC (permalink / raw) To: Markus Armbruster; +Cc: Wolf, Kevin, qemu-devel [-- 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 --] ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 3/3] vl: allow passing JSON to -object 2021-03-11 17:24 [PATCH 0/3] vl: QAPIfy -object Paolo Bonzini 2021-03-11 17:24 ` [PATCH 1/3] tests: convert check-qom-proplist to keyval Paolo Bonzini 2021-03-11 17:24 ` [PATCH 2/3] qom: move user_creatable_add_opts logic to vl.c and QAPIfy it Paolo Bonzini @ 2021-03-11 17:24 ` Paolo Bonzini 2021-03-11 18:38 ` Eric Blake ` (2 more replies) 2021-03-11 17:39 ` [PATCH 0/3] vl: QAPIfy -object no-reply 3 siblings, 3 replies; 17+ messages in thread From: Paolo Bonzini @ 2021-03-11 17:24 UTC (permalink / raw) To: qemu-devel; +Cc: kwolf, armbru 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 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 3/3] vl: allow passing JSON to -object 2021-03-11 17:24 ` [PATCH 3/3] vl: allow passing JSON to -object Paolo Bonzini @ 2021-03-11 18:38 ` Eric Blake 2021-03-12 10:21 ` Kevin Wolf 2021-03-13 9:41 ` Markus Armbruster 2 siblings, 0 replies; 17+ messages in thread From: Eric Blake @ 2021-03-11 18:38 UTC (permalink / raw) To: Paolo Bonzini, qemu-devel; +Cc: kwolf, armbru 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 3/3] vl: allow passing JSON to -object 2021-03-11 17:24 ` [PATCH 3/3] vl: allow passing JSON to -object Paolo Bonzini 2021-03-11 18:38 ` Eric Blake @ 2021-03-12 10:21 ` Kevin Wolf 2021-03-13 9:41 ` Markus Armbruster 2 siblings, 0 replies; 17+ messages in thread From: Kevin Wolf @ 2021-03-12 10:21 UTC (permalink / raw) To: Paolo Bonzini; +Cc: qemu-devel, armbru 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> ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 3/3] vl: allow passing JSON to -object 2021-03-11 17:24 ` [PATCH 3/3] vl: allow passing JSON to -object Paolo Bonzini 2021-03-11 18:38 ` Eric Blake 2021-03-12 10:21 ` Kevin Wolf @ 2021-03-13 9:41 ` Markus Armbruster 2 siblings, 0 replies; 17+ messages in thread From: Markus Armbruster @ 2021-03-13 9:41 UTC (permalink / raw) To: Paolo Bonzini; +Cc: kwolf, qemu-devel 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> ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 0/3] vl: QAPIfy -object 2021-03-11 17:24 [PATCH 0/3] vl: QAPIfy -object Paolo Bonzini ` (2 preceding siblings ...) 2021-03-11 17:24 ` [PATCH 3/3] vl: allow passing JSON to -object Paolo Bonzini @ 2021-03-11 17:39 ` no-reply 3 siblings, 0 replies; 17+ messages in thread From: no-reply @ 2021-03-11 17:39 UTC (permalink / raw) To: pbonzini; +Cc: kwolf, qemu-devel, armbru 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2021-03-13 12:33 UTC | newest] Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-03-11 17:24 [PATCH 0/3] vl: QAPIfy -object Paolo Bonzini 2021-03-11 17:24 ` [PATCH 1/3] tests: convert check-qom-proplist to keyval Paolo Bonzini 2021-03-11 18:29 ` Eric Blake 2021-03-12 10:21 ` Kevin Wolf 2021-03-11 17:24 ` [PATCH 2/3] qom: move user_creatable_add_opts logic to vl.c and QAPIfy it Paolo Bonzini 2021-03-11 18:37 ` Eric Blake 2021-03-12 10:18 ` Kevin Wolf 2021-03-13 9:35 ` Markus Armbruster 2021-03-13 9:40 ` Paolo Bonzini 2021-03-13 12:32 ` Markus Armbruster 2021-03-13 9:57 ` Markus Armbruster 2021-03-13 10:05 ` Paolo Bonzini 2021-03-11 17:24 ` [PATCH 3/3] vl: allow passing JSON to -object Paolo Bonzini 2021-03-11 18:38 ` Eric Blake 2021-03-12 10:21 ` Kevin Wolf 2021-03-13 9:41 ` Markus Armbruster 2021-03-11 17:39 ` [PATCH 0/3] vl: QAPIfy -object no-reply
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.