Markus Armbruster <arm...@redhat.com> writes:
> 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;
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 '/':
