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

[Dr. David Alan Gilbert]

* 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.

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

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