Re: [QEMU HCI-8.0 PATCH 3/5] vhost-blk: add read-only flag

[Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>]

> and set RO respectively. Also compare BLKROGET with the selected

Nit: make commit message start with their own sentence.

> mode and reject r/w if needed.
> 
> https://virtuozzo.atlassian.net/browse/VSTOR-143437
> Signed-off-by: Andrey Zhadchenko <andrey.zhadchenko@virtuozzo.com>
>
> diff --git a/hw/block/vhost-blk.c b/hw/block/vhost-blk.c
> index a3e0010982f..47b6e560d47 100644
> --- a/hw/block/vhost-blk.c
> +++ b/hw/block/vhost-blk.c
> @@ -24,6 +24,7 @@
>  #include "system/system.h"
>  #include "linux-headers/linux/vhost.h"
>  #include <sys/ioctl.h>
> +#include <linux/fs.h>

Previous patch removes the header, now we add it back.  Let's just not
touch it.

>  #include "system/runstate.h"
>  
>  static int vhost_blk_start(VirtIODevice *vdev)
> @@ -233,8 +234,10 @@ static bool vhost_blk_open_backend(VHostBlk *s, Error **errp)
>  {
>      BlockConf *conf = &s->conf.conf;
>      struct stat st;
> +    int readonly;
> +    int open_flags = s->conf.readonly ? O_RDONLY : O_RDWR;
>  
> -    s->backend_fd = qemu_open(s->conf.devpath, O_RDWR, errp);
> +    s->backend_fd = qemu_open(s->conf.devpath, open_flags, errp);
>      if (s->backend_fd < 0) {
>          error_prepend(errp, "vhost-blk: unable to open backend: ");
>          return false;
> @@ -252,6 +255,19 @@ static bool vhost_blk_open_backend(VHostBlk *s, Error **errp)
>          goto fail;
>      }
>  
> +    if (ioctl(s->backend_fd, BLKROGET, &readonly) < 0) {
> +        error_setg_errno(errp, errno,
> +                         "vhost-blk: unable to get read-only status of '%s'",
> +                         s->conf.devpath);
> +        goto fail;
> +    }
> +
> +    if (readonly && !s->conf.readonly) {
> +        error_setg(errp, "vhost-blk: '%s' is read-only",
> +                   s->conf.devpath);
> +        goto fail;
> +    }
> +

How about doing it similarly to file-posix:

    if (!s->conf.readonly) {
        if (ioctl(s->backend_fd, BLKROGET, &readonly) < 0) {
            error_setg_errno(errp, errno,
                             "vhost-blk: unable to get read-only status of "
                             "'%s'", s->conf.devpath);
            goto fail;
         }

         if (readonly) {
             error_setg_errno(errp, EROFS, "The device is not writable");
             goto fail;
         }
    }

In addition: conf.readonly value comes from libvirt.  conf.devpath is
also provided by libvirt.  Shouldn't we check BLKROGET early on and fail
in libvirt instead of waiting till here?  I'd prefer having both checks.
Leave the check here, but fail early on in libvirt.

Andrey

-- 
Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>