Back from my summer break, please excuse the delay. Jonah Palmer <jonah.pal...@oracle.com> writes:
> On 8/7/21 8:35 AM, Markus Armbruster wrote: >> QAPI schema review only. >> >> Jonah Palmer <jonah.pal...@oracle.com> writes: >> >>> From: Laurent Vivier <lviv...@redhat.com> >>> >>> This new command lists all the instances of VirtIODevice with >>> their path and virtio type. >>> >>> Signed-off-by: Laurent Vivier <lviv...@redhat.com> >>> Reviewed-by: Eric Blake <ebl...@redhat.com> >>> Signed-off-by: Jonah Palmer <jonah.pal...@oracle.com> >> [...] >> >>> diff --git a/qapi/qapi-schema.json b/qapi/qapi-schema.json >>> index 4912b97..0c89789 100644 >>> --- a/qapi/qapi-schema.json >>> +++ b/qapi/qapi-schema.json >>> @@ -91,5 +91,6 @@ >>> { 'include': 'misc.json' } >>> { 'include': 'misc-target.json' } >>> { 'include': 'audio.json' } >>> +{ 'include': 'virtio.json' } >>> { 'include': 'acpi.json' } >>> { 'include': 'pci.json' } >>> diff --git a/qapi/virtio.json b/qapi/virtio.json >>> new file mode 100644 >>> index 0000000..804adbe >>> --- /dev/null >>> +++ b/qapi/virtio.json >>> @@ -0,0 +1,72 @@ >> Please insert at the beginning >> >> # -*- Mode: Python -*- >> # vim: filetype=python >> # > > Will do. > >>> +## >>> +# = Virtio devices >>> +## >>> + >>> +## >>> +# @VirtioType: >>> +# >>> +# An enumeration of Virtio device types. >>> +# >>> +# Since: 6.1 >> 6.2 now, here and below. > > Okay, will update for entire series. > >> >>> +## >>> +{ 'enum': 'VirtioType', >>> + 'data': [ 'unknown', 'virtio-net', 'virtio-blk', 'virtio-console', >>> + 'virtio-rng', 'virtio-balloon', 'virtio-iomem', 'virtio-rpmsg', >>> + 'virtio-scsi', 'virtio-9p', 'virtio-mac80211-wlan', >>> + 'virtio-serial', 'virtio-caif', 'virtio-memory-balloon', >>> + 'unknown-14', 'unknown-15', 'virtio-gpu', 'virtio-clock', >>> + 'virtio-input', 'vhost-vsock', 'virtio-crypto', >>> 'virtio-signal-dist', >>> + 'virtio-pstore', 'virtio-iommu', 'virtio-mem', 'unknown-25', >>> + 'vhost-user-fs', 'virtio-pmem', 'unknown-28', >>> 'virtio-mac80211-hwsim' ] >> Please limit line length to approximately 70 characters. > > Fixed, sorry about that. Also, should I continue this up to > 'virtio-bluetooth'? E.g: > > ... > 'virtio-mac80211-hwsim', 'unknown-30', 'unknown-31', > 'unknown-32', 'unknown-33', 'unknown-34', 'unknown-35', > 'unknown-36', 'unknown-37', 'unknown-38', 'unknown-39', > 'virtio-bluetooth' ] 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. 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", ... } 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. [...]