[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 08/25] hmp: replace "O" parser with keyval
From: |
Markus Armbruster |
Subject: |
Re: [PATCH 08/25] hmp: replace "O" parser with keyval |
Date: |
Mon, 25 Jan 2021 10:00:51 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/27.1 (gnu/linux) |
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;
> + 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.
> }
> - qemu_opts_to_qdict(opts, qdict);
> - qemu_opts_del(opts);
> }
> break;
> case '/':
- Re: [PATCH 06/25] tests: convert check-qom-proplist to keyval, (continued)
- [PATCH 10/25] hmp: special case help options for object_add, Paolo Bonzini, 2021/01/18
- [PATCH 15/25] vl: plumb keyval-based options into -set and -readconfig, Paolo Bonzini, 2021/01/18
- [PATCH 16/25] qom: do not modify QDict argument in user_creatable_add_dict, Paolo Bonzini, 2021/01/18
- [PATCH 14/25] qemu-config: parse configuration files to a QDict, Paolo Bonzini, 2021/01/18
- [PATCH 12/25] qemu-config: add error propagation to qemu_config_parse, Paolo Bonzini, 2021/01/18
- [PATCH 13/25] qemu-option: support accept-any QemuOptsList in qemu_opts_absorb_qdict, Paolo Bonzini, 2021/01/18
- [PATCH 08/25] hmp: replace "O" parser with keyval, Paolo Bonzini, 2021/01/18
- Re: [PATCH 08/25] hmp: replace "O" parser with keyval,
Markus Armbruster <=
- [PATCH 17/25] qemu-io: use keyval for -object parsing, Paolo Bonzini, 2021/01/18
- [PATCH 20/25] qemu: use keyval for -object parsing, Paolo Bonzini, 2021/01/18
- [PATCH 21/25] storage-daemon: do not register the "object" group with QemuOpts, Paolo Bonzini, 2021/01/18
- [PATCH 25/25] vl: switch -accel parsing to keyval, Paolo Bonzini, 2021/01/18
- [PATCH 24/25] qemu-option: remove now-dead code, Paolo Bonzini, 2021/01/18
- [PATCH 18/25] qemu-nbd: use keyval for -object parsing, Paolo Bonzini, 2021/01/18
- [PATCH 22/25] qom: export more functions for use with non-UserCreatable objects, Paolo Bonzini, 2021/01/18
- [PATCH 23/25] vl: switch -M parsing to keyval, Paolo Bonzini, 2021/01/18
- [PATCH 19/25] qemu-img: use keyval for -object parsing, Paolo Bonzini, 2021/01/18
- Re: [PATCH 00/25] qemu-option, keyval, vl: switch -object/-M/-accel to keyval parsing, no-reply, 2021/01/18