Re: [PATCH v4 2/2] qapi: add '@fdset' feature for BlockdevOptionsVirtioBlkVhostVdpa

[Jonathon Jongsma <jjongsma@redhat.com>]

On 5/26/23 10:03 AM, Stefano Garzarella wrote:
> The virtio-blk-vhost-vdpa driver in libblkio 1.3.0 supports the fd
> passing through the new 'fd' property.
> 
> Since now we are using qemu_open() on '@path' if the virtio-blk driver
> supports the fd passing, let's announce it.
> In this way, the management layer can pass the file descriptor of an
> already opened vhost-vdpa character device. This is useful especially
> when the device can only be accessed with certain privileges.
> 
> Add the '@fdset' feature only when the virtio-blk-vhost-vdpa driver
> in libblkio supports it.
> 
> Suggested-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> ---
> 
> Notes:
>      v4:
>      - added this patch to allow libvirt to discover we support fdset [Markus]
> 
>   meson.build          | 4 ++++
>   qapi/block-core.json | 8 +++++++-
>   2 files changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/meson.build b/meson.build
> index 78890f0155..8ea911f7b4 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -2108,6 +2108,10 @@ config_host_data.set('CONFIG_LZO', lzo.found())
>   config_host_data.set('CONFIG_MPATH', mpathpersist.found())
>   config_host_data.set('CONFIG_MPATH_NEW_API', mpathpersist_new_api)
>   config_host_data.set('CONFIG_BLKIO', blkio.found())
> +if blkio.found()
> +  config_host_data.set('CONFIG_BLKIO_VHOST_VDPA_FD',
> +                       blkio.version().version_compare('>=1.3.0'))
> +endif
>   config_host_data.set('CONFIG_CURL', curl.found())
>   config_host_data.set('CONFIG_CURSES', curses.found())
>   config_host_data.set('CONFIG_GBM', gbm.found())
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 98d9116dae..1538d84ef4 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -3955,10 +3955,16 @@
>   #
>   # @path: path to the vhost-vdpa character device.
>   #
> +# Features:
> +# @fdset: Member @path supports the special "/dev/fdset/N" path (since 8.1)
> +#
>   # Since: 7.2
>   ##
>   { 'struct': 'BlockdevOptionsVirtioBlkVhostVdpa',
> -  'data': { 'path': 'str' },
> +  'data': { 'path': { 'type': 'str',
> +                      'features': [ { 'name' :'fdset',
> +                                      'if': 'CONFIG_BLKIO_VHOST_VDPA_FD' } ]
> +            } },
>     'if': 'CONFIG_BLKIO' }
>   
>   ##


Take this for what it's worth and do what's best for qemu, but... It's 
easier for libvirt if the 'features' are on the object rather than on 
the 'path' member. Our schema parsing code already supports object 
features but does not yet support features on builtin types.

i.e. something like this just works without any refactoring in libvirt:

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 1538d84ef4..78cfd10cdb 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -3961,11 +3961,11 @@
  # Since: 7.2
  ##
  { 'struct': 'BlockdevOptionsVirtioBlkVhostVdpa',
-  'data': { 'path': { 'type': 'str',
-                      'features': [ { 'name' :'fdset',
-                                      'if': 
'CONFIG_BLKIO_VHOST_VDPA_FD' } ]
-            } },
-  'if': 'CONFIG_BLKIO' }
+  'data': { 'path': 'str' },
+  'features': [ { 'name' :'fdset',
+                'if': 'CONFIG_BLKIO_VHOST_VDPA_FD' } ],
+  'if': 'CONFIG_BLKIO'
+ }

  ##
  # @IscsiTransport: