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

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 25/01/21 10:00, Markus Armbruster wrote:
>> 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?
>
> Back here... No, and that's a feature.  There's no reason to reject 
> those ids.  However, this shows that Kevin's series to move --object to 
> keyval propagates a bug from qemu-storage-daemon to QEMU:
>
> $ storage-daemon/qemu-storage-daemon --object 
> authz-simple,id=123/546,identity=abc --chardev stdio,id=foo --monitor 
> chardev=foo
>  > {'execute':'qmp_capabilities'}
>  > {'execute':'qom-list', 'arguments': {'path':'/objects'}}
> < {"return": [{"name": "type", "type": "string"}, {"name": "123/546", 
> "type": "child<authz-simple>"}]}
>
> Good luck using that object anywhere. :)

There is no reason to reject those IDs other than spoiling the fun we're
having with setting traps for our users.

Since QOM is treating '/' specially in paths, and uses IDs as path
components, it should reject '/' in IDs.  Same reasoning as for file
names.

We already restrict IDs to "letters, digits, '-', '.', '_', starting
with a letter" in several places, including QemuOpts.  We should do that
more, not less.

Permitting arbitrary IDs buys us nothing but trouble.

>> * device_add help,e1000
>> 
>>      {
>>          "e1000": "on",
>>          "driver": "help"
>>      }
>> 
>>    Afterwards:
>>    upstream-qemu: ../util/error.c:59: error_setv: Assertion `*errp == NULL' 
>> failed.
>
> I cannot reproduce it:
>
> $ ./qemu-system-x86_64 -M none -monitor stdio -display none
> QEMU 5.2.50 monitor - type 'help' for more information
> (qemu) device_add help,e1000
> Error: Parameter 'driver' is missing

I'll double-check and report back.