[RFC PATCH] qom: Implement qom-get HMP command

[RFC PATCH] qom: Implement qom-get HMP command

[Cédric Le Goater]

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

Reimplement it based on qmp_qom_get() to avoid converting QObjects back
to strings.

Inspired-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Andreas Färber <afaerber@suse.de>

Slight fix for bit-rot:
Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
[clg: updates for QEMU 5.0 ]
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---

 I would like to restart the discussion on qom-get command to understand
 what was the conclusion and see if things have changed since.

 Thanks,

 C.

 include/monitor/hmp.h |  1 +
 qom/qom-hmp-cmds.c    | 23 +++++++++++++++++++++++
 hmp-commands.hx       | 13 +++++++++++++
 3 files changed, 37 insertions(+)

diff --git a/include/monitor/hmp.h b/include/monitor/hmp.h
index e33ca5a911a5..c986cfd28bc3 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 cd08233a4cfe..b14cf6e785f4 100644
--- a/qom/qom-hmp-cmds.c
+++ b/qom/qom-hmp-cmds.c
@@ -40,6 +40,29 @@ void hmp_qom_list(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;
+    Object *obj;
+    char *value;
+
+    obj = object_resolve_path(path, NULL);
+    if (obj == NULL) {
+        error_set(&err, ERROR_CLASS_DEVICE_NOT_FOUND,
+                  "Device '%s' not found", path);
+        hmp_handle_error(mon, err);
+        return;
+    }
+    value = object_property_print(obj, property, true, &err);
+    if (err == NULL) {
+        monitor_printf(mon, "%s\n", value);
+        g_free(value);
+    }
+    hmp_handle_error(mon, err);
+}
+
 void hmp_qom_set(Monitor *mon, const QDict *qdict)
 {
     const char *path = qdict_get_str(qdict, "path");
diff --git a/hmp-commands.hx b/hmp-commands.hx
index 7f0f3974ad90..4e39b9caed3e 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -1790,6 +1790,19 @@ 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,
+    },
+
+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",
-- 
2.25.1

Re: [RFC PATCH] qom: Implement qom-get HMP command

[Dr. David Alan Gilbert]

* Cédric Le Goater (clg@kaod.org) wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> 
> Reimplement it based on qmp_qom_get() to avoid converting QObjects back
> to strings.

<blows dust off patch>
I'd love to see this or something similar in;  what does it's output
look like for structures - I think that was the main problem people
complained about last time, although IMHO even a version that couldn't
do structures nicely would be better than nothing.

Dave

> 
> Inspired-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Andreas Färber <afaerber@suse.de>
> 
> Slight fix for bit-rot:
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> [clg: updates for QEMU 5.0 ]
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
> 
>  I would like to restart the discussion on qom-get command to understand
>  what was the conclusion and see if things have changed since.
> 
>  Thanks,
> 
>  C.
> 
>  include/monitor/hmp.h |  1 +
>  qom/qom-hmp-cmds.c    | 23 +++++++++++++++++++++++
>  hmp-commands.hx       | 13 +++++++++++++
>  3 files changed, 37 insertions(+)
> 
> diff --git a/include/monitor/hmp.h b/include/monitor/hmp.h
> index e33ca5a911a5..c986cfd28bc3 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 cd08233a4cfe..b14cf6e785f4 100644
> --- a/qom/qom-hmp-cmds.c
> +++ b/qom/qom-hmp-cmds.c
> @@ -40,6 +40,29 @@ void hmp_qom_list(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;
> +    Object *obj;
> +    char *value;
> +
> +    obj = object_resolve_path(path, NULL);
> +    if (obj == NULL) {
> +        error_set(&err, ERROR_CLASS_DEVICE_NOT_FOUND,
> +                  "Device '%s' not found", path);
> +        hmp_handle_error(mon, err);
> +        return;
> +    }
> +    value = object_property_print(obj, property, true, &err);
> +    if (err == NULL) {
> +        monitor_printf(mon, "%s\n", value);
> +        g_free(value);
> +    }
> +    hmp_handle_error(mon, err);
> +}
> +
>  void hmp_qom_set(Monitor *mon, const QDict *qdict)
>  {
>      const char *path = qdict_get_str(qdict, "path");
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index 7f0f3974ad90..4e39b9caed3e 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -1790,6 +1790,19 @@ 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,
> +    },
> +
> +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",
> -- 
> 2.25.1
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [RFC PATCH] qom: Implement qom-get HMP command

[Cédric Le Goater]

On 4/27/20 9:19 PM, Dr. David Alan Gilbert wrote:
> * Cédric Le Goater (clg@kaod.org) wrote:
>> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>>
>> Reimplement it based on qmp_qom_get() to avoid converting QObjects back
>> to strings.
> 
> <blows dust off patch>
> I'd love to see this or something similar in;  what does it's output
> look like for structures - I think that was the main problem people
> complained about last time, although IMHO even a version that couldn't
> do structures nicely would be better than nothing.

Should we merge this patch then ? 

C. 

Re: [RFC PATCH] qom: Implement qom-get HMP command

[Markus Armbruster]

Cédric Le Goater <clg@kaod.org> writes:

> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>
> Reimplement it based on qmp_qom_get() to avoid converting QObjects back
> to strings.
>
> Inspired-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Andreas Färber <afaerber@suse.de>
>
> Slight fix for bit-rot:
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> [clg: updates for QEMU 5.0 ]
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>
>  I would like to restart the discussion on qom-get command to understand
>  what was the conclusion and see if things have changed since.

I've since learned more about QOM.  I believe we should do HMP qom-get,
but not quite like this patch does.  Let me explain.

A QOM property has ->get() and ->set() methods to expose the property
via the Visitor interface.

->get() works with an output visitor.  With the QObject output visitor,
you can get the property value as a QObject.  QMP qom-get uses this.
With the string output visitor, you can get it as a string.  Your
proposed HMP qom-get uses this.

->set() works with an input visitor.  With the QObject input visitor,
you can set the property value from a QObject.  QMP qom-set uses this.
With the string input visitor, you can set it from a string.  HMP
qom-set uses this.  With the options visitor, you can set it from a
QemuOpts.  CLI -object uses this.

The Visitor interface supports arbitrary QAPI types.  The ->get() and
->set() methods can use them all.

Some visitors, howerver, carry restrictions:

 * The string output visitor does not implement support for visiting
 * QAPI structs, alternates, null, or arbitrary QTypes.  It also
 * requires a non-null list argument to visit_start_list().

 * The string input visitor does not implement support for visiting
 * QAPI structs, alternates, null, or arbitrary QTypes. Only flat lists
 * of integers (except type "size") are supported.

 * The Opts input visitor does not implement support for visiting QAPI
 * alternates, numbers (other than integers), null, or arbitrary
 * QTypes.  It also requires a non-null list argument to
 * visit_start_list().

If you try to qom-set a property whose ->set() uses something the string
input visitor doesn't support, QEMU crashes.  Same for -object, and your
proposed qom-get.

I'm not aware of such a ->set(), but this is a death trap all the same.

The obvious way to disarm it is removing the restrictions.

One small step we took towards that goal is visible in the comments
above: support for flat lists of integers.  The code for that will make
your eyes bleed.  It's been a thorn in my side ever since I inherited
QAPI.  Perhaps it could be done better.  But there's a reason why it
should not be done at all: it's language design.

The QObject visitors translate between QAPI types and our internal
representation of JSON.  The JSON parser and formatter complete the
translation to and from JSON.  Sensible enough.

The string visitors translate between QAPI and ...  well, a barely
documented language of our own "design".  Tolerable when the language it
limited to numbers, booleans and strings.  Foolish when it grows into
something isomorphic to JSON.

This is a dead end.

Can we still implement a safe and tolerably useful HMP qom-get and
qom-set?  I think we can.  Remember the basic rule of HMP command
implementation: wrap around the QMP command.  So let's try that.

hmp_qom_get() calls qmp_qom_get() and formats the resulting QObject with
qobject_to_json_pretty().

hmp_qom_get() parses the value with qobject_from_json(), and feeds the
resulting QObject to qmp_qom_set().  To avoid interference with the HMP
parser, we probably want argument type 'S'.

The two commands then parse / print JSON instead of the string visitors'
language.  Is that bad?

* Integers: qom-set loses support for hexadecimal and octal.

* Bools: qom-set loses alternate spellings of true and false.

* Floating-point numbers: no change

* Strings: gain enclosing quotes.

* List of integers: gain enclosing square brackets.  qom-set loses
  support for ranges.

  The only use of lists I can find is memory-backend property
  host-nodes.

* Everything else: lose support for crashing QEMU.

  Again, I'm not aware of properties that crash now.

I think these changes are just fine.  If you disagree, you could try to
mitigate with convenience magic like "if it doesn't parse as JSON, and
it looks hexadecimal, convert to decimal and try again", or "looks kind
of stringy, enclose in double-quotes and try again".  Bad idea if you
ask me, but I'm not the HMP maintainer anymore.

Here's an idea I hate less.  Remember, since the opts visitor also has
restrictions, -object is also prone to crashing.  But that's an instance
of a problem we've thought about at some depth, and where we have a
plan: we intend to replace QemuOpts with QObject (which means we can
switch to the QObject visitors), and have CLI options take either JSON
or a relatively simple KEY=VALUE,... language similar to what QemuOpts
accepts now.

Yes, that's also a language of our own design, but it comes with a few
excuses:

0. It lets us avoid radical change of QEMU's CLI.

1. It's fairly simple.  It does not try to be isomorphic to JSON.  It
doesn't have to, because you can always fall back to JSON when things
get wonky.

2. It's documented.  So far only in util/keyval.c.  No good for users
there, but at least it demonstrates we know what language we're parsing
(modulo parser bugs).  More than what can be said for many ad hoc
languages...

We could use this for a friendlier qom-set.  I'm not sure it's worth the
trouble at this time.

Re: [RFC PATCH] qom: Implement qom-get HMP command

[Dr. David Alan Gilbert]

* Markus Armbruster (armbru@redhat.com) wrote:
> Cédric Le Goater <clg@kaod.org> writes:
> 
> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> >
> > Reimplement it based on qmp_qom_get() to avoid converting QObjects back
> > to strings.
> >
> > Inspired-by: Paolo Bonzini <pbonzini@redhat.com>
> > Signed-off-by: Andreas Färber <afaerber@suse.de>
> >
> > Slight fix for bit-rot:
> > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > [clg: updates for QEMU 5.0 ]
> > Signed-off-by: Cédric Le Goater <clg@kaod.org>
> > ---
> >
> >  I would like to restart the discussion on qom-get command to understand
> >  what was the conclusion and see if things have changed since.
> 
> I've since learned more about QOM.  I believe we should do HMP qom-get,
> but not quite like this patch does.  Let me explain.
> 
> A QOM property has ->get() and ->set() methods to expose the property
> via the Visitor interface.
> 
> ->get() works with an output visitor.  With the QObject output visitor,
> you can get the property value as a QObject.  QMP qom-get uses this.
> With the string output visitor, you can get it as a string.  Your
> proposed HMP qom-get uses this.
> 
> ->set() works with an input visitor.  With the QObject input visitor,
> you can set the property value from a QObject.  QMP qom-set uses this.
> With the string input visitor, you can set it from a string.  HMP
> qom-set uses this.  With the options visitor, you can set it from a
> QemuOpts.  CLI -object uses this.
> 
> The Visitor interface supports arbitrary QAPI types.  The ->get() and
> ->set() methods can use them all.
> 
> Some visitors, howerver, carry restrictions:
> 
>  * The string output visitor does not implement support for visiting
>  * QAPI structs, alternates, null, or arbitrary QTypes.  It also
>  * requires a non-null list argument to visit_start_list().
> 
>  * The string input visitor does not implement support for visiting
>  * QAPI structs, alternates, null, or arbitrary QTypes. Only flat lists
>  * of integers (except type "size") are supported.
> 
>  * The Opts input visitor does not implement support for visiting QAPI
>  * alternates, numbers (other than integers), null, or arbitrary
>  * QTypes.  It also requires a non-null list argument to
>  * visit_start_list().
> 
> If you try to qom-set a property whose ->set() uses something the string
> input visitor doesn't support, QEMU crashes.  Same for -object, and your
> proposed qom-get.

Crashing would be bad.

