Re: [PATCH 2/2] hmp: Simplify qom_set

[Philippe Mathieu-Daudé]

On 5/28/20 4:06 PM, Dr. David Alan Gilbert wrote:
> * Markus Armbruster (armbru@redhat.com) wrote:
>> "Dr. David Alan Gilbert" <dgilbert@redhat.com> writes:
>>
>>> * Philippe Mathieu-Daudé (philmd@redhat.com) wrote:
>>>> On 5/20/20 5:11 PM, Dr. David Alan Gilbert (git) wrote:
>>>>> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>>>>>
>>>>> Simplify qom_set by making it use qmp_qom_set and the JSON parser.
>>>>>
>>>>> Note that qom-set likes JSON strings quoted with ' not ", e.g.:
>>>>>
>>>>> (qemu) qom-get /machine smm
>>>>> "auto"
>>>>> (qemu) qom-set /machine smm 'auto'
>>>>
>>>> Will I get this output using "?
>>>>
>>>> (qemu) qom-set /machine smm "auto"
>>>> Error: Expecting a JSON value
>>>
>>> The error you get is:
>>>
>>> (qemu) qom-set /machine smm "auto"
>>> Error: JSON parse error, invalid keyword 'auto'
>>>
>>> I think, having seen alphanumerics, it's expecting a keyword;
>>> i.e. a true/false making a bool, or a null.
>>
>> The command parses its argument as JSON.
>>
>> Before we get there, the HMP core extracts the argument from the line of
>> input.  The extraction is guided by the command's .args_type, in this
>> case the 's' in "value:s" in
>>
>>     {
>>         .name       = "qom-set",
>>         .args_type  = "path:s,property:s,value:s",
>>         [...]
>>     },
>>
>> monitor/monitor-internal.h documents type code 's' as
>>
>>  * 's'          string (accept optional quote)
>>
>> The implementation boils down to:
>>
>> 1. Skip whitespace.
>>
>> 2. If looking at '"', get the string enclosed in '"', with C-like escape
>>    sequences \n, \r, \\, \', \".
>>
>> 3. Else, get the string up to the next whitespace.
>>
>> See get_str().
>>
>> Therefore, argument "auto" is the same as auto.  Parsing auto as JSON
>> duly fails.
>>
>> Argument 'auto' works, but only because qobject_from_json() recognizes
>> single-quoted strings.  This is as extension over RFC 8259.
>>
>> Using single quotes falls apart when you want to pass something
>> containing whitespace.  Then you'd have to use
>>
>>     "\"ugly and unintuitive\""
>>
>> or, again relying on the extension
>>
>>     "'ugly and unintuitive'"
>>
>> There's a better way, and Paolo pointed it out in
>>
>>     Subject: Re: [RFC PATCH] qom: Implement qom-get HMP command
>>     Date: Thu, 21 May 2020 16:24:12 +0200
>>     Message-ID: <2c148331-78ae-31f7-8702-d65c37a090b0@redhat.com>
>>
>> Use argument type 'S'.  Documented as
>>
>>  * 'S'          it just appends the rest of the string (accept optional 
>> quote)
>>
>> but the parenthesis is confusing.  It really just skips whitespace, then
>> extracts the remainder of the line.  Can't do string with leading
>> whitespace, but that's just fine for us.
> 

Thanks for the detailed explanation.

> Yep, thanks - I spotted Paolo's response but thanks for the deeper
> explanation.
> With that would you give me a Review-by?

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> 
> Dave
> 
>> Please squash in:
>>
>> diff --git a/hmp-commands.hx b/hmp-commands.hx
>> index 250ddae54d..28256209b5 100644
>> --- a/hmp-commands.hx
>> +++ b/hmp-commands.hx
>> @@ -1806,7 +1806,7 @@ ERST
>>  
>>      {
>>          .name       = "qom-set",
>> -        .args_type  = "path:s,property:s,value:s",
>> +        .args_type  = "path:s,property:s,value:S",
>>          .params     = "path property value",
>>          .help       = "set QOM property",
>>          .cmd        = hmp_qom_set,
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
>