[PATCH 0/2] HMP: qom-get and set

[PATCH 0/2] HMP: qom-get and set

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

HMP has had a qom-set for a long time, but not a matching qom-get;
various attempts have been made to add one over the last 5 years.
Here's another go.

It's got simpler due to a suggestion by Markus to take the output
from qmp's qom-get.  While we're here, rework hmp's qom-set to be
a wrapper around the qmp equivalent, which simplifies it a bit.

In one post people didn't particularly like the use of JSON,
but in the intervening 4 years no one has implemented anything to
avoid it, and the output is trivially readable for non-structures
and still pretty readable for (the very rare) structures.

Dave

Dr. David Alan Gilbert (2):
  hmp: Implement qom-get HMP command
  hmp: Simplify qom_set

 hmp-commands.hx        | 14 ++++++++++++++
 include/monitor/hmp.h  |  1 +
 qom/qom-hmp-cmds.c     | 34 +++++++++++++++++++++++-----------
 tests/qtest/test-hmp.c |  1 +
 4 files changed, 39 insertions(+), 11 deletions(-)

-- 
2.26.2

[PATCH 1/2] hmp: Implement qom-get HMP command

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

This started off as Andreas Färber's implementation from
March 2015, but after feedback from Paolo and Markus it morphed into
using the json output which handles structs reasonably.

Use with qom-list to find the members of an object.

(qemu) qom-get /backend/console[0]/device/vga.rom[0] size
65536
(qemu) qom-get /machine smm
"auto"
(qemu) qom-get /machine rtc-time
{
    "tm_year": 120,
    "tm_sec": 51,
    "tm_hour": 9,
    "tm_min": 50,
    "tm_mon": 4,
    "tm_mday": 20
}
(qemu) qom-get /machine frob
Error: Property '.frob' not found

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 hmp-commands.hx        | 14 ++++++++++++++
 include/monitor/hmp.h  |  1 +
 qom/qom-hmp-cmds.c     | 18 ++++++++++++++++++
 tests/qtest/test-hmp.c |  1 +
 4 files changed, 34 insertions(+)

diff --git a/hmp-commands.hx b/hmp-commands.hx
index 7f0f3974ad..250ddae54d 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -1790,6 +1790,20 @@ SRST
   Print QOM properties of object at location *path*
 ERST
 
+    {
+        .name       = "qom-get",
+        .args_type  = "path:s,property:s",
+        .params     = "path property",
+        .help       = "print QOM property",
+        .cmd        = hmp_qom_get,
+        .flags      = "p",
+    },
+
+SRST
+``qom-get`` *path* *property*
+  Print QOM property *property* of object at location *path*
+ERST
+
     {
         .name       = "qom-set",
         .args_type  = "path:s,property:s,value:s",
diff --git a/include/monitor/hmp.h b/include/monitor/hmp.h
index e33ca5a911..c986cfd28b 100644
--- a/include/monitor/hmp.h
+++ b/include/monitor/hmp.h
@@ -96,6 +96,7 @@ void hmp_info_memdev(Monitor *mon, const QDict *qdict);
 void hmp_info_numa(Monitor *mon, const QDict *qdict);
 void hmp_info_memory_devices(Monitor *mon, const QDict *qdict);
 void hmp_qom_list(Monitor *mon, const QDict *qdict);
+void hmp_qom_get(Monitor *mon, const QDict *qdict);
 void hmp_qom_set(Monitor *mon, const QDict *qdict);
 void hmp_info_qom_tree(Monitor *mon, const QDict *dict);
 void object_add_completion(ReadLineState *rs, int nb_args, const char *str);
diff --git a/qom/qom-hmp-cmds.c b/qom/qom-hmp-cmds.c
index cd08233a4c..a8b0a080c7 100644
--- a/qom/qom-hmp-cmds.c
+++ b/qom/qom-hmp-cmds.c
@@ -12,6 +12,8 @@
 #include "qapi/error.h"
 #include "qapi/qapi-commands-qom.h"
 #include "qapi/qmp/qdict.h"
+#include "qapi/qmp/qjson.h"
+#include "qapi/qmp/qstring.h"
 #include "qom/object.h"
 
 void hmp_qom_list(Monitor *mon, const QDict *qdict)
@@ -62,6 +64,22 @@ void hmp_qom_set(Monitor *mon, const QDict *qdict)
     hmp_handle_error(mon, err);
 }
 
