Re: [PATCH 08/25] hmp: replace "O" parser with keyval

[Markus Armbruster]

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 '/':