> I'm not aware of such a ->set(), but this is a death trap all the same.
> 
> The obvious way to disarm it is removing the restrictions.
> 
> One small step we took towards that goal is visible in the comments
> above: support for flat lists of integers.  The code for that will make
> your eyes bleed.  It's been a thorn in my side ever since I inherited
> QAPI.  Perhaps it could be done better.  But there's a reason why it
> should not be done at all: it's language design.
> 
> The QObject visitors translate between QAPI types and our internal
> representation of JSON.  The JSON parser and formatter complete the
> translation to and from JSON.  Sensible enough.
> 
> The string visitors translate between QAPI and ...  well, a barely
> documented language of our own "design".  Tolerable when the language it
> limited to numbers, booleans and strings.  Foolish when it grows into
> something isomorphic to JSON.
> 
> This is a dead end.
> 
> Can we still implement a safe and tolerably useful HMP qom-get and
> qom-set?  I think we can.  Remember the basic rule of HMP command
> implementation: wrap around the QMP command.  So let's try that.
> 
> hmp_qom_get() calls qmp_qom_get() and formats the resulting QObject with
> qobject_to_json_pretty().

Why don't we *just* do this?
OK, we wont fix the qom-set or the CLI, but if we just get hmp_qom_get
to call QMP, format it into json and then dump the json to the user,
then we get a usable, if not pretty, qom-get command, without having to
solve all these hard problems to move it forward?

Dave

> hmp_qom_get() parses the value with qobject_from_json(), and feeds the
> resulting QObject to qmp_qom_set().  To avoid interference with the HMP
> parser, we probably want argument type 'S'.
> 
> The two commands then parse / print JSON instead of the string visitors'
> language.  Is that bad?
> 
> * Integers: qom-set loses support for hexadecimal and octal.
> 
> * Bools: qom-set loses alternate spellings of true and false.
> 
> * Floating-point numbers: no change
> 
> * Strings: gain enclosing quotes.
> 
> * List of integers: gain enclosing square brackets.  qom-set loses
>   support for ranges.
> 
>   The only use of lists I can find is memory-backend property
>   host-nodes.
> 
> * Everything else: lose support for crashing QEMU.
> 
>   Again, I'm not aware of properties that crash now.
> 
> I think these changes are just fine.  If you disagree, you could try to
> mitigate with convenience magic like "if it doesn't parse as JSON, and
> it looks hexadecimal, convert to decimal and try again", or "looks kind
> of stringy, enclose in double-quotes and try again".  Bad idea if you
> ask me, but I'm not the HMP maintainer anymore.
> 
> Here's an idea I hate less.  Remember, since the opts visitor also has
> restrictions, -object is also prone to crashing.  But that's an instance
> of a problem we've thought about at some depth, and where we have a
> plan: we intend to replace QemuOpts with QObject (which means we can
> switch to the QObject visitors), and have CLI options take either JSON
> or a relatively simple KEY=VALUE,... language similar to what QemuOpts
> accepts now.
> 
> Yes, that's also a language of our own design, but it comes with a few
> excuses:
> 
> 0. It lets us avoid radical change of QEMU's CLI.
> 
> 1. It's fairly simple.  It does not try to be isomorphic to JSON.  It
> doesn't have to, because you can always fall back to JSON when things
> get wonky.
> 
> 2. It's documented.  So far only in util/keyval.c.  No good for users
> there, but at least it demonstrates we know what language we're parsing
> (modulo parser bugs).  More than what can be said for many ad hoc
> languages...
> 
> We could use this for a friendlier qom-set.  I'm not sure it's worth the
> trouble at this time.
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [RFC PATCH] qom: Implement qom-get HMP command

[Markus Armbruster]

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

