[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v6 1/6] qmp: add QMP command x-debug-query-virtio
From: |
Markus Armbruster |
Subject: |
Re: [PATCH v6 1/6] qmp: add QMP command x-debug-query-virtio |
Date: |
Mon, 23 Aug 2021 15:27:21 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/27.2 (gnu/linux) |
Back from my summer break, please excuse the delay.
Jonah Palmer <jonah.palmer@oracle.com> writes:
> On 8/7/21 8:35 AM, Markus Armbruster wrote:
>> QAPI schema review only.
>>
>> Jonah Palmer <jonah.palmer@oracle.com> writes:
>>
>>> From: Laurent Vivier <lvivier@redhat.com>
>>>
>>> This new command lists all the instances of VirtIODevice with
>>> their path and virtio type.
>>>
>>> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
>>> Reviewed-by: Eric Blake <eblake@redhat.com>
>>> Signed-off-by: Jonah Palmer <jonah.palmer@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.
[...]