+void hmp_qom_get(Monitor *mon, const QDict *qdict)
+{
+    const char *path = qdict_get_str(qdict, "path");
+    const char *property = qdict_get_str(qdict, "property");
+    Error *err = NULL;
+    QObject *obj = qmp_qom_get(path, property, &err);
+
+    if (err == NULL) {
+        QString *str = qobject_to_json_pretty(obj);
+        monitor_printf(mon, "%s\n", qstring_get_str(str));
+        qobject_unref(str);
+    }
+
+    hmp_handle_error(mon, err);
+}
+
 typedef struct QOMCompositionState {
     Monitor *mon;
     int indent;
diff --git a/tests/qtest/test-hmp.c b/tests/qtest/test-hmp.c
index f8aa5f92c5..b8b1271b9e 100644
--- a/tests/qtest/test-hmp.c
+++ b/tests/qtest/test-hmp.c
@@ -61,6 +61,7 @@ static const char *hmp_cmds[] = {
     "p $pc + 8",
     "qom-list /",
     "qom-set /machine initrd test",
+    "qom-get /machine initrd",
     "screendump /dev/null",
     "sendkey x",
     "singlestep on",
-- 
2.26.2

[PATCH 2/2] hmp: Simplify qom_set

[Dr. David Alan Gilbert (git)]

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'

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 qom/qom-hmp-cmds.c | 16 +++++-----------
 1 file changed, 5 insertions(+), 11 deletions(-)

diff --git a/qom/qom-hmp-cmds.c b/qom/qom-hmp-cmds.c
index a8b0a080c7..f704b6949a 100644
--- a/qom/qom-hmp-cmds.c
+++ b/qom/qom-hmp-cmds.c
@@ -48,19 +48,13 @@ void hmp_qom_set(Monitor *mon, const QDict *qdict)
     const char *property = qdict_get_str(qdict, "property");
     const char *value = qdict_get_str(qdict, "value");
     Error *err = NULL;
-    bool ambiguous = false;
-    Object *obj;
+    QObject *obj;
 
-    obj = object_resolve_path(path, &ambiguous);
-    if (obj == NULL) {
-        error_set(&err, ERROR_CLASS_DEVICE_NOT_FOUND,
-                  "Device '%s' not found", path);
-    } else {
-        if (ambiguous) {
-            monitor_printf(mon, "Warning: Path '%s' is ambiguous\n", path);
-        }
-        object_property_parse(obj, value, property, &err);
+    obj = qobject_from_json(value, &err);
+    if (err == NULL) {
+        qmp_qom_set(path, property, obj, &err);
     }
+
     hmp_handle_error(mon, err);
 }
 
-- 
2.26.2

Re: [PATCH 1/2] hmp: Implement qom-get HMP command

[Philippe Mathieu-Daudé]

On 5/20/20 5:11 PM, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> 
> This started off as Andreas Färber's implementation from
> March 2015, but after feedback from Paolo and Markus it morphed into
> using the json output which handles structs reasonably.
> 
> Use with qom-list to find the members of an object.
> 
> (qemu) qom-get /backend/console[0]/device/vga.rom[0] size
> 65536
> (qemu) qom-get /machine smm
> "auto"
> (qemu) qom-get /machine rtc-time
> {
>      "tm_year": 120,
>      "tm_sec": 51,
>      "tm_hour": 9,
>      "tm_min": 50,
>      "tm_mon": 4,
>      "tm_mday": 20
> }
> (qemu) qom-get /machine frob
> Error: Property '.frob' not found
> 
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> ---
>   hmp-commands.hx        | 14 ++++++++++++++
>   include/monitor/hmp.h  |  1 +
>   qom/qom-hmp-cmds.c     | 18 ++++++++++++++++++
>   tests/qtest/test-hmp.c |  1 +
>   4 files changed, 34 insertions(+)
> 
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index 7f0f3974ad..250ddae54d 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -1790,6 +1790,20 @@ SRST
>     Print QOM properties of object at location *path*
>   ERST
>   
> +    {
> +        .name       = "qom-get",
> +        .args_type  = "path:s,property:s",
> +        .params     = "path property",
> +        .help       = "print QOM property",
> +        .cmd        = hmp_qom_get,
> +        .flags      = "p",
> +    },
> +
> +SRST
> +``qom-get`` *path* *property*
> +  Print QOM property *property* of object at location *path*
> +ERST
> +
>       {
>           .name       = "qom-set",
>           .args_type  = "path:s,property:s,value:s",
> diff --git a/include/monitor/hmp.h b/include/monitor/hmp.h
> index e33ca5a911..c986cfd28b 100644
> --- a/include/monitor/hmp.h
> +++ b/include/monitor/hmp.h
> @@ -96,6 +96,7 @@ void hmp_info_memdev(Monitor *mon, const QDict *qdict);
>   void hmp_info_numa(Monitor *mon, const QDict *qdict);
>   void hmp_info_memory_devices(Monitor *mon, const QDict *qdict);
>   void hmp_qom_list(Monitor *mon, const QDict *qdict);
> +void hmp_qom_get(Monitor *mon, const QDict *qdict);
>   void hmp_qom_set(Monitor *mon, const QDict *qdict);
>   void hmp_info_qom_tree(Monitor *mon, const QDict *dict);
>   void object_add_completion(ReadLineState *rs, int nb_args, const char *str);
> diff --git a/qom/qom-hmp-cmds.c b/qom/qom-hmp-cmds.c
> index cd08233a4c..a8b0a080c7 100644
> --- a/qom/qom-hmp-cmds.c
> +++ b/qom/qom-hmp-cmds.c
> @@ -12,6 +12,8 @@
>   #include "qapi/error.h"
>   #include "qapi/qapi-commands-qom.h"
>   #include "qapi/qmp/qdict.h"
> +#include "qapi/qmp/qjson.h"
> +#include "qapi/qmp/qstring.h"
>   #include "qom/object.h"
>   
>   void hmp_qom_list(Monitor *mon, const QDict *qdict)
> @@ -62,6 +64,22 @@ void hmp_qom_set(Monitor *mon, const QDict *qdict)
>       hmp_handle_error(mon, err);
>   }
>   
> +void hmp_qom_get(Monitor *mon, const QDict *qdict)
> +{
> +    const char *path = qdict_get_str(qdict, "path");
> +    const char *property = qdict_get_str(qdict, "property");
> +    Error *err = NULL;
> +    QObject *obj = qmp_qom_get(path, property, &err);
> +
> +    if (err == NULL) {
> +        QString *str = qobject_to_json_pretty(obj);
> +        monitor_printf(mon, "%s\n", qstring_get_str(str));
> +        qobject_unref(str);

Simple, does the job, lovely!

> +    }
> +
> +    hmp_handle_error(mon, err);
> +}
> +
>   typedef struct QOMCompositionState {
>       Monitor *mon;
>       int indent;
> diff --git a/tests/qtest/test-hmp.c b/tests/qtest/test-hmp.c
> index f8aa5f92c5..b8b1271b9e 100644
> --- a/tests/qtest/test-hmp.c
> +++ b/tests/qtest/test-hmp.c
> @@ -61,6 +61,7 @@ static const char *hmp_cmds[] = {
>       "p $pc + 8",
>       "qom-list /",
>       "qom-set /machine initrd test",
> +    "qom-get /machine initrd",
>       "screendump /dev/null",
>       "sendkey x",
>       "singlestep on",
> 

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

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

[Philippe Mathieu-Daudé]

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

> 
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> ---
>   qom/qom-hmp-cmds.c | 16 +++++-----------
>   1 file changed, 5 insertions(+), 11 deletions(-)
> 
> diff --git a/qom/qom-hmp-cmds.c b/qom/qom-hmp-cmds.c
> index a8b0a080c7..f704b6949a 100644
> --- a/qom/qom-hmp-cmds.c
> +++ b/qom/qom-hmp-cmds.c
> @@ -48,19 +48,13 @@ void hmp_qom_set(Monitor *mon, const QDict *qdict)
>       const char *property = qdict_get_str(qdict, "property");
>       const char *value = qdict_get_str(qdict, "value");
>       Error *err = NULL;
> -    bool ambiguous = false;
> -    Object *obj;
> +    QObject *obj;
>   
> -    obj = object_resolve_path(path, &ambiguous);
> -    if (obj == NULL) {
> -        error_set(&err, ERROR_CLASS_DEVICE_NOT_FOUND,
> -                  "Device '%s' not found", path);
> -    } else {
> -        if (ambiguous) {
> -            monitor_printf(mon, "Warning: Path '%s' is ambiguous\n", path);
> -        }
> -        object_property_parse(obj, value, property, &err);
> +    obj = qobject_from_json(value, &err);
> +    if (err == NULL) {
> +        qmp_qom_set(path, property, obj, &err);
>       }
> +
>       hmp_handle_error(mon, err);
>   }
>   
> 

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

[Dr. David Alan Gilbert]

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

Dave

> > 
> > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > ---
> >   qom/qom-hmp-cmds.c | 16 +++++-----------
> >   1 file changed, 5 insertions(+), 11 deletions(-)
> > 
> > diff --git a/qom/qom-hmp-cmds.c b/qom/qom-hmp-cmds.c
> > index a8b0a080c7..f704b6949a 100644
> > --- a/qom/qom-hmp-cmds.c
> > +++ b/qom/qom-hmp-cmds.c
> > @@ -48,19 +48,13 @@ void hmp_qom_set(Monitor *mon, const QDict *qdict)
> >       const char *property = qdict_get_str(qdict, "property");
> >       const char *value = qdict_get_str(qdict, "value");
> >       Error *err = NULL;
> > -    bool ambiguous = false;
> > -    Object *obj;
> > +    QObject *obj;
> > -    obj = object_resolve_path(path, &ambiguous);
> > -    if (obj == NULL) {
> > -        error_set(&err, ERROR_CLASS_DEVICE_NOT_FOUND,
> > -                  "Device '%s' not found", path);
> > -    } else {
> > -        if (ambiguous) {
> > -            monitor_printf(mon, "Warning: Path '%s' is ambiguous\n", path);
> > -        }
> > -        object_property_parse(obj, value, property, &err);
> > +    obj = qobject_from_json(value, &err);
> > +    if (err == NULL) {
> > +        qmp_qom_set(path, property, obj, &err);
> >       }
> > +
> >       hmp_handle_error(mon, err);
> >   }
> > 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

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

[Markus Armbruster]

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

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,

Re: [PATCH 1/2] hmp: Implement qom-get HMP command

[Markus Armbruster]

"Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> writes:

> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>
> This started off as Andreas Färber's implementation from
> March 2015, but after feedback from Paolo and Markus it morphed into
> using the json output which handles structs reasonably.
>
> Use with qom-list to find the members of an object.
>
> (qemu) qom-get /backend/console[0]/device/vga.rom[0] size
> 65536
> (qemu) qom-get /machine smm
> "auto"
> (qemu) qom-get /machine rtc-time
> {
>     "tm_year": 120,
>     "tm_sec": 51,
>     "tm_hour": 9,
>     "tm_min": 50,
>     "tm_mon": 4,
>     "tm_mday": 20
> }
> (qemu) qom-get /machine frob
> Error: Property '.frob' not found

  (qemu) qom-get /machine peripheral
  "/machine/peripheral"

Not this patch's fault, but WTF?

Turns out it's simply what object_get_child_property() does.

Paolo, is this what we want for qom-get?

Also not this patch's fault: separating path and property feels like a
pointless complication of the interface to me.  Why

    {"execute": "qom-get", "arguments": {"path": "/machine", "property": "smm"}}

and not

    {"execute": "qom-get", "arguments": {"path": "/machine/smm"}}

?

Too late to change for QMP, I guess.

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

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
> 

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

[Markus Armbruster]

"Dr. David Alan Gilbert" <dgilbert@redhat.com> writes:

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

Also fix the typo in the title "hmp: Simplify qom_set": it's qom-set.

Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [PATCH 1/2] hmp: Implement qom-get HMP command

[Markus Armbruster]

Markus Armbruster <armbru@redhat.com> writes:

> "Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> writes:
>
>> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>>
>> This started off as Andreas Färber's implementation from
>> March 2015, but after feedback from Paolo and Markus it morphed into
>> using the json output which handles structs reasonably.
>>
>> Use with qom-list to find the members of an object.
>>
>> (qemu) qom-get /backend/console[0]/device/vga.rom[0] size
>> 65536
>> (qemu) qom-get /machine smm
>> "auto"
>> (qemu) qom-get /machine rtc-time
>> {
>>     "tm_year": 120,
>>     "tm_sec": 51,
>>     "tm_hour": 9,
>>     "tm_min": 50,
>>     "tm_mon": 4,
>>     "tm_mday": 20
>> }
>> (qemu) qom-get /machine frob
>> Error: Property '.frob' not found
>
>   (qemu) qom-get /machine peripheral
>   "/machine/peripheral"
>
> Not this patch's fault, but WTF?

Because it's not this patch's fault (QMP behaves the same way), we
shouldn't let it block the patch, so
Reviewed-by: Markus Armbruster <armbru@redhat.com>

I just sent

    Subject: QMP qom-get feels useless for child properties
    Date: Fri, 29 May 2020 08:57:11 +0200
    Message-ID: <87lflbns8o.fsf@dusky.pond.sub.org>

[...]

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:
> 
> > * 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?
> 
> Also fix the typo in the title "hmp: Simplify qom_set": it's qom-set.

Done.

> Reviewed-by: Markus Armbruster <armbru@redhat.com>

Thanks.

--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH 0/2] HMP: qom-get and set

[Dr. David Alan Gilbert]

* Dr. David Alan Gilbert (git) (dgilbert@redhat.com) wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> 
> HMP has had a qom-set for a long time, but not a matching qom-get;
> various attempts have been made to add one over the last 5 years.
> Here's another go.
> 
> It's got simpler due to a suggestion by Markus to take the output
> from qmp's qom-get.  While we're here, rework hmp's qom-set to be
> a wrapper around the qmp equivalent, which simplifies it a bit.
> 
> In one post people didn't particularly like the use of JSON,
> but in the intervening 4 years no one has implemented anything to
> avoid it, and the output is trivially readable for non-structures
> and still pretty readable for (the very rare) structures.
> 
> Dave

Queued

> 
> Dr. David Alan Gilbert (2):
>   hmp: Implement qom-get HMP command
>   hmp: Simplify qom_set
> 
>  hmp-commands.hx        | 14 ++++++++++++++
>  include/monitor/hmp.h  |  1 +
>  qom/qom-hmp-cmds.c     | 34 +++++++++++++++++++++++-----------
>  tests/qtest/test-hmp.c |  1 +
>  4 files changed, 39 insertions(+), 11 deletions(-)
> 
> -- 
> 2.26.2
> 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK