David Hildenbrand <da...@redhat.com> writes:
> On 15.06.20 08:17, Markus Armbruster wrote:
>> David Hildenbrand <da...@redhat.com> writes:
>>
>>> Commit 7d2ef6dcc1cf ("hmp: Simplify qom-set") switched to the json
>>> parser, making it possible to specify complex types. However, with this
>>> change it is no longer possible to specify proper sizes (e.g., 2G, 128M),
>>> turning the interface harder to use for properties that consume sizes.
>>>
>>> Let's switch back to the previous handling and allow to specify passing
>>> json via the "-j" parameter.
>>
>> Two issues:
>>
>> 1. Makes qom-get and qom-set inconsistent
>>
>> qom-get formats as JSON, always.
>>
>> qom-set parses the string visitor's undocumented ad hoc language by
>> default. You can make it parse JSON by passing -j.
>
> This is the same language the QEMU cmdline uses, no?
The CLI uses many, many languages. The string visitor's language may
well be among them; can't tell offhand.
>
>>
>> Not a show stopper, but sure ugly. I feel documentation should point
>> it out.
>
> Sure, we can fine-tune the documentation. For now we didn't have any
> qom-get users, in contrast to qom-set. Not sure if it makes sense to
> implement the same functionality for qom-get.
>
> For now I can e.g.,
>
> "echo "qom-set vm1 requested-size 256M" | sudo nc -U /var/tmp/mon_src"
>
> then I can
>
> echo "qom-get vm1 requested-size " | sudo nc -U /var/tmp/mon_src
> -> 268435456
>
> which is a value I can punch back into qom-set. At least for sizes this
> works. Not perfect, not bad. Opinions?
It happens to work in this case, because the JSON number returned by
qom-get happens to get parsed the right way by qom-set.
Is this the case for all properties where qom-set isn't deadly due to
issue 2.? Nobody knows.
>> 2. Rearms the string visitor death trap
>>
>> If you try to qom-set a property whose ->set() uses something the
>> string input visitor doesn't support, QEMU crashes. I'm not aware of
>> such a ->set(), but this is a death trap all the same. Mind, I
>> didn't actually *look* for such a ->set(). Details:
>
> Thanks. Maybe I am missing something important, but this sounds like we
> are missing a bunch of checks+errors.
The string visitor feels like a quick hack to get something that is
human-friendly. It provides just enough functionality for its initial
uses. The trouble is new uses that violate its restrictions are hard to
spot.
In my opinion, what we're really missing a replacement of the
ill-conceived string visitor. The less it's used, the better.
Since a replacement isn't being worked on, we may have to make it less
dangerous to use. Patches welcome.
> (wouldn't we be able to crash
> using the QEMU cmdline as well when setting such properties?).
If the string visitor is used there. Nobody knows.
>> Subject: Re: [RFC PATCH] qom: Implement qom-get HMP command
>> Date: Sat, 02 May 2020 08:02:43 +0200 (6 weeks, 2 days, 4 minutes ago)
>> Message-ID: <87a72q6fi4....@dusky.pond.sub.org>
>> https://lists.nongnu.org/archive/html/qemu-devel/2020-05/msg00178.html
>>
>> Since we've had this death trap in the code for a number of years, I
>> can't call its restoration a show stopper. It does feel like an
>> unadvisable risk, though.
>>
>
> As long as there are no better alternatives to punch in data in the same
> format the QEMU cmdline consumes, I think this is perfectly reasonable.
> No good reason to make a HMP interface harder to use by humans IMHO.
Yes, HMP should be human-friendly. Not at any cost, though; I reiterate
my conviction that this is an unadvisable risk.
A crash is the most unfriendly response of all.
