Re: [PATCH v6 1/6] qmp: add QMP command x-debug-query-virtio

[Markus Armbruster]

Jonah Palmer <jonah.palmer@oracle.com> writes:

> No problem! Comments below:
>
> On 8/23/21 9:27 AM, Markus Armbruster wrote:

[...]

>> Hmm...  how is this enum used?  In this patch:
>>
>>      VirtioInfoList *qmp_x_debug_query_virtio(Error **errp)
>>      {
>>          VirtioInfoList *list = NULL;
>>          VirtioInfoList *node;
>>          VirtIODevice *vdev;
>>
>>          QTAILQ_FOREACH(vdev, &virtio_list, next) {
>>              DeviceState *dev = DEVICE(vdev);
>>              node = g_new0(VirtioInfoList, 1);
>>              node->value = g_new(VirtioInfo, 1);
>>              node->value->path = g_strdup(dev->canonical_path);
>> --->        node->value->type = qapi_enum_parse(&VirtioType_lookup, 
>> vdev->name,
>> --->                                            VIRTIO_TYPE_UNKNOWN, NULL);
>>              QAPI_LIST_PREPEND(list, node->value);
>>          }
>>
>>          return list;
>>      }
>>
>> This maps VirtioDevice member @name (a string) to an enum value.
>>
>> As far as I can tell, this member is set in virtio_init() only, to its
>> argument.  All callers pass string literals.  They also pass a numeric
>> device_id, and the two seem to match, e.g. "virtio-blk" and
>> VIRTIO_ID_BLOCK.
>>
>> Thus, the pairs (numeric device ID, string device ID) already exist in
>> the code, but not in a way that lets you map between them.  To get that,
>> you *duplicate* the pairs in QAPI.
>>
>> Having two copies means we get to keep them consistent.  Can we avoid
>> that?
>>
>> The enum has a special member 'unknown' that gets used when @name does
>> not parse as enum member, i.e. we failed at keeping the copies
>> consistent.  I'm afraid this sweeps a programming error under the rug.
>>
>> The enum has a bunch of dummy members like 'unknown-14' to make QAPI
>> generate numeric enum values match the device IDs.  Error prone.  Can't
>> see offhand why we need them to match.
>
> Sure, I don't mind doing this instead. Just as an FYI, from the previous
> RFC series (RFC v5 1/6), David recommended here that I create a list of
> all the types and in the same order as 
> include/standard-headers/linux/virtio_ids.h.
>
> The benefit from this was that we never have to convert between the QAPI 
> number
> and the header number.

Yes, but it comes with the disadvantages I listed above.

> Let me know if this is still something you'd like me to do!

I think it's simpler overall, especially if you can pick option
"2. Without QAPI enum VirtioType" below.

I could be wrong.  Suggest to try it out to see what you like better.

>>
>> What about the following.  Define a map from numeric device ID to
>> string, like so
>>
>>      const char *virtio_device_name[] = {
>>          [VIRTIO_ID_NET] = "virtio-net",
>>          [VIRTIO_ID_BLOCK] = "virtio-blk",
>>          ...
>>      }
>
> Sorry if this is obvious, but where should I define this mapping?
> virtio.c or virtio.h?

Variable definitions normally live in .c.

> Jonah
>
>> This lets you
>>
>> * map device ID to string by subscripting virtio_device_name[].
>>    Guarding with assert(device_id < G_N_ELEMENTS(virtio_device_name)) may
>>    be advisable.
>>
>> * map string to device ID by searching virtio_device_name[].  May want a
>>    function for that,
>>
>> Now you can have virtio_init() map parameter @device_id to string, and
>> drop parameter @name.
>>
>> Then you have two options:
>>
>> 1. With QAPI enum VirtioType
>>
>>     Define it without the special and the dummy members.
>>
>>     To map from string to QAPI enum, use qapi_enum_parse().
>>
>>     To map from QAPI enum to string, use VirtioType_str().
>>
>>     To map from QAPI enum to device ID, map through string.
>>
>> 2. Without QAPI enum VirtioType
>>
>>     Simply use uint16_t device_id, just like struct VirtioDevice.
>>
>> The choice between 1. and 2. depends on whether you actually need
>> additional functionality provided by QAPI, such as types being visible
>> in query-qmp-schema.
>>
>> [...]
>>