Re: [Qemu-block] [Qemu-devel] [RFC v6 1/2] virtio: introduce `query-virtio' QMP command

[Markus Armbruster]

QAPI/QMP interface review only.

You neglected to cc: the maintainers of qapi-schema.json, so I'm doing
that for you.

Jan Dakinevich <address@hidden> writes:

> The command is intended for gathering virtio information such as status,
> feature bits, negotiation status. It is convenient and useful for debug
> purpose.
>
> The commands returns generic virtio information for virtio such as
> common feature names and status bits names and information for all
> attached to current machine devices.
>
> To retrieve names of device-specific features `tell_feature_name'
> callback in VirtioDeviceClass also was introduced.
>
> Cc: Denis V. Lunev <address@hidden>
> Signed-off-by: Jan Dakinevich <address@hidden>
[...]
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 1845795..51cd0f3 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -3200,3 +3200,73 @@
>  # Since: 2.11
>  ##
>  { 'command': 'watchdog-set-action', 'data' : {'action': 'WatchdogAction'} }
> +
> +##
> +# @VirtioFeature:
> +#
> +# Virtio feature bit with negotiation status
> +#
> +# @name: name of feature bit
> +#
> +# @acked: negotiation status, `true' if guest acknowledged the feature
> +#
> +# Since: 2.12
> +##
> +{
> +    'struct': 'VirtioFeature',
> +    'data': {
> +        'name': 'str',
> +        'acked': 'bool'
> +    }
> +}
> +
> +##
> +# @VirtioInfo:
> +#
> +# Information about virtio device
> +#
> +# @qom-path: QOM path of the device
> +#
> +# @status: status bitmask
> +#
> +# @host-features: bitmask of features, exposed by device
> +#
> +# @guest-features: bitmask of features, acknowledged by guest

Quoting my review of v4: "Unsigned integer interpreted as combination of
well-known bit-valued symbols" is a fine C interface, but a pretty
horrid QMP interface.  What's wrong with doing a set the straightforward
way as "array of enum"?  As far as I can see, I didn't get a reply back
then.  You'll have to give one to get the patch accepted.

In case symbolic names may not be known at QEMU compile time (me having
to wonder at v6 is a sign of rather ineffective patch review, sorry
about that): you have to explain this in the doc comment, with a
reference to where the bits are specified.

> +#
> +# @status-names: names of checked bits in status bitmask

How are the strings in @status-names connected to the bits in @status?
Spell it out in this doc string, please.

> +#
> +# @common-features-names: names exposed features and negotiation status,
> +# that are common to all devices
> +#
> +# @device-features-names: names exposed features and negotiation status,
> +# that are specific to this device

Likewise.

> +#
> +# Since: 2.12
> +##
> +{
> +    'struct': 'VirtioInfo',
> +    'data': {
> +        'qom-path': 'str',
> +
> +        'status': 'uint8',
> +        'host-features': 'uint64',
> +        'guest-features': 'uint64',
> +
> +        'status-names': ['str'],
> +        'common-features-names': ['VirtioFeature'],
> +        'device-features-names': ['VirtioFeature']
> +    }
> +}
> +
> +##
> +# @query-virtio:
> +#
> +# Returns virtio information such as device status and features
> +#
> +# Since: 2.12
> +##
> +{
> +    'command': 'query-virtio',
> +    'data': {'*path': 'str'},
> +    'returns': ['VirtioInfo']
> +}