Re: [PATCH v3 1/1] block/blkio: use qemu_open() to support fd passing for virtio-blk

[Jonathon Jongsma <jjongsma@redhat.com>]

On 5/11/23 4:15 AM, Stefano Garzarella wrote:
> The virtio-blk-vhost-vdpa driver in libblkio 1.3.0 supports the new
> 'fd' property. Let's expose this to the user, so 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.
> 
> If the libblkio virtio-blk driver supports fd passing, let's always
> use qemu_open() to open the `path`, so we can handle fd passing
> from the management layer through the "/dev/fdset/N" special path.
> 
> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> ---
> 
> Notes:
>      v3:
>      - use qemu_open() on `path` to simplify libvirt code [Jonathon]


Thanks

The one drawback now is that it doesn't seem possible for libvirt to 
introspect whether or not qemu supports passing an fd to the driver or 
not. When I was writing my initial patch (before I realized that it was 
missing fd-passing), I just checked for the existence of the 
virtio-blk-vhost-vdpa device. But we actually need to know both that 
this device exists and supports fd passing. As far as I can tell, 
versions 7.2.0 and 8.0.0 include this device but won't accept fds.

Jonathon


> 
>   block/blkio.c | 53 ++++++++++++++++++++++++++++++++++++++++++---------
>   1 file changed, 44 insertions(+), 9 deletions(-)
> 
> diff --git a/block/blkio.c b/block/blkio.c
> index 0cdc99a729..6a6f20f923 100644
> --- a/block/blkio.c
> +++ b/block/blkio.c
> @@ -672,25 +672,60 @@ static int blkio_virtio_blk_common_open(BlockDriverState *bs,
>   {
>       const char *path = qdict_get_try_str(options, "path");
>       BDRVBlkioState *s = bs->opaque;
> -    int ret;
> +    bool fd_supported = false;
> +    int fd, ret;
>   
>       if (!path) {
>           error_setg(errp, "missing 'path' option");
>           return -EINVAL;
>       }
>   
> -    ret = blkio_set_str(s->blkio, "path", path);
> -    qdict_del(options, "path");
> -    if (ret < 0) {
> -        error_setg_errno(errp, -ret, "failed to set path: %s",
> -                         blkio_get_error_msg());
> -        return ret;
> -    }
> -
>       if (!(flags & BDRV_O_NOCACHE)) {
>           error_setg(errp, "cache.direct=off is not supported");
>           return -EINVAL;
>       }
> +
> +    if (blkio_get_int(s->blkio, "fd", &fd) == 0) {
> +        fd_supported = true;
> +    }
> +
> +    /*
> +     * If the libblkio driver supports fd passing, let's always use qemu_open()
> +     * to open the `path`, so we can handle fd passing from the management
> +     * layer through the "/dev/fdset/N" special path.
> +     */
> +    if (fd_supported) {
> +        int open_flags;
> +
> +        if (flags & BDRV_O_RDWR) {
> +            open_flags = O_RDWR;
> +        } else {
> +            open_flags = O_RDONLY;
> +        }
> +
> +        fd = qemu_open(path, open_flags, errp);
> +        if (fd < 0) {
> +            return -EINVAL;
> +        }
> +
> +        ret = blkio_set_int(s->blkio, "fd", fd);
> +        if (ret < 0) {
> +            error_setg_errno(errp, -ret, "failed to set fd: %s",
> +                             blkio_get_error_msg());
> +            qemu_close(fd);
> +            return ret;
> +        }
> +    } else {
> +        ret = blkio_set_str(s->blkio, "path", path);
> +        if (ret < 0) {
> +            error_setg_errno(errp, -ret, "failed to set path: %s",
> +                             blkio_get_error_msg());
> +            return ret;
> +        }
> +    }
> +
> +    qdict_del(options, "path");
> +
>       return 0;
>   }
>