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

[Markus Armbruster]

"Dr. David Alan Gilbert" <address@hidden> writes:

> * 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?

Count me in favour.  I'd like to see the matching change to HMP qom-set,
though.