Re: [RFC 1/2] qapi/virtio: introduce the "show-bits" argument for x-query-virtio-status

[Markus Armbruster <armbru@redhat.com>]

Laurent Vivier <lvivier@redhat.com> writes:

> On 12/1/23 16:21, Markus Armbruster wrote:

[...]

>> Both use cases are valid, but I dislike both the existing and the
>> proposed interface.
>>
>> We can change it: x-query-virtio-status isn't stable (it's for debugging
>> and testing).  But even unstable interfaces should only be changed for
>> good, clear reasons.
>>
>> I feel the change from "bits encoded as a number" to "bits as list of
>> descriptive strings plus number for the unknown ones" fell short.  Let
>> me explain.
>>
>> The initial version of the command had "bits encoded as number".  Unless
>> we understand why that was done, we should assume it was done for a
>> reason.  We now know it was: Hyman Huang posted a patch to get it back.
>>
>> Instead of "bits as list of descriptive strings plus number for the
>> unknown ones", we could have done "bits encoded as number, plus list of
>> descriptive strings", or plus some other human-readable encoding.
>>
>> QMP output of the form "WELL_KNOWN_SYMBOL: human readable explanation"
>> smells of encoding structured information in strings, which is a no-no.
>>
>> Perhaps we could have added human-readable output just in HMP.  That's
>> what we normally do.
>>
>> Here are a few possible alternatives to Hyman Huang's patch:
>>
>> 1. Revert commit f3034ad71fc for QMP, keep it for HMP.
>>
>> 2. Replace @unknown-FOO (just the unknown bits) by @FOO-bits (all bits).
>>
>> 3. Add @FOO-bits next to @unknown-FOO, deprecate @unknown-FOO.
>>
>> 4. Create a QAPI enum for the known bits.  Clients can use introspection
>>    to learn the mapping between symbols and bits.  Requires dumbing down
>>    the descriptive strings to just the symbols.  This feels
>>    both overengineered and cumbersome to use.
>>
>> For 2 and 3, I'd prefer to also dumb down the descriptive strings to
>> just the symbols.
>>
>> Thoughts?
>> 
>
> I agree with you. As x-CMD are unstable, perhaps we can go directly to 2?

We can.  It might incovenience existing users of @unknown-FOO.

> (and of course to remove the descriptive strings. Is it easily possible to keep them for the HMP version?)

We could change qmp_virtio_feature_map_t to

    typedef struct {
        int virtio_bit;
        const char *feature_name;
        const char *feature_desc;
    } qmp_virtio_feature_map_t;

and FEATURE_ENTRY() to

    #define FEATURE_ENTRY(bit, name, desc) (qmp_virtio_feature_map_t) \
        { .virtio_bit = (bit), .feature_name = (name), .feature_desc = (desc) }

Aside: POSIX reserves names ending with _t.  An actual clash is of
course vanishingly unlikely.  But avoiding _t suffix is a good habit.

qmp_x_query_virtio_status() could then convert bits to a list of known names
using .feature_name.

To keep the descriptions in HMP, simply print the bits using
.feature_name and .feature_desc, ignoring list of known names returned
by qmp_x_query_virtio_status().

Alternatively, make the code to convert bits to list of strings flexible
enough to be usable in both places.

If qmp_virtio_feature_map_t is still only used in virtio-qmp.c, move its
there.