* Markus Armbruster (arm...@redhat.com) wrote: > Cédric Le Goater <c...@kaod.org> writes: > > > From: "Dr. David Alan Gilbert" <dgilb...@redhat.com> > > > > Reimplement it based on qmp_qom_get() to avoid converting QObjects back > > to strings. > > > > Inspired-by: Paolo Bonzini <pbonz...@redhat.com> > > Signed-off-by: Andreas Färber <afaer...@suse.de> > > > > Slight fix for bit-rot: > > Signed-off-by: Dr. David Alan Gilbert <dgilb...@redhat.com> > > [clg: updates for QEMU 5.0 ] > > Signed-off-by: Cédric Le Goater <c...@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 / dgilb...@redhat.com / Manchester, UK