[v1] block/blkio: do not use open flags in qemu_open()

[PATCH] block/blkio: do not use open flags in qemu_open()

Posted by Stefano Garzarella 9 months, 1 week ago

qemu_open() in blkio_virtio_blk_common_open() is used to open the
character device (e.g. /dev/vhost-vdpa-0 or /dev/vfio/vfio) or in
the future eventually the unix socket.

In all these cases we cannot open the path in read-only mode,
when the `read-only` option of blockdev is on, because the exchange
of IOCTL commands for example will fail.

In order to open the device read-only, we have to use the `read-only`
property of the libblkio driver as we already do in blkio_file_open().

Fixes: cad2ccc395 ("block/blkio: use qemu_open() to support fd passing for virtio-blk")
Reported-by: Qing Wang <qinwang@redhat.com>
Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
---
 block/blkio.c | 10 +---------
 1 file changed, 1 insertion(+), 9 deletions(-)

diff --git a/block/blkio.c b/block/blkio.c
index 1798648134..fe9bf8ea5f 100644
--- a/block/blkio.c
+++ b/block/blkio.c
@@ -686,15 +686,7 @@ static int blkio_virtio_blk_common_open(BlockDriverState *bs,
      * 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);
+        fd = qemu_open(path, O_RDWR, errp);
         if (fd < 0) {
             return -EINVAL;
         }
-- 
2.41.0

Re: [PATCH] block/blkio: do not use open flags in qemu_open()

Posted by Daniel P. Berrangé 9 months, 1 week ago

On Tue, Jul 25, 2023 at 01:11:55PM +0200, Stefano Garzarella wrote:
> qemu_open() in blkio_virtio_blk_common_open() is used to open the
> character device (e.g. /dev/vhost-vdpa-0 or /dev/vfio/vfio) or in
> the future eventually the unix socket.
> 
> In all these cases we cannot open the path in read-only mode,
> when the `read-only` option of blockdev is on, because the exchange
> of IOCTL commands for example will fail.
> 
> In order to open the device read-only, we have to use the `read-only`
> property of the libblkio driver as we already do in blkio_file_open().
> 
> Fixes: cad2ccc395 ("block/blkio: use qemu_open() to support fd passing for virtio-blk")
> Reported-by: Qing Wang <qinwang@redhat.com>
> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> ---
>  block/blkio.c | 10 +---------
>  1 file changed, 1 insertion(+), 9 deletions(-)
> 
> diff --git a/block/blkio.c b/block/blkio.c
> index 1798648134..fe9bf8ea5f 100644
> --- a/block/blkio.c
> +++ b/block/blkio.c
> @@ -686,15 +686,7 @@ static int blkio_virtio_blk_common_open(BlockDriverState *bs,
>       * 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);

I'd suggest taking the paragraph from the commit message explaining why
it is correct to hardcode O_RDWR and putting it into a comment here.

> +        fd = qemu_open(path, O_RDWR, errp);
>          if (fd < 0) {
>              return -EINVAL;
>          }
> -- 
> 2.41.0
> 
> 

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH] block/blkio: do not use open flags in qemu_open()

Posted by Stefano Garzarella 9 months, 1 week ago

On Tue, Jul 25, 2023 at 12:15:40PM +0100, Daniel P. Berrangé wrote:
>On Tue, Jul 25, 2023 at 01:11:55PM +0200, Stefano Garzarella wrote:
>> qemu_open() in blkio_virtio_blk_common_open() is used to open the
>> character device (e.g. /dev/vhost-vdpa-0 or /dev/vfio/vfio) or in
>> the future eventually the unix socket.
>>
>> In all these cases we cannot open the path in read-only mode,
>> when the `read-only` option of blockdev is on, because the exchange
>> of IOCTL commands for example will fail.
>>
>> In order to open the device read-only, we have to use the `read-only`
>> property of the libblkio driver as we already do in blkio_file_open().
>>
>> Fixes: cad2ccc395 ("block/blkio: use qemu_open() to support fd passing for virtio-blk")
>> Reported-by: Qing Wang <qinwang@redhat.com>
>> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
>> ---
>>  block/blkio.c | 10 +---------
>>  1 file changed, 1 insertion(+), 9 deletions(-)
>>
>> diff --git a/block/blkio.c b/block/blkio.c
>> index 1798648134..fe9bf8ea5f 100644
>> --- a/block/blkio.c
>> +++ b/block/blkio.c
>> @@ -686,15 +686,7 @@ static int blkio_virtio_blk_common_open(BlockDriverState *bs,
>>       * 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);
>
>I'd suggest taking the paragraph from the commit message explaining why
>it is correct to hardcode O_RDWR and putting it into a comment here.

Ack, I'd also add:

Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=2225439

in the commit message.

I'll fix in v2.

Thanks,
Stefano

>
>> +        fd = qemu_open(path, O_RDWR, errp);
>>          if (fd < 0) {
>>              return -EINVAL;
>>          }
>> --
>> 2.41.0
>>
>>
>
>With regards,
>Daniel
>-- 
>|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
>|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
>|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
>