Re: [RFC PATCH] qapi: Incorrect attempt to fix building with MC146818RTC=n

[Markus Armbruster <armbru@redhat.com>]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 22/01/20 06:41, Markus Armbruster wrote:
>> Paolo Bonzini <pbonzini@redhat.com> writes:
>> 
>>> Il mar 21 gen 2020, 15:22 Markus Armbruster <armbru@redhat.com> ha scritto:
>>>
>>>>> To see it a different way, these are the "C bindings" to QMP, just that
>>>>> the implementation is an in-process call rather than RPC.  If the QAPI
>>>>> code generator was also able to generate Python bindings and the like,
>>>>> they would have to be the same for all QEMU binaries, wouldn't they?
>>>>
>>>> Ommitting the kind of #if we've been discussing is relatively harmless
>>>> but what about this one, in qapi-types-block-core.h:
>>>>
>>>>     typedef enum BlockdevDriver {
>>>>         BLOCKDEV_DRIVER_BLKDEBUG,
>>>>         [...]
>>>>     #if defined(CONFIG_REPLICATION)
>>>>         BLOCKDEV_DRIVER_REPLICATION,
>>>>     #endif /* defined(CONFIG_REPLICATION) */
>>>>         [...]
>>>>         BLOCKDEV_DRIVER__MAX,
>>>>     } BlockdevDriver;
>>>>
>>>
>>> Well, I don't think this should be conditional at all. Introspection is a
>>> tool to detect unsupported features, not working features.
>> 
>> Isn't this what it does?  To detect "replication" is unsupported, check
>> whether it's absent, and "supported" does not imply "works".
>
> Indeed...
>
>>>                                                            KVM will be
>>> present in introspection data even if /dev/kvm doesn't exist on your
>>> machine or you don't have permission to access it.
>> 
>> Yes.
>> 
>> QAPI/QMP introspection is compile-time static by design.  It can't tell
>> you more than "this QEMU build supports X".
>
> ... and I think it would be fine even if it told you less: "this QEMU
> will not give a parse error if X appears in QMP syntax".  For example,
> QEMU could accept "replication" even if CONFIG_REPLICATION is not
> defined and therefore using it would always fail.  This would allow
> limiting even more use of conditional compilation.

This is effectively how things worked before we added 'if' to the QAPI
schema language.

A feature F may

(1) not exist in this version of QEMU

(2) exist, but configured off for this build of QEMU

(3) be present in this build of QEMU

When the management application sees (1) or (2), it knows that using F
cannot possibly work no matter what you do to the host short of using
another build of QEMU.  The management application can then reject
attempts to use F with a useful error message, or make such attempts
impossible (think graying out menu entries).

When the management application sees (3), it still needs to be prepared
for actual use to fail.  Possible failures depend on the feature.
Identifying the various kinds of failures can be awkward and/or brittle.
Useful error reporting is often hard.

A management application doesn't care for the difference between (1) and
(2).  We added 'if' to the QAPI schema language in part to make the
difference disappear.

Without it, introspection munges together (2) and (3) instead.  The
management application's information degrades from "the QEMU I have does
not provide F" to "something went wrong".  Perhaps the management
application can figure out what went wrong, perhaps it can't.  Perhaps
it doesn't need to know.

My point is: we trade away information at the external interface for the
benefit of "limiting even more use of conditional compilation".  I doubt
that's a good trade, not least because I can't quite see how exactly the
benefit is, well, beneficial[*].

Anyway, we've had introspection reflect compile time configuration since
v3.0.0.  I don't think we can break that now.



[*] It saves us including headers we really should be including.
Anything else?