[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, 01 Mar 2021 11:14:01 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/27.1 (gnu/linux) |
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.
- Re: [PATCH 08/25] hmp: replace "O" parser with keyval,
Markus Armbruster <=