[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PULL 04/12] hmp: Simplify qom-set

From: David Hildenbrand
Subject: Re: [PULL 04/12] hmp: Simplify qom-set
Date: Wed, 3 Jun 2020 14:43:10 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.7.0

On 03.06.20 14:26, David Hildenbrand wrote:
> On 03.06.20 14:24, Dr. David Alan Gilbert wrote:
>> * David Hildenbrand (david@redhat.com) wrote:
>>> On 03.06.20 13:43, Dr. David Alan Gilbert wrote:
>>>> * David Hildenbrand (david@redhat.com) wrote:
>>>>> On 03.06.20 12:43, Dr. David Alan Gilbert wrote:
>>>>>> * David Hildenbrand (david@redhat.com) wrote:
>>>>>>> On 02.06.20 11:26, Dr. David Alan Gilbert wrote:
>>>>>>>> * Markus Armbruster (armbru@redhat.com) wrote:
>>>>>>>>> "Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> writes:
>>>>>>>>>> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>>>>>>>>>> Simplify qom_set by making it use qmp_qom_set and the JSON parser.
>>>>>>>>>> (qemu) qom-get /machine smm
>>>>>>>>>> "auto"
>>>>>>>>>> (qemu) qom-set /machine smm "auto"
>>>>>>>>>> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
>>>>>>>>>> Message-Id: <20200520151108.160598-3-dgilbert@redhat.com>
>>>>>>>>>> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>>>>>>>>>> Reviewed-by: Markus Armbruster <armbru@redhat.com>
>>>>>>>>>> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
>>>>>>>>>>   With 's'->'S' type change suggested by Paolo and Markus
>>>>>>>>> This is actually more than just simplification, it's disarming a bear
>>>>>>>>> trap: the string visitor is restricted to a subset of the QAPI types,
>>>>>>>>> and when you qom-set a property with a type it can't handle, QEMU
>>>>>>>>> aborts.  I mentioned this in the discussion of possible ways out of 
>>>>>>>>> the
>>>>>>>>> qom-get impasse, but missed reraising it in patch review.
>>>>>>>>> A suitably amended commit would be nice, but respinning the PR just 
>>>>>>>>> for
>>>>>>>>> that may not be worthwhile.
>>>>>>>> A bit late; still as long as we're removing bear traps not adding them.
>>>>>>> This breaks qom-set for my (virtio-mem) use case:
>>>>>>> echo "qom-set vm0 requested-size 300M" | sudo nc -U /var/tmp/mon_src
>>>>>>> QEMU 5.0.50 monitor - type 'help' for more information
>>>>>>> (qemu) qom-set vm0 requested-size 300M
>>>>>>> Error: Expecting at most one JSON value
>>>>>> Does qom-set vm0 requested-size 300e6 do the same thing?
>>>>> The property is defined to be of type "size".
>>>>> (qemu) qom-set vm0 requested-size 300e6
>>>>> Error: Parameter 'requested-size' expects uint64
>>>>> (not sure how "size" and "uint64" are mapped here)
>>>> I think the problem here is that the JSON parser is converting anything
>>>> with an 'e' as a float; JSON itself doesn't have the distinction
>>>> between int and float.
>>> (and just to clarify - I assume you are aware - 300e6 != 300M. So the
>>> interface becomes way harder to use in case one wants to specify
>>> properly aligned sizes - 300M vs 314572800)
>> Oops, yes, good point.
>> I think on balance it's probably best that this keeps supporting JSON;
>> although tbh I'm not convinced there are any complex types that can be
>> set.
>> I'm not seeing a prettier answer.
> So, I have to use a calculator from now on to set a property that I can
> set on the QEMU cmdline just fine without it? :(
> This feels like a step backwards, @Markus any way to keep supporting sizes?

Or what about adding qom-set-json instead for complex types instead of
changing the behavior if an existing interface?


David / dhildenb

reply via email to

[Prev in Thread] Current Thread [Next in Thread]