> * Markus Armbruster (armbru@redhat.com) wrote:
>> Cédric Le Goater <clg@kaod.org> writes:
>> 
>> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>> >
>> > Reimplement it based on qmp_qom_get() to avoid converting QObjects back
>> > to strings.
>> >
>> > Inspired-by: Paolo Bonzini <pbonzini@redhat.com>
>> > Signed-off-by: Andreas Färber <afaerber@suse.de>
>> >
>> > Slight fix for bit-rot:
>> > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
>> > [clg: updates for QEMU 5.0 ]
>> > Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> > ---
>> >
>> >  I would like to restart the discussion on qom-get command to understand
>> >  what was the conclusion and see if things have changed since.
>> 
>> I've since learned more about QOM.  I believe we should do HMP qom-get,
>> but not quite like this patch does.  Let me explain.
>> 
>> A QOM property has ->get() and ->set() methods to expose the property
>> via the Visitor interface.
>> 
>> ->get() works with an output visitor.  With the QObject output visitor,
>> you can get the property value as a QObject.  QMP qom-get uses this.
>> With the string output visitor, you can get it as a string.  Your
>> proposed HMP qom-get uses this.
>> 
>> ->set() works with an input visitor.  With the QObject input visitor,
>> you can set the property value from a QObject.  QMP qom-set uses this.
>> With the string input visitor, you can set it from a string.  HMP
>> qom-set uses this.  With the options visitor, you can set it from a
>> QemuOpts.  CLI -object uses this.
>> 
>> The Visitor interface supports arbitrary QAPI types.  The ->get() and
>> ->set() methods can use them all.
>> 
>> Some visitors, howerver, carry restrictions:
>> 
>>  * The string output visitor does not implement support for visiting
>>  * QAPI structs, alternates, null, or arbitrary QTypes.  It also
>>  * requires a non-null list argument to visit_start_list().
>> 
>>  * The string input visitor does not implement support for visiting
>>  * QAPI structs, alternates, null, or arbitrary QTypes. Only flat lists
>>  * of integers (except type "size") are supported.
>> 
>>  * The Opts input visitor does not implement support for visiting QAPI
>>  * alternates, numbers (other than integers), null, or arbitrary
>>  * QTypes.  It also requires a non-null list argument to
>>  * visit_start_list().
>> 
>> If you try to qom-set a property whose ->set() uses something the string
>> input visitor doesn't support, QEMU crashes.  Same for -object, and your
>> proposed qom-get.
>
> Crashing would be bad.
>
>> I'm not aware of such a ->set(), but this is a death trap all the same.
>> 
>> The obvious way to disarm it is removing the restrictions.
>> 
>> One small step we took towards that goal is visible in the comments
>> above: support for flat lists of integers.  The code for that will make
>> your eyes bleed.  It's been a thorn in my side ever since I inherited
>> QAPI.  Perhaps it could be done better.  But there's a reason why it
>> should not be done at all: it's language design.
>> 
>> The QObject visitors translate between QAPI types and our internal
>> representation of JSON.  The JSON parser and formatter complete the
>> translation to and from JSON.  Sensible enough.
>> 
>> The string visitors translate between QAPI and ...  well, a barely
>> documented language of our own "design".  Tolerable when the language it
>> limited to numbers, booleans and strings.  Foolish when it grows into
>> something isomorphic to JSON.
>> 
>> This is a dead end.
>> 
>> Can we still implement a safe and tolerably useful HMP qom-get and
>> qom-set?  I think we can.  Remember the basic rule of HMP command
>> implementation: wrap around the QMP command.  So let's try that.
>> 
>> hmp_qom_get() calls qmp_qom_get() and formats the resulting QObject with
>> qobject_to_json_pretty().
>
> Why don't we *just* do this?
> OK, we wont fix the qom-set or the CLI, but if we just get hmp_qom_get
> to call QMP, format it into json and then dump the json to the user,
> then we get a usable, if not pretty, qom-get command, without having to
> solve all these hard problems to move it forward?

Count me in favour.  I'd like to see the matching change to HMP qom-set,
though.

Re: [RFC PATCH] qom: Implement qom-get HMP command

[Dr. David Alan Gilbert]

