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. 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.