From: Markus Armbruster <armbru@redhat.com>
To: Markus Armbruster <armbru@redhat.com>
Cc: kwolf@redhat.com, Paolo Bonzini <pbonzini@redhat.com>,
qemu-devel@nongnu.org, imammedo@redhat.com
Subject: Re: [PATCH 08/25] hmp: replace "O" parser with keyval
Date: Mon, 01 Mar 2021 11:43:38 +0100 [thread overview]
Message-ID: <87k0qrjgrp.fsf@dusky.pond.sub.org> (raw)
In-Reply-To: <87zh0xo0fw.fsf@dusky.pond.sub.org> (Markus Armbruster's message of "Mon, 25 Jan 2021 10:00:51 +0100")
Markus Armbruster <armbru@redhat.com> writes:
> Paolo Bonzini <pbonzini@redhat.com> writes:
>
>> HMP is using QemuOpts to parse free-form commands device_add,
>> netdev_add and object_add. However, none of these need QemuOpts
>> for validation (these three QemuOptsLists are all of the catch-all
>> kind), and keyval is already able to parse into QDict. So use
>> keyval directly, avoiding the detour from
>> string to QemuOpts to QDict.
>>
>> The args_type now stores the implied key. This arguably makes more
>> sense than storing the QemuOptsList name; at least, it _is_ a key
>> that might end up in the arguments QDict.
>>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>
> Switching from QemuOpts to keyval changes the accepted language. We may
> change it, because HMP is not a stable interface. The commit message
> should point out the change, though. Maybe even release notes, not
> sure.
>
> Let's recap the differences briefly:
>
> * Boolean sugar: deprecated in QemuOpts, nonexistent in keyval
>
> * QemuOpts accepts a number of more or less crazy corner cases keyval
> rejects: invalid key, long key (silently truncated), first rather than
> last id= wins (unlike other keys), implied key with empty value.
>
> * QemuOpts rejects anti-social ID such as id=666, keyval leaves this to
> the caller, because key "id" is not special in keyval.
>
> Are these still rejected with your patch?
>
>> ---
>> hmp-commands.hx | 6 +++---
>> monitor/hmp.c | 20 +++++++++-----------
>> 2 files changed, 12 insertions(+), 14 deletions(-)
>>
>> diff --git a/hmp-commands.hx b/hmp-commands.hx
>> index 73e0832ea1..6ee746b53e 100644
>> --- a/hmp-commands.hx
>> +++ b/hmp-commands.hx
>> @@ -669,7 +669,7 @@ ERST
>>
>> {
>> .name = "device_add",
>> - .args_type = "device:O",
>> + .args_type = "driver:O",
>> .params = "driver[,prop=value][,...]",
>> .help = "add device, like -device on the command line",
>> .cmd = hmp_device_add,
>> @@ -1315,7 +1315,7 @@ ERST
>>
>> {
>> .name = "netdev_add",
>> - .args_type = "netdev:O",
>> + .args_type = "type:O",
>> .params = "[user|tap|socket|vde|bridge|hubport|netmap|vhost-user],id=str[,prop=value][,...]",
>> .help = "add host network device",
>> .cmd = hmp_netdev_add,
>> @@ -1343,7 +1343,7 @@ ERST
>>
>> {
>> .name = "object_add",
>> - .args_type = "object:O",
>> + .args_type = "qom-type:O",
>> .params = "[qom-type=]type,id=str[,prop=value][,...]",
>> .help = "create QOM object",
>> .cmd = hmp_object_add,
>> diff --git a/monitor/hmp.c b/monitor/hmp.c
>> index 6c0b33a0b1..d2cb886da5 100644
>> --- a/monitor/hmp.c
>> +++ b/monitor/hmp.c
>> @@ -744,13 +744,9 @@ static QDict *monitor_parse_arguments(Monitor *mon,
>> break;
>> case 'O':
>> {
>> - QemuOptsList *opts_list;
>> - QemuOpts *opts;
>> + Error *errp;
Missing initializer. This is what causes the assertion failure reported
below.
>> + bool help;
>>
>> - opts_list = qemu_find_opts(key);
>> - if (!opts_list || opts_list->desc->name) {
>> - goto bad_type;
>> - }
>> while (qemu_isspace(*p)) {
>> p++;
>> }
>> @@ -760,12 +756,14 @@ static QDict *monitor_parse_arguments(Monitor *mon,
>> if (get_str(buf, sizeof(buf), &p) < 0) {
>> goto fail;
>> }
>> - opts = qemu_opts_parse_noisily(opts_list, buf, true);
>> - if (!opts) {
>> - goto fail;
>> + keyval_parse_into(qdict, buf, key, &help, &errp);
>> + if (help) {
>
> Uh...
>
>> + if (qdict_haskey(qdict, key)) {
>
> If we parsed a value for the implied key (sugared or not),
>
>> + qdict_put_bool(qdict, "help", true);
>
> then encode the help request by mapping key "help" to true,
>
>> + } else {
>> + qdict_put_str(qdict, key, "help");
>
> else by mapping the implied key to "help".
>
>> + }
>
> Test cases:
>
> * device_add help
>
> @qdict before the patch:
>
> {
> "driver": "help"
> }
>
> No change.
>
> * device_add e1000,help
>
> @qdict before the patch:
>
> {
> "driver": "e1000",
> "help": "on"
> }
>
> Afterwards:
>
> {
> "driver": "e1000",
> "help": true
> }
>
> If this is okay, the commit message should explain it.
>
> * device_add help,e1000
>
> {
> "e1000": "on",
> "driver": "help"
> }
>
> Afterwards:
> upstream-qemu: ../util/error.c:59: error_setv: Assertion `*errp == NULL' failed.
Optimization masks this crash for me.
With proper initialization, I get
{
"driver": "help"
}
instead. If this change is okay, the commit message should explain it.
>
>> }
>> - qemu_opts_to_qdict(opts, qdict);
>> - qemu_opts_del(opts);
>> }
>> break;
>> case '/':
next prev parent reply other threads:[~2021-03-01 10:44 UTC|newest]
Thread overview: 67+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-01-18 16:30 [PATCH 00/25] qemu-option, keyval, vl: switch -object/-M/-accel to keyval parsing Paolo Bonzini
2021-01-18 16:30 ` [PATCH 01/25] qemu-option: clean up id vs. list->merge_lists Paolo Bonzini
2021-01-19 12:33 ` Kevin Wolf
2021-01-19 13:58 ` Markus Armbruster
2021-01-19 14:20 ` Paolo Bonzini
2021-01-20 8:03 ` Markus Armbruster
2021-01-20 12:37 ` Paolo Bonzini
2021-01-20 12:50 ` Markus Armbruster
2021-01-18 16:30 ` [PATCH 02/25] qemu-option: move help handling to get_opt_name_value Paolo Bonzini
2021-01-19 15:10 ` Markus Armbruster
2021-01-18 16:30 ` [PATCH 03/25] qemu-option: warn for short-form boolean options Paolo Bonzini
2021-01-19 15:56 ` Markus Armbruster
2021-01-19 17:04 ` Paolo Bonzini
2021-01-20 8:42 ` Markus Armbruster
2021-01-20 12:40 ` Paolo Bonzini
2021-01-20 12:59 ` Markus Armbruster
2021-01-20 14:05 ` Paolo Bonzini
2021-01-18 16:30 ` [PATCH 04/25] keyval: accept escaped commas in implied option Paolo Bonzini
2021-01-21 12:58 ` Markus Armbruster
2021-01-22 8:39 ` Markus Armbruster
2021-01-18 16:30 ` [PATCH 05/25] keyval: simplify keyval_parse_one Paolo Bonzini
2021-01-22 13:48 ` Markus Armbruster
2021-01-22 15:00 ` Paolo Bonzini
2021-01-22 15:44 ` Markus Armbruster
2021-01-18 16:30 ` [PATCH 06/25] tests: convert check-qom-proplist to keyval Paolo Bonzini
2021-01-22 14:14 ` Markus Armbruster
2021-01-22 14:38 ` Paolo Bonzini
2021-01-22 14:48 ` Paolo Bonzini
2021-01-18 16:30 ` [PATCH 07/25] keyval: introduce keyval_parse_into Paolo Bonzini
2021-01-22 14:22 ` Markus Armbruster
2021-01-22 14:30 ` Paolo Bonzini
2021-01-18 16:30 ` [PATCH 08/25] hmp: replace "O" parser with keyval Paolo Bonzini
2021-01-25 9:00 ` Markus Armbruster
2021-02-26 11:25 ` Paolo Bonzini
2021-03-01 10:14 ` Markus Armbruster
2021-03-01 10:23 ` Paolo Bonzini
2021-03-01 13:35 ` Markus Armbruster
2021-03-01 10:43 ` Markus Armbruster [this message]
2021-03-01 11:54 ` Paolo Bonzini
2021-01-18 16:30 ` [PATCH 09/25] qom: use qemu_printf to print help for user-creatable objects Paolo Bonzini
2021-01-25 12:47 ` Markus Armbruster
2021-01-18 16:30 ` [PATCH 10/25] hmp: special case help options for object_add Paolo Bonzini
2021-01-25 12:48 ` Markus Armbruster
2021-01-25 12:49 ` Paolo Bonzini
2021-01-25 14:02 ` Markus Armbruster
2021-01-18 16:30 ` [PATCH 11/25] remove -writeconfig Paolo Bonzini
2021-01-25 12:53 ` Markus Armbruster
2021-01-25 14:01 ` Paolo Bonzini
2021-01-25 14:12 ` Daniel P. Berrangé
2021-01-18 16:31 ` [PATCH 12/25] qemu-config: add error propagation to qemu_config_parse Paolo Bonzini
2021-01-25 13:54 ` Markus Armbruster
2021-01-18 16:31 ` [PATCH 13/25] qemu-option: support accept-any QemuOptsList in qemu_opts_absorb_qdict Paolo Bonzini
2021-01-18 16:31 ` [PATCH 14/25] qemu-config: parse configuration files to a QDict Paolo Bonzini
2021-01-18 16:31 ` [PATCH 15/25] vl: plumb keyval-based options into -set and -readconfig Paolo Bonzini
2021-01-25 11:48 ` Markus Armbruster
2021-01-25 13:59 ` Paolo Bonzini
2021-01-18 16:31 ` [PATCH 16/25] qom: do not modify QDict argument in user_creatable_add_dict Paolo Bonzini
2021-01-18 16:31 ` [PATCH 17/25] qemu-io: use keyval for -object parsing Paolo Bonzini
2021-01-18 16:31 ` [PATCH 18/25] qemu-nbd: " Paolo Bonzini
2021-01-18 16:31 ` [PATCH 19/25] qemu-img: " Paolo Bonzini
2021-01-18 16:31 ` [PATCH 20/25] qemu: " Paolo Bonzini
2021-01-18 16:31 ` [PATCH 21/25] storage-daemon: do not register the "object" group with QemuOpts Paolo Bonzini
2021-01-18 16:31 ` [PATCH 22/25] qom: export more functions for use with non-UserCreatable objects Paolo Bonzini
2021-01-18 16:31 ` [PATCH 23/25] vl: switch -M parsing to keyval Paolo Bonzini
2021-01-18 16:31 ` [PATCH 24/25] qemu-option: remove now-dead code Paolo Bonzini
2021-01-18 16:31 ` [PATCH 25/25] vl: switch -accel parsing to keyval Paolo Bonzini
2021-01-18 17:18 ` [PATCH 00/25] qemu-option, keyval, vl: switch -object/-M/-accel to keyval parsing no-reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87k0qrjgrp.fsf@dusky.pond.sub.org \
--to=armbru@redhat.com \
--cc=imammedo@redhat.com \
--cc=kwolf@redhat.com \
--cc=pbonzini@redhat.com \
--cc=qemu-devel@nongnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).