[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [RFC PATCH] qom: Implement qom-get HMP command
From: |
Dr. David Alan Gilbert |
Subject: |
Re: [RFC PATCH] qom: Implement qom-get HMP command |
Date: |
Thu, 7 May 2020 18:06:11 +0100 |
User-agent: |
Mutt/1.13.4 (2020-02-15) |
* 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