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

[Naphtali Sprei]

Simon Horman wrote:
> 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?

I don't have a plan, this is a product management issue.

> In particular, I'm curious to know why it will
> be able to be done later but not now.

There must be a way to give users advanced warning of a change coming, so they 
can get prepared for it.
Maybe also some deprecation process needed, don't know how is it done in QEMU.

> 
>> 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.

I felt this is more readable, even though it's a bit longer.

> 
> 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
>>
>>
>>