* Markus Armbruster (armbru@redhat.com) wrote:
> "Dr. David Alan Gilbert" <dgilbert@redhat.com> writes:
> 
> > * Markus Armbruster (armbru@redhat.com) wrote:
> >> Cédric Le Goater <clg@kaod.org> writes:
> >> 
> >> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> >> >
> >> > Reimplement it based on qmp_qom_get() to avoid converting QObjects back
> >> > to strings.
> >> >
> >> > Inspired-by: Paolo Bonzini <pbonzini@redhat.com>
> >> > Signed-off-by: Andreas Färber <afaerber@suse.de>
> >> >
> >> > Slight fix for bit-rot:
> >> > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> >> > [clg: updates for QEMU 5.0 ]
> >> > Signed-off-by: Cédric Le Goater <clg@kaod.org>
> >> > ---
> >> >
> >> >  I would like to restart the discussion on qom-get command to understand
> >> >  what was the conclusion and see if things have changed since.
> >> 
> >> I've since learned more about QOM.  I believe we should do HMP qom-get,
> >> but not quite like this patch does.  Let me explain.
> >> 
> >> A QOM property has ->get() and ->set() methods to expose the property
> >> via the Visitor interface.
> >> 
> >> ->get() works with an output visitor.  With the QObject output visitor,
> >> you can get the property value as a QObject.  QMP qom-get uses this.
> >> With the string output visitor, you can get it as a string.  Your
> >> proposed HMP qom-get uses this.
> >> 
> >> ->set() works with an input visitor.  With the QObject input visitor,
> >> you can set the property value from a QObject.  QMP qom-set uses this.
> >> With the string input visitor, you can set it from a string.  HMP
> >> qom-set uses this.  With the options visitor, you can set it from a
> >> QemuOpts.  CLI -object uses this.
> >> 
> >> The Visitor interface supports arbitrary QAPI types.  The ->get() and
> >> ->set() methods can use them all.
> >> 
> >> Some visitors, howerver, carry restrictions:
> >> 
> >>  * The string output visitor does not implement support for visiting
> >>  * QAPI structs, alternates, null, or arbitrary QTypes.  It also
> >>  * requires a non-null list argument to visit_start_list().
> >> 
> >>  * The string input visitor does not implement support for visiting
> >>  * QAPI structs, alternates, null, or arbitrary QTypes. Only flat lists
> >>  * of integers (except type "size") are supported.
> >> 
> >>  * The Opts input visitor does not implement support for visiting QAPI
> >>  * alternates, numbers (other than integers), null, or arbitrary
> >>  * QTypes.  It also requires a non-null list argument to
> >>  * visit_start_list().
> >> 
> >> If you try to qom-set a property whose ->set() uses something the string
> >> input visitor doesn't support, QEMU crashes.  Same for -object, and your
> >> proposed qom-get.
> >
> > Crashing would be bad.
> >
> >> I'm not aware of such a ->set(), but this is a death trap all the same.
> >> 
> >> The obvious way to disarm it is removing the restrictions.
> >> 
> >> One small step we took towards that goal is visible in the comments
> >> above: support for flat lists of integers.  The code for that will make
> >> your eyes bleed.  It's been a thorn in my side ever since I inherited
> >> QAPI.  Perhaps it could be done better.  But there's a reason why it
> >> should not be done at all: it's language design.
> >> 
> >> The QObject visitors translate between QAPI types and our internal
> >> representation of JSON.  The JSON parser and formatter complete the
> >> translation to and from JSON.  Sensible enough.
> >> 
> >> The string visitors translate between QAPI and ...  well, a barely
> >> documented language of our own "design".  Tolerable when the language it
> >> limited to numbers, booleans and strings.  Foolish when it grows into
> >> something isomorphic to JSON.
> >> 
> >> This is a dead end.
> >> 
> >> Can we still implement a safe and tolerably useful HMP qom-get and
> >> qom-set?  I think we can.  Remember the basic rule of HMP command
> >> implementation: wrap around the QMP command.  So let's try that.
> >> 
> >> hmp_qom_get() calls qmp_qom_get() and formats the resulting QObject with
> >> qobject_to_json_pretty().
> >
> > Why don't we *just* do this?
> > OK, we wont fix the qom-set or the CLI, but if we just get hmp_qom_get
> > to call QMP, format it into json and then dump the json to the user,
> > then we get a usable, if not pretty, qom-get command, without having to
> > solve all these hard problems to move it forward?
> 
> Count me in favour.  I'd like to see the matching change to HMP qom-set,
> though.

It turns out I actually did do a JSON version, as the follow up to the
version Cédric reposted; but then that got lost in people not liking
JSON;   https://lists.gnu.org/archive/html/qemu-devel/2016-09/msg01041.html

Having just resurrected that code, the only difference from my patch
then and what I just wrote was that instead of doing the object
resolution and stuff, I just call the qmp function.
It's still JSON output and I don't think the arguments from 4 years ago
moved forward.  I'll post it soon.

Dave

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

Re: [RFC PATCH] qom: Implement qom-get HMP command

[Paolo Bonzini]

On 20/05/20 11:59, Dr. David Alan Gilbert wrote:
>> Count me in favour.  I'd like to see the matching change to HMP qom-set,
>> though.
> It turns out I actually did do a JSON version, as the follow up to the
> version Cédric reposted; but then that got lost in people not liking
> JSON;   https://lists.gnu.org/archive/html/qemu-devel/2016-09/msg01041.html

Great.  I think we're in agreement then that making qom-get and qom-set
just take JSON is the best option.  So qom-get would be
"path:s,property:s" as in your patch, and qom-set would be
"path:s,property:s,value:S".

It is not a big deal to make this a backwards-incompatible change.

Paolo