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

[Dr. David Alan Gilbert]

* Markus Armbruster (address@hidden) wrote:
> 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.

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 / address@hidden / Manchester, UK