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

[Markus Armbruster]

Cédric Le Goater <address@hidden> writes:

> From: "Dr. David Alan Gilbert" <address@hidden>
>
> Reimplement it based on qmp_qom_get() to avoid converting QObjects back
> to strings.
>
> Inspired-by: Paolo Bonzini <address@hidden>
> Signed-off-by: Andreas Färber <address@hidden>
>
> Slight fix for bit-rot:
> Signed-off-by: Dr. David Alan Gilbert <address@hidden>
> [clg: updates for QEMU 5.0 ]
> Signed-off-by: Cédric Le Goater <address@hidden>
> ---
>
>  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.