Jonah Palmer <jonah.pal...@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.
>>
>> [...]
>>
