Re: [Qemu-devel] [PATCH] Added 'access' option to -drive flag

[Simon Horman]

On Wed, Dec 23, 2009 at 02:12:23PM +0200, Naphtali Sprei wrote:
> Added 'access' option to -drive flag
> 
> The new option is: access=[rw|ro|auto]
> rw: open the drive's file with Read and Write permission, don't continue if 
> failed
> ro: open the file only with Read permission
> auto: open the file with Read and Write permission, if failed, try only Read 
> permision
> 
> For compatibility reasons, the default is 'auto'. Should be changed later on.

What is the plan for changing the default later?
In particular, I'm curious to know why it will
be able to be done later but not now.

> This option is to replace the 'readonly' options added lately.
> 
> Instead of using the field 'readonly' of the BlockDriverState struct for 
> passing the request,
> pass the request in the flags parameter to the function.
> 
> Signed-off-by: Naphtali Sprei <address@hidden>
> ---
>  block.c           |   27 +++++++++++++++------------
>  block.h           |    6 ++++--
>  block/raw-posix.c |    2 +-
>  block/raw-win32.c |    4 ++--
>  hw/xen_disk.c     |    3 ++-
>  monitor.c         |    2 +-
>  qemu-config.c     |    4 ++--
>  qemu-img.c        |   14 ++++++++------
>  qemu-nbd.c        |    2 +-
>  qemu-options.hx   |    2 +-
>  vl.c              |   42 +++++++++++++++++++++++++++++++++---------
>  11 files changed, 70 insertions(+), 38 deletions(-)
> 

[snip]

> index e881e45..79b8c27 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 *access_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,6 @@ 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);
>  
>      file = qemu_opt_get(opts, "file");
>      serial = qemu_opt_get(opts, "serial");
> @@ -2420,6 +2420,31 @@ DriveInfo *drive_init(QemuOpts *opts, void *opaque,
>          *fatal_error = 0;
>          return NULL;
>      }
> +
> +    read_write = 1;
> +    ro_fallback = 1;
> +    access_buf = qemu_opt_get(opts, "access");
> +    if (access_buf) {
> +        if (!strcmp(access_buf, "ro")) {
> +            read_write = 0;
> +            ro_fallback = 0;
> +        } else if (!strcmp(access_buf, "rw")) {
> +            read_write = 1;
> +            ro_fallback = 0;
> +        } else if (!strcmp(access_buf, "auto")) { /* default, but keep it 
> explicit */
> +            read_write = 1;
> +            ro_fallback = 1;
> +        } else {
> +            fprintf(stderr, "qemu: '%s' invalid 'access' option (valid 
> values: [rw|ro|auto] )\n", access_buf);
> +            return NULL;
> +        }
> +        if (type != IF_SCSI && type != IF_VIRTIO && type != IF_FLOPPY &&
> +            ( !strcmp(access_buf, "ro") || !strcmp(access_buf, "auto") )) {
> +            fprintf(stderr, "qemu: access=%s flag not supported for drive of 
> this interface\n", access_buf);
> +            return NULL;
> +        }
> +    }
> +    
I wonder if it would be easier for the parsing
code to use flags directly rather than abstracting
things out to read_write, ro_fallback.

e.g.

(Sorry if I mucked up the logic)

    int access_flags = BDRV_O_RDWR | BDRV_O_RO_FALLBACK;
    access_buf = qemu_opt_get(opts, "access");
    if (access_buf) {
        if (!strcmp(access_buf, "ro"))
            access_flags = BDRV_O_RDONLY;
        else if (!strcmp(access_buf, "rw")) 
            access_flags = BDRV_O_RDWR;
        else if (!strcmp(access_buf, "auto")) { /* default, but keep it 
explicit */
            access_flags = BDRV_O_RDWR | BDRV_O_RO_FALLBACK;
        ...

        bdrv_flags |= access_flags;

> diff --git a/vl.c b/vl.c
>      bdrv_flags = 0;
>      if (snapshot) {
>          bdrv_flags |= BDRV_O_SNAPSHOT;
> @@ -2436,16 +2461,15 @@ 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);
> +    bdrv_flags |= read_write ? BDRV_O_RDWR : BDRV_O_RDONLY;
> +
> +    if (ro_fallback) {
> +        bdrv_flags |= BDRV_O_RO_FALLBACK;
>      }
> +  
>  
>      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;
>      }
> -- 
> 1.6.3.3
> 
> 
>