Re: [Qemu-devel] [PATCH] A different way to ask for readonly drive

[Kevin Wolf]

Am 14.12.2009 14:35, schrieb Naphtali Sprei:
> Hi,
> After feedback from Red Hat guys, I've decided to slightly modify the 
> approach to drive's readonly.
> The new approach also addresses the silent fall-back to open the drives' file 
> as read-only when read-write fails
> (permission denied) that causes unexpected behavior.
> Instead of the 'readonly' boolean flag, another flag introduced (a 
> replacement), 'read_write' with three values [on|off|try]:
> read_write=on : open with read and write permission, no fall-back to read-only
> read_write=off: open with read-only permission
> read_write=try: open with read and write permission and if fails, fall-back 
> to read-only (the default if nothing specified)
> 
> Suggestions for better naming for flag/values welcomed.
> 
> I've tried to explicitly pass the required flags for the bdrv_open function 
> in callers, but probably missed some.
> 
>  Naphtali
> 
> 
> 
> Signed-off-by: Naphtali Sprei <address@hidden>
> ---
>  block.c       |   29 +++++++++++++++++------------
>  block.h       |    7 +++++--
>  hw/xen_disk.c |    3 ++-
>  monitor.c     |    2 +-
>  qemu-config.c |    4 ++--
>  qemu-img.c    |   14 ++++++++------
>  qemu-nbd.c    |    2 +-
>  vl.c          |   42 +++++++++++++++++++++++++++++++++---------
>  8 files changed, 69 insertions(+), 34 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 3f3496e..75788ca 100644
> --- a/block.c
> +++ b/block.c
> @@ -356,7 +356,7 @@ int bdrv_open(BlockDriverState *bs, const char *filename, 
> int flags)
>  int bdrv_open2(BlockDriverState *bs, const char *filename, int flags,
>                 BlockDriver *drv)
>  {
> -    int ret, open_flags, try_rw;
> +    int ret, open_flags;
>      char tmp_filename[PATH_MAX];
>      char backing_filename[PATH_MAX];
>  
> @@ -378,7 +378,7 @@ int bdrv_open2(BlockDriverState *bs, const char 
> *filename, int flags,
>  
>          /* if there is a backing file, use it */
>          bs1 = bdrv_new("");
> -        ret = bdrv_open2(bs1, filename, 0, drv);
> +        ret = bdrv_open2(bs1, filename, BDRV_O_RDONLY, drv);
>          if (ret < 0) {
>              bdrv_delete(bs1);
>              return ret;
> @@ -445,19 +445,22 @@ int bdrv_open2(BlockDriverState *bs, const char 
> *filename, int flags,
>          bs->enable_write_cache = 1;
>  
>      /* Note: for compatibility, we open disk image files as RDWR, and
> -       RDONLY as fallback */
> -    try_rw = !bs->read_only || bs->is_temporary;
> -    if (!(flags & BDRV_O_FILE))
> -        open_flags = (try_rw ? BDRV_O_RDWR : 0) |
> -            (flags & (BDRV_O_CACHE_MASK|BDRV_O_NATIVE_AIO));
> -    else
> +       RDONLY as fallback (if flag BDRV_O_RO_FBCK is on) */
> +    bs->read_only = BDRV_FLAGS_IS_RO(flags);
> +    if (!(flags & BDRV_O_FILE)) {
> +        open_flags = (flags & (BDRV_O_ACCESS | 
> BDRV_O_CACHE_MASK|BDRV_O_NATIVE_AIO));
> +        if (bs->is_temporary) { /* snapshot */
> +            open_flags |= BDRV_O_RDWR;

open_flags = (open_flags & ~BDRV_O_ACCESS) | BDRV_O_RDWR;

Yes, I know, RDONLY is 0 anyway, but still... Or move the first
open_flags line into the if and have a second one for is_temporary that
doesn't copy BDRV_O_ACCESS.

> +        }
> +    } else
>          open_flags = flags & ~(BDRV_O_FILE | BDRV_O_SNAPSHOT);
>      if (use_bdrv_whitelist && !bdrv_is_whitelisted(drv))
>          ret = -ENOTSUP;
>      else
>          ret = drv->bdrv_open(bs, filename, open_flags);
> -    if ((ret == -EACCES || ret == -EPERM) && !(flags & BDRV_O_FILE)) {
> -        ret = drv->bdrv_open(bs, filename, open_flags & ~BDRV_O_RDWR);
> +    if ((ret == -EACCES || ret == -EPERM) && !(flags & BDRV_O_FILE) &&
> +        (flags & BDRV_O_RO_FBCK)) {
> +        ret = drv->bdrv_open(bs, filename, (open_flags & ~BDRV_O_RDWR) | 
> BDRV_O_RDONLY);

Mask BDRV_O_ACCESS out, not only BDRV_O_RDWR.

>          bs->read_only = 1;
>      }
>      if (ret < 0) {
> @@ -481,12 +484,14 @@ int bdrv_open2(BlockDriverState *bs, const char 
> *filename, int flags,
>          /* if there is a backing file, use it */
>          BlockDriver *back_drv = NULL;
>          bs->backing_hd = bdrv_new("");
> -        /* pass on read_only property to the backing_hd */
> -        bs->backing_hd->read_only = bs->read_only;
>          path_combine(backing_filename, sizeof(backing_filename),
>                       filename, bs->backing_file);
>          if (bs->backing_format[0] != '\0')
>              back_drv = bdrv_find_format(bs->backing_format);
> +        /* pass on ro flags to the backing_hd */
> +        bs->backing_hd->read_only =  BDRV_FLAGS_IS_RO(flags);
> +        open_flags &= ~BDRV_O_ACCESS;
> +        open_flags |= (flags & BDRV_O_ACCESS);
>          ret = bdrv_open2(bs->backing_hd, backing_filename, open_flags,
>                           back_drv);
>          if (ret < 0) {
> diff --git a/block.h b/block.h
> index fa51ddf..b32c6a4 100644
> --- a/block.h
> +++ b/block.h
> @@ -28,8 +28,9 @@ typedef struct QEMUSnapshotInfo {
>  } QEMUSnapshotInfo;
>  
>  #define BDRV_O_RDONLY      0x0000
> -#define BDRV_O_RDWR        0x0002
> -#define BDRV_O_ACCESS      0x0003
> +#define BDRV_O_RDWR        0x0001
> +#define BDRV_O_ACCESS      0x0001

Is this needed? I can't see why we would need more than a flag that says
if we are read-write or not, but if you were to remove the old two-bit
field, you should do the complete cleanup. There is not reason for
BDRV_O_ACCESS to exist then any more.

In any case I don't think that saving the wasted bit belong into this
patch, so better separate it out.

> +#define BDRV_O_RO_FBCK     0x0002

Come on, we can afford the four additional letters to make it
BDRV_O_RO_FALLBACK and suddenly it becomes readable.

>  #define BDRV_O_CREAT       0x0004 /* create an empty file */
>  #define BDRV_O_SNAPSHOT    0x0008 /* open the file read only and save writes 
> in a snapshot */
>  #define BDRV_O_FILE        0x0010 /* open as a raw file (do not try to
> @@ -41,6 +42,8 @@ typedef struct QEMUSnapshotInfo {
>  #define BDRV_O_NATIVE_AIO  0x0080 /* use native AIO instead of the thread 
> pool */
>  
>  #define BDRV_O_CACHE_MASK  (BDRV_O_NOCACHE | BDRV_O_CACHE_WB)
> +#define BDRV_O_DFLT_OPEN   (BDRV_O_RDWR | BDRV_O_RO_FBCK)

Same here, what's wrong with spelling DEFAULT out?

> +#define BDRV_FLAGS_IS_RO(flags) ((flags & BDRV_O_ACCESS) == BDRV_O_RDONLY)
>  
>  #define BDRV_SECTOR_BITS   9
>  #define BDRV_SECTOR_SIZE   (1 << BDRV_SECTOR_BITS)
> diff --git a/hw/xen_disk.c b/hw/xen_disk.c
> index 5c55251..13688db 100644
> --- a/hw/xen_disk.c
> +++ b/hw/xen_disk.c
> @@ -610,7 +610,8 @@ static int blk_init(struct XenDevice *xendev)
>      /* read-only ? */
>      if (strcmp(blkdev->mode, "w") == 0) {
>       mode   = O_RDWR;
> -     qflags = BDRV_O_RDWR;
> +        /* for compatibility, also allow readonly fallback, for now */
> +     qflags = BDRV_O_RDWR | BDRV_O_RO_FBCK;
>      } else {
>       mode   = O_RDONLY;
>       qflags = BDRV_O_RDONLY;
> diff --git a/monitor.c b/monitor.c
> index d97d529..440e17e 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -910,7 +910,7 @@ static void do_change_block(Monitor *mon, const char 
> *device,
>      }
>      if (eject_device(mon, bs, 0) < 0)
>          return;
> -    bdrv_open2(bs, filename, 0, drv);
> +    bdrv_open2(bs, filename, BDRV_O_DFLT_OPEN, drv);
>      monitor_read_bdrv_key_start(mon, bs, NULL, NULL);
>  }
>  
> diff --git a/qemu-config.c b/qemu-config.c
> index c3203c8..b559459 100644
> --- a/qemu-config.c
> +++ b/qemu-config.c
> @@ -76,8 +76,8 @@ QemuOptsList qemu_drive_opts = {
>              .type = QEMU_OPT_STRING,
>              .help = "pci address (virtio only)",
>          },{
> -            .name = "readonly",
> -            .type = QEMU_OPT_BOOL,
> +            .name = "read_write",
> +            .type = QEMU_OPT_STRING,
>          },
>          { /* end if list */ }
>      },
> diff --git a/qemu-img.c b/qemu-img.c
> index 1d97f2e..dee3e60 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -201,7 +201,7 @@ static BlockDriverState *bdrv_new_open(const char 
> *filename,
>      } else {
>          drv = NULL;
>      }
> -    if (bdrv_open2(bs, filename, BRDV_O_FLAGS, drv) < 0) {
> +    if (bdrv_open2(bs, filename, BRDV_O_FLAGS | BDRV_O_DFLT_OPEN, drv) < 0) {
>          error("Could not open '%s'", filename);
>      }
>      if (bdrv_is_encrypted(bs)) {
> @@ -406,7 +406,7 @@ static int img_check(int argc, char **argv)
>      } else {
>          drv = NULL;
>      }
> -    if (bdrv_open2(bs, filename, BRDV_O_FLAGS, drv) < 0) {
> +    if (bdrv_open2(bs, filename, BRDV_O_FLAGS | BDRV_O_RDONLY, drv) < 0) {
>          error("Could not open '%s'", filename);
>      }
>      ret = bdrv_check(bs);
> @@ -465,7 +465,7 @@ static int img_commit(int argc, char **argv)
>      } else {
>          drv = NULL;
>      }
> -    if (bdrv_open2(bs, filename, BRDV_O_FLAGS, drv) < 0) {
> +    if (bdrv_open2(bs, filename, BRDV_O_FLAGS | BDRV_O_RDWR, drv) < 0) {
>          error("Could not open '%s'", filename);
>      }
>      ret = bdrv_commit(bs);
> @@ -884,7 +884,7 @@ static int img_info(int argc, char **argv)
>      } else {
>          drv = NULL;
>      }
> -    if (bdrv_open2(bs, filename, BRDV_O_FLAGS, drv) < 0) {
> +    if (bdrv_open2(bs, filename, BRDV_O_FLAGS | BDRV_O_RDONLY, drv) < 0) {
>          error("Could not open '%s'", filename);
>      }
>      bdrv_get_format(bs, fmt_name, sizeof(fmt_name));
> @@ -932,10 +932,11 @@ static int img_snapshot(int argc, char **argv)
>      BlockDriverState *bs;
>      QEMUSnapshotInfo sn;
>      char *filename, *snapshot_name = NULL;
> -    int c, ret;
> +    int c, ret, bdrv_oflags;
>      int action = 0;
>      qemu_timeval tv;
>  
> +    bdrv_oflags = BDRV_O_RDWR; /* but no read-only fallback */
>      /* Parse commandline parameters */
>      for(;;) {
>          c = getopt(argc, argv, "la:c:d:h");
> @@ -951,6 +952,7 @@ static int img_snapshot(int argc, char **argv)
>                  return 0;
>              }
>              action = SNAPSHOT_LIST;
> +            bdrv_oflags = BDRV_O_RDONLY; /* no need for RW */
>              break;
>          case 'a':
>              if (action) {
> @@ -988,7 +990,7 @@ static int img_snapshot(int argc, char **argv)
>      if (!bs)
>          error("Not enough memory");
>  
> -    if (bdrv_open2(bs, filename, 0, NULL) < 0) {
> +    if (bdrv_open2(bs, filename, bdrv_oflags, NULL) < 0) {

Not related to your patch, but I think we want to have BRDV_O_FLAGS
here, too. And we need to get rid of that typo some time. Well, I guess
something for my own to-do list.

>          error("Could not open '%s'", filename);
>      }
>  
> diff --git a/qemu-nbd.c b/qemu-nbd.c
> index 6cdb834..64f4c68 100644
> --- a/qemu-nbd.c
> +++ b/qemu-nbd.c
> @@ -212,7 +212,7 @@ int main(int argc, char **argv)
>      int opt_ind = 0;
>      int li;
>      char *end;
> -    int flags = 0;
> +    int flags = BDRV_O_DFLT_OPEN;
>      int partition = -1;
>      int ret;
>      int shared = 1;
> diff --git a/vl.c b/vl.c
> index c0d98f5..cef2d67 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -2090,6 +2090,7 @@ DriveInfo *drive_init(QemuOpts *opts, void *opaque,
>                        int *fatal_error)
>  {
>      const char *buf;
> +    const char *rw_buf = 0;
>      const char *file = NULL;
>      char devname[128];
>      const char *serial;
> @@ -2104,12 +2105,12 @@ DriveInfo *drive_init(QemuOpts *opts, void *opaque,
>      int index;
>      int cache;
>      int aio = 0;
> -    int ro = 0;
>      int bdrv_flags;
>      int on_read_error, on_write_error;
>      const char *devaddr;
>      DriveInfo *dinfo;
>      int snapshot = 0;
> +    int read_write, ro_fallback;
>  
>      *fatal_error = 1;
>  
> @@ -2137,7 +2138,7 @@ DriveInfo *drive_init(QemuOpts *opts, void *opaque,
>      secs  = qemu_opt_get_number(opts, "secs", 0);
>  
>      snapshot = qemu_opt_get_bool(opts, "snapshot", 0);
> -    ro = qemu_opt_get_bool(opts, "readonly", 0);
> +    rw_buf = qemu_opt_get(opts, "read_write");
>  
>      file = qemu_opt_get(opts, "file");
>      serial = qemu_opt_get(opts, "serial");
> @@ -2420,6 +2421,29 @@ DriveInfo *drive_init(QemuOpts *opts, void *opaque,
>          *fatal_error = 0;
>          return NULL;
>      }
> +
> +    read_write = 1;
> +    ro_fallback = 1;
> +    if (rw_buf) {
> +        if (!strcmp(rw_buf, "off")) {
> +            read_write = 0;
> +            ro_fallback = 0;
> +            if (type != IF_SCSI && type != IF_VIRTIO && type != IF_FLOPPY) {
> +                fprintf(stderr, "qemu: read_write=off flag not supported for 
> drive of this interface\n");
> +                return NULL;
> +            }
> +        } else if (!strcmp(rw_buf, "on")) {
> +            read_write = 1;
> +            ro_fallback = 0;
> +        } else if (!strcmp(rw_buf, "try")) { /* default, but keep it 
> explicit */
> +            read_write = 1;
> +            ro_fallback = 1;

We probably should have the check or SCSI/virtio/floppy here, too. If
the user explicitly requests that we should try read-only and it's not
available with the device I think that's a reason to exit with an error.

> +        } else {
> +            fprintf(stderr, "qemu: '%s' invalid read_write option (valid 
> values: [on|off|try] )\n", buf);
> +            return NULL;
> +        }
> +    }
> +    
>      bdrv_flags = 0;
>      if (snapshot) {
>          bdrv_flags |= BDRV_O_SNAPSHOT;
> @@ -2436,16 +2460,16 @@ DriveInfo *drive_init(QemuOpts *opts, void *opaque,
>          bdrv_flags &= ~BDRV_O_NATIVE_AIO;
>      }
>  
> -    if (ro == 1) {
> -        if (type == IF_IDE) {
> -            fprintf(stderr, "qemu: readonly flag not supported for drive 
> with ide interface\n");
> -            return NULL;
> -        }
> -        (void)bdrv_set_read_only(dinfo->bdrv, 1);
> +    if (read_write) {
> +        bdrv_flags |= BDRV_O_RDWR;
> +    }

If BDRV_O_ACCESS continues to exist, BDRV_O_RDWR is not a flag but a
value for that field. To make things clearer, I'd prefer something like
this then:

/* Set BDRV_O_ACCESS value */
bdrv_flags |= read_write ? BDRV_O_RDWR : BDRV_O_RDONLY;

> +    if (ro_fallback) {
> +        bdrv_flags |= BDRV_O_RO_FBCK;
>      }
> +  
>  
>      if (bdrv_open2(dinfo->bdrv, file, bdrv_flags, drv) < 0) {
> -        fprintf(stderr, "qemu: could not open disk image %s: %s\n",
> +        fprintf(stderr, "qemu: could not open disk image %s with requested 
> permission: %s\n",
>                          file, strerror(errno));
>          return NULL;
>      }

Kevin