Laurent Vivier <lviv...@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.
