Re: [RFC v3 04/29] vhost: Add x-vhost-enable-shadow-vq qmp

[Markus Armbruster]

Eugenio Perez Martin <eperezma@redhat.com> writes:

> On Fri, May 21, 2021 at 9:05 AM Markus Armbruster <armbru@redhat.com> wrote:
>>
>> Eugenio Pérez <eperezma@redhat.com> writes:
>>
>> > Command to enable shadow virtqueue looks like:
>> >
>> > { "execute": "x-vhost-enable-shadow-vq",
>> >   "arguments": { "name": "dev0", "enable": true } }
>> >
>> > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
>> > ---
>> >  qapi/net.json     | 22 ++++++++++++++++++++++
>> >  hw/virtio/vhost.c |  6 ++++++
>> >  2 files changed, 28 insertions(+)
>> >
>> > diff --git a/qapi/net.json b/qapi/net.json
>> > index c31748c87f..660feafdd2 100644
>> > --- a/qapi/net.json
>> > +++ b/qapi/net.json
>> > @@ -77,6 +77,28 @@
>> >  ##
>> >  { 'command': 'netdev_del', 'data': {'id': 'str'} }
>> >
>> > +##
>> > +# @x-vhost-enable-shadow-vq:
>> > +#
>> > +# Use vhost shadow virtqueue.
>> > +#
>> > +# @name: the device name of the VirtIO device
>> > +#
>> > +# @enable: true to use he alternate shadow VQ notification path

[...]

>> > +#
>> > +# Returns: Error if failure, or 'no error' for success. Not found if 
>> > vhost is not enabled.
>>
>> This is confusing.  What do you mean by "Not found"?
>>
>> If you mean DeviceNotFound:
>>
>> 1. Not actually true: qmp_x_vhost_enable_shadow_vq() always fails with
>> GenericError.  Perhaps later patches will change that.

[...]

>> 2. Do you really need to distinguish "vhost is not enabled" from other
>> errors?
>>
>
> SVQ cannot work if the device backend is not vhost, like qemu VirtIO
> dev. What I meant is that "qemu will only look for its name in the set
> of vhost devices, so you will have a device not found if the device is
> not a vhost one", which may not be 100% clear at first glance. Maybe
> this wording is better?

We might be talking past each other.  Let me try again :)

The following question is *not* about the doc comment, it's about the
*code*: what practical problem is solved by using DeviceNotFound instead
of GenericError for some errors?

[...]