Paolo Bonzini <pbonz...@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 <pbonz...@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 '/':
