"Dr. David Alan Gilbert" <dgilb...@redhat.com> writes: > * 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?
Count me in favour. I'd like to see the matching change to HMP qom-set, though.
