Re: [PATCH v2] hmp: Make json format optional for qom-set

[Markus Armbruster]

David Hildenbrand <david@redhat.com> writes:

> On 15.06.20 08:17, Markus Armbruster wrote:
>> David Hildenbrand <david@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.fsf@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.