[Qemu-devel] [PATCH v2 0/4] Modifications to the drives' readonly attribute

[Qemu-devel] [PATCH v2 0/4] Modifications to the drives' readonly attribute

[Naphtali Sprei]

This is version 2, to replace previous set.
Addresses all Kevin comments.


Naphtali Sprei (4):
  Make CDROM a read-only drive
  Clean-up a little bit the RW related bits of BDRV_O_FLAGS.
    BDRV_O_RDONLY gone (and so is BDRV_O_ACCESS). Default value for
    bdrv_flags (0/zero) is READ-ONLY. Need to explicitly request
    READ-WRITE.
  Added drives' readonly option
  Disable fall-back to read-only when cannot open drive's file for
    read-write

 block.c           |   29 +++++++++++++----------------
 block.h           |    2 --
 block/raw-posix.c |    2 +-
 block/raw-win32.c |    4 ++--
 block/vmdk.c      |    9 +++++----
 hw/xen_disk.c     |    2 +-
 monitor.c         |    2 +-
 qemu-img.c        |   10 ++++++----
 qemu-io.c         |   14 ++++++--------
 qemu-nbd.c        |    2 +-
 qemu-options.hx   |    2 +-
 vl.c              |   13 ++++++++++---
 12 files changed, 47 insertions(+), 44 deletions(-)

[Qemu-devel] [PATCH v2 1/4] Make CDROM a read-only drive

[Naphtali Sprei]

Signed-off-by: Naphtali Sprei <nsprei@redhat.com>
---
 vl.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/vl.c b/vl.c
index 06cb40d..76ef8ca 100644
--- a/vl.c
+++ b/vl.c
@@ -2233,6 +2233,13 @@ DriveInfo *drive_init(QemuOpts *opts, void *opaque,
         }
         (void)bdrv_set_read_only(dinfo->bdrv, 1);
     }
+    /* 
+     * cdrom is read-only. Set it now, after above interface checking
+     * since readonly attribute not explicitly required, so no error.
+     */
+    if (media == MEDIA_CDROM) {
+        (void)bdrv_set_read_only(dinfo->bdrv, 1);
+    }
 
     if (bdrv_open2(dinfo->bdrv, file, bdrv_flags, drv) < 0) {
         fprintf(stderr, "qemu: could not open disk image %s: %s\n",
-- 
1.6.3.3

[Qemu-devel] [PATCH v2 2/4] Clean-up a little bit the RW related bits of BDRV_O_FLAGS. BDRV_O_RDONLY gone (and so is BDRV_O_ACCESS). Default value for bdrv_flags (0/zero) is READ-ONLY. Need to explicitly request READ-WRITE.

[Naphtali Sprei]

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 <nsprei@redhat.com>
---
 block.c           |   31 +++++++++++++++++--------------
 block.h           |    2 --
 block/raw-posix.c |    2 +-
 block/raw-win32.c |    4 ++--
 block/vmdk.c      |    9 +++++----
 hw/xen_disk.c     |    2 +-
 monitor.c         |    2 +-
 qemu-img.c        |   10 ++++++----
 qemu-io.c         |   14 ++++++--------
 qemu-nbd.c        |    2 +-
 vl.c              |    8 ++++----
 11 files changed, 44 insertions(+), 42 deletions(-)

diff --git a/block.c b/block.c
index 115e591..8def3c4 100644
--- a/block.c
+++ b/block.c
@@ -310,7 +310,7 @@ static BlockDriver *find_image_format(const char *filename)
     if (drv && strcmp(drv->format_name, "vvfat") == 0)
         return drv;
 
-    ret = bdrv_file_open(&bs, filename, BDRV_O_RDONLY);
+    ret = bdrv_file_open(&bs, filename, 0);
     if (ret < 0)
         return NULL;
     ret = bdrv_pread(bs, 0, buf, sizeof(buf));
@@ -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];
 
@@ -446,19 +446,23 @@ int bdrv_open2(BlockDriverState *bs, const char *filename, int flags,
 
     /* 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
+    bs->read_only = (flags & BDRV_O_RDWR) == 0;
+    if (!(flags & BDRV_O_FILE)) {
+        open_flags = (flags & (BDRV_O_RDWR | BDRV_O_CACHE_MASK|BDRV_O_NATIVE_AIO));
+        if (bs->is_temporary) { /* snapshot should be writeable */
+            open_flags |= BDRV_O_RDWR;
+        }
+    } else {
         open_flags = flags & ~(BDRV_O_FILE | BDRV_O_SNAPSHOT);
-    if (use_bdrv_whitelist && !bdrv_is_whitelisted(drv))
+    }
+    if (use_bdrv_whitelist && !bdrv_is_whitelisted(drv)) {
         ret = -ENOTSUP;
-    else
+    } 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);
-        bs->read_only = 1;
+        if ((ret == -EACCES || ret == -EPERM) && !(flags & BDRV_O_FILE)) {
+            ret = drv->bdrv_open(bs, filename, open_flags & ~BDRV_O_RDWR);
+            bs->read_only = 1;
+        }
     }
     if (ret < 0) {
         qemu_free(bs->opaque);
@@ -481,14 +485,13 @@ 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);
         ret = bdrv_open2(bs->backing_hd, backing_filename, open_flags,
                          back_drv);
+        bs->backing_hd->read_only =  (open_flags & BDRV_O_RDWR) == 0;
         if (ret < 0) {
             bdrv_close(bs);
             return ret;
diff --git a/block.h b/block.h
index bee9ec5..fd4e8dd 100644
--- a/block.h
+++ b/block.h
@@ -27,9 +27,7 @@ typedef struct QEMUSnapshotInfo {
     uint64_t vm_clock_nsec; /* VM clock relative to boot */
 } QEMUSnapshotInfo;
 
-#define BDRV_O_RDONLY      0x0000
 #define BDRV_O_RDWR        0x0002
-#define BDRV_O_ACCESS      0x0003
 #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
diff --git a/block/raw-posix.c b/block/raw-posix.c
index 5a6a22b..b427211 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -138,7 +138,7 @@ static int raw_open_common(BlockDriverState *bs, const char *filename,
 
     s->open_flags = open_flags | O_BINARY;
     s->open_flags &= ~O_ACCMODE;
-    if ((bdrv_flags & BDRV_O_ACCESS) == BDRV_O_RDWR) {
+    if (bdrv_flags & BDRV_O_RDWR) {
         s->open_flags |= O_RDWR;
     } else {
         s->open_flags |= O_RDONLY;
diff --git a/block/raw-win32.c b/block/raw-win32.c
index 72acad5..01a6d25 100644
--- a/block/raw-win32.c
+++ b/block/raw-win32.c
@@ -81,7 +81,7 @@ static int raw_open(BlockDriverState *bs, const char *filename, int flags)
 
     s->type = FTYPE_FILE;
 
-    if ((flags & BDRV_O_ACCESS) == O_RDWR) {
+    if (flags & BDRV_O_RDWR) {
         access_flags = GENERIC_READ | GENERIC_WRITE;
     } else {
         access_flags = GENERIC_READ;
@@ -337,7 +337,7 @@ static int hdev_open(BlockDriverState *bs, const char *filename, int flags)
     }
     s->type = find_device_type(bs, filename);
 
-    if ((flags & BDRV_O_ACCESS) == O_RDWR) {
+    if (flags & BDRV_O_RDWR) {
         access_flags = GENERIC_READ | GENERIC_WRITE;
     } else {
         access_flags = GENERIC_READ;
diff --git a/block/vmdk.c b/block/vmdk.c
index 4e48622..ddc2fcb 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -357,7 +357,7 @@ static int vmdk_parent_open(BlockDriverState *bs, const char * filename)
             return -1;
         }
         parent_open = 1;
-        if (bdrv_open(bs->backing_hd, parent_img_name, BDRV_O_RDONLY) < 0)
+        if (bdrv_open(bs->backing_hd, parent_img_name, 0) < 0)
             goto failure;
         parent_open = 0;
     }
@@ -371,9 +371,10 @@ static int vmdk_open(BlockDriverState *bs, const char *filename, int flags)
     uint32_t magic;
     int l1_size, i, ret;
 
-    if (parent_open)
-        // Parent must be opened as RO.
-        flags = BDRV_O_RDONLY;
+    if (parent_open) {
+        /* Parent must be opened as RO, no RDWR. */
+        flags = 0;
+    }
 
     ret = bdrv_file_open(&s->hd, filename, flags);
     if (ret < 0)
diff --git a/hw/xen_disk.c b/hw/xen_disk.c
index 5c55251..beadf90 100644
--- a/hw/xen_disk.c
+++ b/hw/xen_disk.c
@@ -613,7 +613,7 @@ static int blk_init(struct XenDevice *xendev)
 	qflags = BDRV_O_RDWR;
     } else {
 	mode   = O_RDONLY;
-	qflags = BDRV_O_RDONLY;
+	qflags = 0;
 	info  |= VDISK_READONLY;
     }
 
diff --git a/monitor.c b/monitor.c
index b824e7c..5bb8872 100644
--- a/monitor.c
+++ b/monitor.c
@@ -916,7 +916,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_RDWR, drv);
     monitor_read_bdrv_key_start(mon, bs, NULL, NULL);
 }
 
diff --git a/qemu-img.c b/qemu-img.c
index 48b9315..3cea8ce 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -204,7 +204,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_RDWR, drv) < 0) {
         error("Could not open '%s'", filename);
     }
     if (bdrv_is_encrypted(bs)) {
@@ -468,7 +468,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);
@@ -966,10 +966,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;
     /* Parse commandline parameters */
     for(;;) {
         c = getopt(argc, argv, "la:c:d:h");
@@ -985,6 +986,7 @@ static int img_snapshot(int argc, char **argv)
                 return 0;
             }
             action = SNAPSHOT_LIST;
+            bdrv_oflags &= ~BDRV_O_RDWR; /* no need for RW */
             break;
         case 'a':
             if (action) {
@@ -1022,7 +1024,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) {
         error("Could not open '%s'", filename);
     }
 
diff --git a/qemu-io.c b/qemu-io.c
index 1c19b92..b159bc9 100644
--- a/qemu-io.c
+++ b/qemu-io.c
@@ -1359,10 +1359,9 @@ open_f(int argc, char **argv)
 		}
 	}
 
-	if (readonly)
-		flags |= BDRV_O_RDONLY;
-	else
-		flags |= BDRV_O_RDWR;
+	if (!readonly) {
+            flags |= BDRV_O_RDWR;
+        }
 
 	if (optind != argc - 1)
 		return command_usage(&open_cmd);
@@ -1506,10 +1505,9 @@ int main(int argc, char **argv)
 	add_check_command(init_check_command);
 
 	/* open the device */
-	if (readonly)
-		flags |= BDRV_O_RDONLY;
-	else
-		flags |= BDRV_O_RDWR;
+	if (!readonly) {
+            flags |= BDRV_O_RDWR;
+        }
 
 	if ((argc - optind) == 1)
 		openfile(argv[optind], flags, growable);
diff --git a/qemu-nbd.c b/qemu-nbd.c
index 6707ea5..4463679 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -213,7 +213,7 @@ int main(int argc, char **argv)
     int opt_ind = 0;
     int li;
     char *end;
-    int flags = 0;
+    int flags = BDRV_O_RDWR;
     int partition = -1;
     int ret;
     int shared = 1;
diff --git a/vl.c b/vl.c
index 76ef8ca..eee59dd 100644
--- a/vl.c
+++ b/vl.c
@@ -2227,19 +2227,19 @@ DriveInfo *drive_init(QemuOpts *opts, void *opaque,
     }
 
     if (ro == 1) {
-        if (type == IF_IDE) {
-            fprintf(stderr, "qemu: readonly flag not supported for drive with ide interface\n");
+        if (type != IF_SCSI && type != IF_VIRTIO && type != IF_FLOPPY) {
+            fprintf(stderr, "qemu: readonly flag not supported for drive with this interface\n");
             return NULL;
         }
-        (void)bdrv_set_read_only(dinfo->bdrv, 1);
     }
     /* 
      * cdrom is read-only. Set it now, after above interface checking
      * since readonly attribute not explicitly required, so no error.
      */
     if (media == MEDIA_CDROM) {
-        (void)bdrv_set_read_only(dinfo->bdrv, 1);
+        ro = 1;
     }
+    bdrv_flags |= ro ? 0 : BDRV_O_RDWR;
 
     if (bdrv_open2(dinfo->bdrv, file, bdrv_flags, drv) < 0) {
         fprintf(stderr, "qemu: could not open disk image %s: %s\n",
-- 
1.6.3.3

[Qemu-devel] [PATCH v2 3/4] Added drives' readonly option

[Naphtali Sprei]

Signed-off-by: Naphtali Sprei <nsprei@redhat.com>
---
 qemu-options.hx |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/qemu-options.hx b/qemu-options.hx
index e2edd71..4617867 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -103,7 +103,7 @@ DEF("drive", HAS_ARG, QEMU_OPTION_drive,
     "-drive [file=file][,if=type][,bus=n][,unit=m][,media=d][,index=i]\n"
     "       [,cyls=c,heads=h,secs=s[,trans=t]][,snapshot=on|off]\n"
     "       [,cache=writethrough|writeback|none][,format=f][,serial=s]\n"
-    "       [,addr=A][,id=name][,aio=threads|native]\n"
+    "       [,addr=A][,id=name][,aio=threads|native][,readonly=on|off]\n"
     "                use 'file' as a drive image\n")
 DEF("set", HAS_ARG, QEMU_OPTION_set,
     "-set group.id.arg=value\n"
-- 
1.6.3.3

[Qemu-devel] [PATCH v2 4/4] Disable fall-back to read-only when cannot open drive's file for read-write

[Naphtali Sprei]

Signed-off-by: Naphtali Sprei <nsprei@redhat.com>
---
 block.c |    6 ------
 1 files changed, 0 insertions(+), 6 deletions(-)

diff --git a/block.c b/block.c
index 8def3c4..f90e983 100644
--- a/block.c
+++ b/block.c
@@ -444,8 +444,6 @@ int bdrv_open2(BlockDriverState *bs, const char *filename, int flags,
     if (flags & (BDRV_O_CACHE_WB|BDRV_O_NOCACHE))
         bs->enable_write_cache = 1;
 
-    /* Note: for compatibility, we open disk image files as RDWR, and
-       RDONLY as fallback */
     bs->read_only = (flags & BDRV_O_RDWR) == 0;
     if (!(flags & BDRV_O_FILE)) {
         open_flags = (flags & (BDRV_O_RDWR | BDRV_O_CACHE_MASK|BDRV_O_NATIVE_AIO));
@@ -459,10 +457,6 @@ int bdrv_open2(BlockDriverState *bs, const char *filename, int flags,
         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);
-            bs->read_only = 1;
-        }
     }
     if (ret < 0) {
         qemu_free(bs->opaque);
-- 
1.6.3.3

[Qemu-devel] Re: [PATCH v2 4/4] Disable fall-back to read-only when cannot open drive's file for read-write

[Michael S. Tsirkin]

On Sun, Jan 17, 2010 at 04:48:15PM +0200, Naphtali Sprei wrote:
> 
> Signed-off-by: Naphtali Sprei <nsprei@redhat.com>
> ---
>  block.c |    6 ------
>  1 files changed, 0 insertions(+), 6 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 8def3c4..f90e983 100644
> --- a/block.c
> +++ b/block.c
> @@ -444,8 +444,6 @@ int bdrv_open2(BlockDriverState *bs, const char *filename, int flags,
>      if (flags & (BDRV_O_CACHE_WB|BDRV_O_NOCACHE))
>          bs->enable_write_cache = 1;
>  
> -    /* Note: for compatibility, we open disk image files as RDWR, and
> -       RDONLY as fallback */
>      bs->read_only = (flags & BDRV_O_RDWR) == 0;
>      if (!(flags & BDRV_O_FILE)) {
>          open_flags = (flags & (BDRV_O_RDWR | BDRV_O_CACHE_MASK|BDRV_O_NATIVE_AIO));
> @@ -459,10 +457,6 @@ int bdrv_open2(BlockDriverState *bs, const char *filename, int flags,
>          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);
> -            bs->read_only = 1;
> -        }

Maybe print an error message explaining the problem and
suggesting the solution?

>      }
>      if (ret < 0) {
>          qemu_free(bs->opaque);
> -- 
> 1.6.3.3
> 
> 

[Qemu-devel] Re: [PATCH v2 4/4] Disable fall-back to read-only when cannot open drive's file for read-write

[Naphtali Sprei]

Michael S. Tsirkin wrote:
> On Sun, Jan 17, 2010 at 04:48:15PM +0200, Naphtali Sprei wrote:
>> Signed-off-by: Naphtali Sprei <nsprei@redhat.com>
>> ---
>>  block.c |    6 ------
>>  1 files changed, 0 insertions(+), 6 deletions(-)
>>
>> diff --git a/block.c b/block.c
>> index 8def3c4..f90e983 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -444,8 +444,6 @@ int bdrv_open2(BlockDriverState *bs, const char *filename, int flags,
>>      if (flags & (BDRV_O_CACHE_WB|BDRV_O_NOCACHE))
>>          bs->enable_write_cache = 1;
>>  
>> -    /* Note: for compatibility, we open disk image files as RDWR, and
>> -       RDONLY as fallback */
>>      bs->read_only = (flags & BDRV_O_RDWR) == 0;
>>      if (!(flags & BDRV_O_FILE)) {
>>          open_flags = (flags & (BDRV_O_RDWR | BDRV_O_CACHE_MASK|BDRV_O_NATIVE_AIO));
>> @@ -459,10 +457,6 @@ int bdrv_open2(BlockDriverState *bs, const char *filename, int flags,
>>          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);
>> -            bs->read_only = 1;
>> -        }
> 
> Maybe print an error message explaining the problem and
> suggesting the solution?

Printing done in (some of the) callers.
Should add suggestion. How is it done in QMP ?

> 
>>      }
>>      if (ret < 0) {
>>          qemu_free(bs->opaque);
>> -- 
>> 1.6.3.3
>>
>>

[Qemu-devel] Re: [PATCH v2 2/4] Clean-up a little bit the RW related bits of BDRV_O_FLAGS. BDRV_O_RDONLY gone (and so is BDRV_O_ACCESS). Default value for bdrv_flags (0/zero) is READ-ONLY. Need to explicitly request READ-WRITE.

[Michael S. Tsirkin]

On Sun, Jan 17, 2010 at 04:48:13PM +0200, Naphtali Sprei wrote:
> 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 <nsprei@redhat.com>

Many changes seem to be about passing 0 to bdrv_open. This is not what
the changelog says the patch does. Better split unrelated changes to a
separate patch.

One of the things you seem to do is get rid of BDRV_O_RDONLY.  Why is
this an improvement? Symbolic name like BDRV_O_RDONLY seems better than
0.

> ---
>  block.c           |   31 +++++++++++++++++--------------
>  block.h           |    2 --
>  block/raw-posix.c |    2 +-
>  block/raw-win32.c |    4 ++--
>  block/vmdk.c      |    9 +++++----
>  hw/xen_disk.c     |    2 +-
>  monitor.c         |    2 +-
>  qemu-img.c        |   10 ++++++----
>  qemu-io.c         |   14 ++++++--------
>  qemu-nbd.c        |    2 +-
>  vl.c              |    8 ++++----
>  11 files changed, 44 insertions(+), 42 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 115e591..8def3c4 100644
> --- a/block.c
> +++ b/block.c
> @@ -310,7 +310,7 @@ static BlockDriver *find_image_format(const char *filename)
>      if (drv && strcmp(drv->format_name, "vvfat") == 0)
>          return drv;
>  
> -    ret = bdrv_file_open(&bs, filename, BDRV_O_RDONLY);
> +    ret = bdrv_file_open(&bs, filename, 0);
>      if (ret < 0)
>          return NULL;
>      ret = bdrv_pread(bs, 0, buf, sizeof(buf));
> @@ -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];
>  
> @@ -446,19 +446,23 @@ int bdrv_open2(BlockDriverState *bs, const char *filename, int flags,
>  
>      /* 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
> +    bs->read_only = (flags & BDRV_O_RDWR) == 0;

!(flags & BDRV_O_RDWR) is a standard idiom, and it's more readable IMO.

> +    if (!(flags & BDRV_O_FILE)) {
> +        open_flags = (flags & (BDRV_O_RDWR | BDRV_O_CACHE_MASK|BDRV_O_NATIVE_AIO));
> +        if (bs->is_temporary) { /* snapshot should be writeable */
> +            open_flags |= BDRV_O_RDWR;
> +        }
> +    } else {
>          open_flags = flags & ~(BDRV_O_FILE | BDRV_O_SNAPSHOT);
> -    if (use_bdrv_whitelist && !bdrv_is_whitelisted(drv))
> +    }
> +    if (use_bdrv_whitelist && !bdrv_is_whitelisted(drv)) {
>          ret = -ENOTSUP;
> -    else
> +    } 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);
> -        bs->read_only = 1;
> +        if ((ret == -EACCES || ret == -EPERM) && !(flags & BDRV_O_FILE)) {
> +            ret = drv->bdrv_open(bs, filename, open_flags & ~BDRV_O_RDWR);
> +            bs->read_only = 1;
> +        }
>      }
>      if (ret < 0) {
>          qemu_free(bs->opaque);
> @@ -481,14 +485,13 @@ 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);
>          ret = bdrv_open2(bs->backing_hd, backing_filename, open_flags,
>                           back_drv);
> +        bs->backing_hd->read_only =  (open_flags & BDRV_O_RDWR) == 0;

!(open_flags & BDRV_O_RDWR) is a standard idiom and it's more readable
IMO.
Further, pls don't put two spaces after ==.

>          if (ret < 0) {
>              bdrv_close(bs);
>              return ret;
> diff --git a/block.h b/block.h
> index bee9ec5..fd4e8dd 100644
> --- a/block.h
> +++ b/block.h
> @@ -27,9 +27,7 @@ typedef struct QEMUSnapshotInfo {
>      uint64_t vm_clock_nsec; /* VM clock relative to boot */
>  } QEMUSnapshotInfo;
>  
> -#define BDRV_O_RDONLY      0x0000
>  #define BDRV_O_RDWR        0x0002
> -#define BDRV_O_ACCESS      0x0003
>  #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
> diff --git a/block/raw-posix.c b/block/raw-posix.c
> index 5a6a22b..b427211 100644
> --- a/block/raw-posix.c
> +++ b/block/raw-posix.c
> @@ -138,7 +138,7 @@ static int raw_open_common(BlockDriverState *bs, const char *filename,
>  
>      s->open_flags = open_flags | O_BINARY;
>      s->open_flags &= ~O_ACCMODE;
> -    if ((bdrv_flags & BDRV_O_ACCESS) == BDRV_O_RDWR) {
> +    if (bdrv_flags & BDRV_O_RDWR) {
>          s->open_flags |= O_RDWR;
>      } else {
>          s->open_flags |= O_RDONLY;
> diff --git a/block/raw-win32.c b/block/raw-win32.c
> index 72acad5..01a6d25 100644
> --- a/block/raw-win32.c
> +++ b/block/raw-win32.c
> @@ -81,7 +81,7 @@ static int raw_open(BlockDriverState *bs, const char *filename, int flags)
>  
>      s->type = FTYPE_FILE;
>  
> -    if ((flags & BDRV_O_ACCESS) == O_RDWR) {
> +    if (flags & BDRV_O_RDWR) {
>          access_flags = GENERIC_READ | GENERIC_WRITE;
>      } else {
>          access_flags = GENERIC_READ;
> @@ -337,7 +337,7 @@ static int hdev_open(BlockDriverState *bs, const char *filename, int flags)
>      }
>      s->type = find_device_type(bs, filename);
>  
> -    if ((flags & BDRV_O_ACCESS) == O_RDWR) {
> +    if (flags & BDRV_O_RDWR) {
>          access_flags = GENERIC_READ | GENERIC_WRITE;
>      } else {
>          access_flags = GENERIC_READ;

The above changes seem nothing to do with passing in parameter to the
function. If the are intentional, pls mention them in changelog or split
to a separate patch.

> diff --git a/block/vmdk.c b/block/vmdk.c
> index 4e48622..ddc2fcb 100644
> --- a/block/vmdk.c
> +++ b/block/vmdk.c
> @@ -357,7 +357,7 @@ static int vmdk_parent_open(BlockDriverState *bs, const char * filename)
>              return -1;
>          }
>          parent_open = 1;
> -        if (bdrv_open(bs->backing_hd, parent_img_name, BDRV_O_RDONLY) < 0)
> +        if (bdrv_open(bs->backing_hd, parent_img_name, 0) < 0)
>              goto failure;
>          parent_open = 0;
>      }
> @@ -371,9 +371,10 @@ static int vmdk_open(BlockDriverState *bs, const char *filename, int flags)
>      uint32_t magic;
>      int l1_size, i, ret;
>  
> -    if (parent_open)
> -        // Parent must be opened as RO.
> -        flags = BDRV_O_RDONLY;
> +    if (parent_open) {
> +        /* Parent must be opened as RO, no RDWR. */
> +        flags = 0;
> +    }
>  
>      ret = bdrv_file_open(&s->hd, filename, flags);
>      if (ret < 0)
> diff --git a/hw/xen_disk.c b/hw/xen_disk.c
> index 5c55251..beadf90 100644
> --- a/hw/xen_disk.c
> +++ b/hw/xen_disk.c
> @@ -613,7 +613,7 @@ static int blk_init(struct XenDevice *xendev)
>  	qflags = BDRV_O_RDWR;
>      } else {
>  	mode   = O_RDONLY;
> -	qflags = BDRV_O_RDONLY;
> +	qflags = 0;
>  	info  |= VDISK_READONLY;
>      }
>  
> diff --git a/monitor.c b/monitor.c
> index b824e7c..5bb8872 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -916,7 +916,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_RDWR, drv);

This and below seems to change file from rdwr to readonly.
Intentional?

>      monitor_read_bdrv_key_start(mon, bs, NULL, NULL);
>  }
>  
> diff --git a/qemu-img.c b/qemu-img.c
> index 48b9315..3cea8ce 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -204,7 +204,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_RDWR, drv) < 0) {
>          error("Could not open '%s'", filename);
>      }
>      if (bdrv_is_encrypted(bs)) {
> @@ -468,7 +468,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);
> @@ -966,10 +966,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;
>      /* Parse commandline parameters */
>      for(;;) {
>          c = getopt(argc, argv, "la:c:d:h");
> @@ -985,6 +986,7 @@ static int img_snapshot(int argc, char **argv)
>                  return 0;
>              }
>              action = SNAPSHOT_LIST;
> +            bdrv_oflags &= ~BDRV_O_RDWR; /* no need for RW */

bdrv_oflags = BDRV_O_RDONLY would be clearer, and no need
for comment then?

>              break;
>          case 'a':
>              if (action) {
> @@ -1022,7 +1024,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) {
>          error("Could not open '%s'", filename);
>      }
>  
> diff --git a/qemu-io.c b/qemu-io.c
> index 1c19b92..b159bc9 100644
> --- a/qemu-io.c
> +++ b/qemu-io.c
> @@ -1359,10 +1359,9 @@ open_f(int argc, char **argv)
>  		}
>  	}
>  
> -	if (readonly)
> -		flags |= BDRV_O_RDONLY;
> -	else
> -		flags |= BDRV_O_RDWR;
> +	if (!readonly) {
> +            flags |= BDRV_O_RDWR;
> +        }
>  
>  	if (optind != argc - 1)
>  		return command_usage(&open_cmd);
> @@ -1506,10 +1505,9 @@ int main(int argc, char **argv)
>  	add_check_command(init_check_command);
>  
>  	/* open the device */
> -	if (readonly)
> -		flags |= BDRV_O_RDONLY;
> -	else
> -		flags |= BDRV_O_RDWR;
> +	if (!readonly) {
> +            flags |= BDRV_O_RDWR;
> +        }

alignment seems broken in 2 places above

>  
>  	if ((argc - optind) == 1)
>  		openfile(argv[optind], flags, growable);
> diff --git a/qemu-nbd.c b/qemu-nbd.c
> index 6707ea5..4463679 100644
> --- a/qemu-nbd.c
> +++ b/qemu-nbd.c
> @@ -213,7 +213,7 @@ int main(int argc, char **argv)
>      int opt_ind = 0;
>      int li;
>      char *end;
> -    int flags = 0;
> +    int flags = BDRV_O_RDWR;
>      int partition = -1;
>      int ret;
>      int shared = 1;
> diff --git a/vl.c b/vl.c
> index 76ef8ca..eee59dd 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -2227,19 +2227,19 @@ DriveInfo *drive_init(QemuOpts *opts, void *opaque,
>      }
>  
>      if (ro == 1) {
> -        if (type == IF_IDE) {
> -            fprintf(stderr, "qemu: readonly flag not supported for drive with ide interface\n");
> +        if (type != IF_SCSI && type != IF_VIRTIO && type != IF_FLOPPY) {

So each new user will have to be added here? Any idea how todo this
better? Can't relevant drive emulation check read_only flag and report
an error? Or set a flag in drive info declaring readonly support.

> +            fprintf(stderr, "qemu: readonly flag not supported for drive with this interface\n");
>              return NULL;
>          }
> -        (void)bdrv_set_read_only(dinfo->bdrv, 1);
>      }
>      /* 
>       * cdrom is read-only. Set it now, after above interface checking
>       * since readonly attribute not explicitly required, so no error.
>       */
>      if (media == MEDIA_CDROM) {
> -        (void)bdrv_set_read_only(dinfo->bdrv, 1);
> +        ro = 1;
>      }
> +    bdrv_flags |= ro ? 0 : BDRV_O_RDWR;
>  
>      if (bdrv_open2(dinfo->bdrv, file, bdrv_flags, drv) < 0) {
>          fprintf(stderr, "qemu: could not open disk image %s: %s\n",
> -- 
> 1.6.3.3
> 
> 

Re: [Qemu-devel] Re: [PATCH v2 2/4] Clean-up a little bit the RW related bits of BDRV_O_FLAGS. BDRV_O_RDONLY gone (and so is BDRV_O_ACCESS). Default value for bdrv_flags (0/zero) is READ-ONLY. Need to explicitly request READ-WRITE.

[Markus Armbruster]

"Michael S. Tsirkin" <mst@redhat.com> writes:

> On Sun, Jan 17, 2010 at 04:48:13PM +0200, Naphtali Sprei wrote:
>> 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 <nsprei@redhat.com>
>
> Many changes seem to be about passing 0 to bdrv_open. This is not what
> the changelog says the patch does. Better split unrelated changes to a
> separate patch.
>
> One of the things you seem to do is get rid of BDRV_O_RDONLY.  Why is
> this an improvement? Symbolic name like BDRV_O_RDONLY seems better than
> 0.

BDRV_O_RDWR is a flag, just like BDRV_SNAPSHOT.  We don't have
BDRV_DONT_SNAPSHOT, either.

The old code can't quite devide whether BDRV_O_RDWR is a flag, or
whether to use bits BDRV_O_ACCESS for an access mode, with possible
values BDRV_O_RDONLY and BDRV_O_RDWR.  I asked Naphtali to clean this
up, and recommended to go with flag rather than access mode:

    In my opinion, any benefit in readability you might hope gain by
    having BDRV_O_RDONLY is outweighed by the tortuous bit twiddling you
    need to keep knowledge of its encoding out of its users.

http://lists.nongnu.org/archive/html/qemu-devel/2009-12/msg02504.html

[...]
>> @@ -985,6 +986,7 @@ static int img_snapshot(int argc, char **argv)
>>                  return 0;
>>              }
>>              action = SNAPSHOT_LIST;
>> +            bdrv_oflags &= ~BDRV_O_RDWR; /* no need for RW */
>
> bdrv_oflags = BDRV_O_RDONLY would be clearer, and no need
> for comment then?

BDRV_O_RDWR is a flag, and this is the clean way to clear it.

"bdrv_oflags = BDRV_O_RDONLY" assumes that everything but the access
mode in bdrv_oflags is clear.  Tolerable, because the correctness
argument is fairly local, but the clean way to do it would be

    bdrv_oflags = (bdrv_oflags & ~ BDRV_O_ACCESS) | BDRV_O_RDONLY;

That's what I meant by "tortuous bit twiddling".

[...]

Re: [Qemu-devel] Re: [PATCH v2 2/4] Clean-up a little bit the RW related bits of BDRV_O_FLAGS. BDRV_O_RDONLY gone (and so is BDRV_O_ACCESS). Default value for bdrv_flags (0/zero) is READ-ONLY. Need to explicitly request READ-WRITE.

[Michael S. Tsirkin]

On Mon, Jan 18, 2010 at 11:34:59AM +0100, Markus Armbruster wrote:
> "Michael S. Tsirkin" <mst@redhat.com> writes:
> 
> > On Sun, Jan 17, 2010 at 04:48:13PM +0200, Naphtali Sprei wrote:
> >> 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 <nsprei@redhat.com>
> >
> > Many changes seem to be about passing 0 to bdrv_open. This is not what
> > the changelog says the patch does. Better split unrelated changes to a
> > separate patch.
> >
> > One of the things you seem to do is get rid of BDRV_O_RDONLY.  Why is
> > this an improvement? Symbolic name like BDRV_O_RDONLY seems better than
> > 0.
> 
> BDRV_O_RDWR is a flag, just like BDRV_SNAPSHOT.  We don't have
> BDRV_DONT_SNAPSHOT, either.

Well, this just mirros the file access macros: we have RDONLY, WRONLY
and RDRW. I assume this similarity is just historical?

> The old code can't quite devide whether BDRV_O_RDWR is a flag, or
> whether to use bits BDRV_O_ACCESS for an access mode, with possible
> values BDRV_O_RDONLY and BDRV_O_RDWR.  I asked Naphtali to clean this
> up, and recommended to go with flag rather than access mode:
> 
>     In my opinion, any benefit in readability you might hope gain by
>     having BDRV_O_RDONLY is outweighed by the tortuous bit twiddling you
>     need to keep knowledge of its encoding out of its users.
> 
> http://lists.nongnu.org/archive/html/qemu-devel/2009-12/msg02504.html
> 
> [...]
> >> @@ -985,6 +986,7 @@ static int img_snapshot(int argc, char **argv)
> >>                  return 0;
> >>              }
> >>              action = SNAPSHOT_LIST;
> >> +            bdrv_oflags &= ~BDRV_O_RDWR; /* no need for RW */
> >
> > bdrv_oflags = BDRV_O_RDONLY would be clearer, and no need
> > for comment then?
> 
> BDRV_O_RDWR is a flag, and this is the clean way to clear it.
> 
> "bdrv_oflags = BDRV_O_RDONLY" assumes that everything but the access
> mode in bdrv_oflags is clear.  Tolerable, because the correctness
> argument is fairly local, but the clean way to do it would be
> 
>     bdrv_oflags = (bdrv_oflags & ~ BDRV_O_ACCESS) | BDRV_O_RDONLY;
> 
> That's what I meant by "tortuous bit twiddling".
> 
> [...]

Thinking about it, /* no need for RW */ comment can just go.  But other
places in code just do flags = 0 maybe they should all do &=
~BDRV_O_RDWR?  I don't really have an opinion here but I do think this
patch needs a better commit log (all it says "pass the request in the
flags parameter to the function") and be split up:
patch 1 - get rid of BDRV_O_RDONLY/BDRV_O_ACCESS
patch 2 - pass the request in the flags parameter to the function
patch 3 - any other fixups

As it is, sometimes e.g. BDRV_O_RDWR is replaced with 0 sometimes as
well, and it's hard to see why.

-- 
MST

Re: [Qemu-devel] Re: [PATCH v2 2/4] Clean-up a little bit the RW related bits of BDRV_O_FLAGS. BDRV_O_RDONLY gone (and so is BDRV_O_ACCESS). Default value for bdrv_flags (0/zero) is READ-ONLY. Need to explicitly request READ-WRITE.

[Markus Armbruster]

"Michael S. Tsirkin" <mst@redhat.com> writes:

> On Mon, Jan 18, 2010 at 11:34:59AM +0100, Markus Armbruster wrote:
>> "Michael S. Tsirkin" <mst@redhat.com> writes:
>> 
>> > On Sun, Jan 17, 2010 at 04:48:13PM +0200, Naphtali Sprei wrote:
>> >> 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 <nsprei@redhat.com>
>> >
>> > Many changes seem to be about passing 0 to bdrv_open. This is not what
>> > the changelog says the patch does. Better split unrelated changes to a
>> > separate patch.
>> >
>> > One of the things you seem to do is get rid of BDRV_O_RDONLY.  Why is
>> > this an improvement? Symbolic name like BDRV_O_RDONLY seems better than
>> > 0.
>> 
>> BDRV_O_RDWR is a flag, just like BDRV_SNAPSHOT.  We don't have
>> BDRV_DONT_SNAPSHOT, either.
>
> Well, this just mirros the file access macros: we have RDONLY, WRONLY
> and RDRW. I assume this similarity is just historical?

Plausible.

>> The old code can't quite devide whether BDRV_O_RDWR is a flag, or
>> whether to use bits BDRV_O_ACCESS for an access mode, with possible
>> values BDRV_O_RDONLY and BDRV_O_RDWR.  I asked Naphtali to clean this
>> up, and recommended to go with flag rather than access mode:
>> 
>>     In my opinion, any benefit in readability you might hope gain by
>>     having BDRV_O_RDONLY is outweighed by the tortuous bit twiddling you
>>     need to keep knowledge of its encoding out of its users.
>> 
>> http://lists.nongnu.org/archive/html/qemu-devel/2009-12/msg02504.html
>> 
>> [...]
>> >> @@ -985,6 +986,7 @@ static int img_snapshot(int argc, char **argv)
>> >>                  return 0;
>> >>              }
>> >>              action = SNAPSHOT_LIST;
>> >> +            bdrv_oflags &= ~BDRV_O_RDWR; /* no need for RW */
>> >
>> > bdrv_oflags = BDRV_O_RDONLY would be clearer, and no need
>> > for comment then?
>> 
>> BDRV_O_RDWR is a flag, and this is the clean way to clear it.
>> 
>> "bdrv_oflags = BDRV_O_RDONLY" assumes that everything but the access
>> mode in bdrv_oflags is clear.  Tolerable, because the correctness
>> argument is fairly local, but the clean way to do it would be
>> 
>>     bdrv_oflags = (bdrv_oflags & ~ BDRV_O_ACCESS) | BDRV_O_RDONLY;
>> 
>> That's what I meant by "tortuous bit twiddling".
>> 
>> [...]
>
> Thinking about it, /* no need for RW */ comment can just go.

Agree.

>                                                               But other
> places in code just do flags = 0 maybe they should all do &=
> ~BDRV_O_RDWR?  I don't really have an opinion here but I do think this
> patch needs a better commit log (all it says "pass the request in the
> flags parameter to the function") and be split up:
> patch 1 - get rid of BDRV_O_RDONLY/BDRV_O_ACCESS
> patch 2 - pass the request in the flags parameter to the function
> patch 3 - any other fixups
>
> As it is, sometimes e.g. BDRV_O_RDWR is replaced with 0 sometimes as
> well, and it's hard to see why.

Fair enough.

Re: [Qemu-devel] Re: [PATCH v2 2/4] Clean-up a little bit the RW related bits of BDRV_O_FLAGS. BDRV_O_RDONLY gone (and so is BDRV_O_ACCESS). Default value for bdrv_flags (0/zero) is READ-ONLY. Need to explicitly request READ-WRITE.

[Jamie Lokier]

Michael S. Tsirkin wrote:
> On Mon, Jan 18, 2010 at 11:34:59AM +0100, Markus Armbruster wrote:
> > BDRV_O_RDWR is a flag, just like BDRV_SNAPSHOT.  We don't have
> > BDRV_DONT_SNAPSHOT, either.
> 
> Well, this just mirros the file access macros: we have RDONLY, WRONLY
> and RDRW. I assume this similarity is just historical?

To avoid confusion, why don't we just call the flag BDRV_O_WRITABLE.
Then it's obvious what clearing that flag means.

-- Jamie

Re: [Qemu-devel] Re: [PATCH v2 2/4] Clean-up a little bit the RW related bits of BDRV_O_FLAGS. BDRV_O_RDONLY gone (and so is BDRV_O_ACCESS). Default value for bdrv_flags (0/zero) is READ-ONLY. Need to explicitly request READ-WRITE.

[Markus Armbruster]

Jamie Lokier <jamie@shareable.org> writes:

> Michael S. Tsirkin wrote:
>> On Mon, Jan 18, 2010 at 11:34:59AM +0100, Markus Armbruster wrote:
>> > BDRV_O_RDWR is a flag, just like BDRV_SNAPSHOT.  We don't have
>> > BDRV_DONT_SNAPSHOT, either.
>> 
>> Well, this just mirros the file access macros: we have RDONLY, WRONLY
>> and RDRW. I assume this similarity is just historical?
>
> To avoid confusion, why don't we just call the flag BDRV_O_WRITABLE.
> Then it's obvious what clearing that flag means.

Sounds good to me.

Re: [Qemu-devel] Re: [PATCH v2 2/4] Clean-up a little bit the RW related bits of BDRV_O_FLAGS. BDRV_O_RDONLY gone (and so is BDRV_O_ACCESS). Default value for bdrv_flags (0/zero) is READ-ONLY. Need to explicitly request READ-WRITE.

[Michael S. Tsirkin]

On Wed, Jan 20, 2010 at 08:26:56AM +0100, Markus Armbruster wrote:
> Jamie Lokier <jamie@shareable.org> writes:
> 
> > Michael S. Tsirkin wrote:
> >> On Mon, Jan 18, 2010 at 11:34:59AM +0100, Markus Armbruster wrote:
> >> > BDRV_O_RDWR is a flag, just like BDRV_SNAPSHOT.  We don't have
> >> > BDRV_DONT_SNAPSHOT, either.
> >> 
> >> Well, this just mirros the file access macros: we have RDONLY, WRONLY
> >> and RDRW. I assume this similarity is just historical?
> >
> > To avoid confusion, why don't we just call the flag BDRV_O_WRITABLE.
> > Then it's obvious what clearing that flag means.
> 
> Sounds good to me.

Won't it be confused with WRONLY?

-- 
MST

Re: [Qemu-devel] Re: [PATCH v2 2/4] Clean-up a little bit the RW related bits of BDRV_O_FLAGS. BDRV_O_RDONLY gone (and so is BDRV_O_ACCESS). Default value for bdrv_flags (0/zero) is READ-ONLY. Need to explicitly request READ-WRITE.

[Markus Armbruster]

"Michael S. Tsirkin" <mst@redhat.com> writes:

> On Wed, Jan 20, 2010 at 08:26:56AM +0100, Markus Armbruster wrote:
>> Jamie Lokier <jamie@shareable.org> writes:
>> 
>> > Michael S. Tsirkin wrote:
>> >> On Mon, Jan 18, 2010 at 11:34:59AM +0100, Markus Armbruster wrote:
>> >> > BDRV_O_RDWR is a flag, just like BDRV_SNAPSHOT.  We don't have
>> >> > BDRV_DONT_SNAPSHOT, either.
>> >> 
>> >> Well, this just mirros the file access macros: we have RDONLY, WRONLY
>> >> and RDRW. I assume this similarity is just historical?
>> >
>> > To avoid confusion, why don't we just call the flag BDRV_O_WRITABLE.
>> > Then it's obvious what clearing that flag means.
>> 
>> Sounds good to me.
>
> Won't it be confused with WRONLY?

I doubt it.  "Writable" implies "write-only" no more than "readable"
implies "read-only".

Re: [Qemu-devel] Re: [PATCH v2 2/4] Clean-up a little bit the RW related bits of BDRV_O_FLAGS. BDRV_O_RDONLY gone (and so is BDRV_O_ACCESS). Default value for bdrv_flags (0/zero) is READ-ONLY. Need to explicitly request READ-WRITE.

[Michael S. Tsirkin]

On Wed, Jan 20, 2010 at 01:09:29PM +0100, Markus Armbruster wrote:
> "Michael S. Tsirkin" <mst@redhat.com> writes:
> 
> > On Wed, Jan 20, 2010 at 08:26:56AM +0100, Markus Armbruster wrote:
> >> Jamie Lokier <jamie@shareable.org> writes:
> >> 
> >> > Michael S. Tsirkin wrote:
> >> >> On Mon, Jan 18, 2010 at 11:34:59AM +0100, Markus Armbruster wrote:
> >> >> > BDRV_O_RDWR is a flag, just like BDRV_SNAPSHOT.  We don't have
> >> >> > BDRV_DONT_SNAPSHOT, either.
> >> >> 
> >> >> Well, this just mirros the file access macros: we have RDONLY, WRONLY
> >> >> and RDRW. I assume this similarity is just historical?
> >> >
> >> > To avoid confusion, why don't we just call the flag BDRV_O_WRITABLE.
> >> > Then it's obvious what clearing that flag means.
> >> 
> >> Sounds good to me.
> >
> > Won't it be confused with WRONLY?
> 
> I doubt it.  "Writable" implies "write-only" no more than "readable"
> implies "read-only".

Yes :) But what I am saying is that the disk is readable as well.

-- 
MST

Re: [Qemu-devel] Re: [PATCH v2 2/4] Clean-up a little bit the RW related bits of BDRV_O_FLAGS. BDRV_O_RDONLY gone (and so is BDRV_O_ACCESS). Default value for bdrv_flags (0/zero) is READ-ONLY. Need to explicitly request READ-WRITE.

[Markus Armbruster]

"Michael S. Tsirkin" <mst@redhat.com> writes:

> On Wed, Jan 20, 2010 at 01:09:29PM +0100, Markus Armbruster wrote:
>> "Michael S. Tsirkin" <mst@redhat.com> writes:
>> 
>> > On Wed, Jan 20, 2010 at 08:26:56AM +0100, Markus Armbruster wrote:
>> >> Jamie Lokier <jamie@shareable.org> writes:
>> >> 
>> >> > Michael S. Tsirkin wrote:
>> >> >> On Mon, Jan 18, 2010 at 11:34:59AM +0100, Markus Armbruster wrote:
>> >> >> > BDRV_O_RDWR is a flag, just like BDRV_SNAPSHOT.  We don't have
>> >> >> > BDRV_DONT_SNAPSHOT, either.
>> >> >> 
>> >> >> Well, this just mirros the file access macros: we have RDONLY, WRONLY
>> >> >> and RDRW. I assume this similarity is just historical?
>> >> >
>> >> > To avoid confusion, why don't we just call the flag BDRV_O_WRITABLE.
>> >> > Then it's obvious what clearing that flag means.
>> >> 
>> >> Sounds good to me.
>> >
>> > Won't it be confused with WRONLY?
>> 
>> I doubt it.  "Writable" implies "write-only" no more than "readable"
>> implies "read-only".
>
> Yes :) But what I am saying is that the disk is readable as well.

"Writable" doesn't imply "not readable" either :)

Re: [Qemu-devel] Re: [PATCH v2 2/4] Clean-up a little bit the RW related bits of BDRV_O_FLAGS. BDRV_O_RDONLY gone (and so is BDRV_O_ACCESS). Default value for bdrv_flags (0/zero) is READ-ONLY. Need to explicitly request READ-WRITE.

[Jamie Lokier]

Michael S. Tsirkin wrote:
> On Wed, Jan 20, 2010 at 08:26:56AM +0100, Markus Armbruster wrote:
> > Jamie Lokier <jamie@shareable.org> writes:
> > 
> > > Michael S. Tsirkin wrote:
> > >> On Mon, Jan 18, 2010 at 11:34:59AM +0100, Markus Armbruster wrote:
> > >> > BDRV_O_RDWR is a flag, just like BDRV_SNAPSHOT.  We don't have
> > >> > BDRV_DONT_SNAPSHOT, either.
> > >> 
> > >> Well, this just mirros the file access macros: we have RDONLY, WRONLY
> > >> and RDRW. I assume this similarity is just historical?
> > >
> > > To avoid confusion, why don't we just call the flag BDRV_O_WRITABLE.
> > > Then it's obvious what clearing that flag means.
> > 
> > Sounds good to me.
> 
> Won't it be confused with WRONLY?

No, because nobody sane would expect qemu's blockdevs to need WRONLY :-)

-- Jamie

[Qemu-devel] Re: [PATCH v2 2/4] Clean-up a little bit the RW related bits of BDRV_O_FLAGS. BDRV_O_RDONLY gone (and so is BDRV_O_ACCESS). Default value for bdrv_flags (0/zero) is READ-ONLY. Need to explicitly request READ-WRITE.

[Naphtali Sprei]

Michael S. Tsirkin wrote:
> On Sun, Jan 17, 2010 at 04:48:13PM +0200, Naphtali Sprei wrote:
>> 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 <nsprei@redhat.com>
> 
> Many changes seem to be about passing 0 to bdrv_open. This is not what
> the changelog says the patch does. Better split unrelated changes to a
> separate patch.

Thanks for the review.

Unfortunately, some of my comments went to the subject and are not part of the changelog.
So here's the (intended) changelog. This will clear-up some of your comments.

Clean-up a little bit the RW related bits of BDRV_O_FLAGS.
BDRV_O_RDONLY gone (and so is BDRV_O_ACCESS).
Default	value for bdrv_flags (0/zero) is READ-ONLY. Need to explicitly request READ-WRITE.
Instead of using the field 'readonly' of the BlockDriverState struct for passing the request,
pass the request in the flags parameter to the function.



> 
> One of the things you seem to do is get rid of BDRV_O_RDONLY.  Why is
> this an improvement? Symbolic name like BDRV_O_RDONLY seems better than
> 0.

See Markus reply (thanks Markus).

> 
>> ---
>>  block.c           |   31 +++++++++++++++++--------------
>>  block.h           |    2 --
>>  block/raw-posix.c |    2 +-
>>  block/raw-win32.c |    4 ++--
>>  block/vmdk.c      |    9 +++++----
>>  hw/xen_disk.c     |    2 +-
>>  monitor.c         |    2 +-
>>  qemu-img.c        |   10 ++++++----
>>  qemu-io.c         |   14 ++++++--------
>>  qemu-nbd.c        |    2 +-
>>  vl.c              |    8 ++++----
>>  11 files changed, 44 insertions(+), 42 deletions(-)
>>
>> diff --git a/block.c b/block.c
>> index 115e591..8def3c4 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -310,7 +310,7 @@ static BlockDriver *find_image_format(const char *filename)
>>      if (drv && strcmp(drv->format_name, "vvfat") == 0)
>>          return drv;
>>  
>> -    ret = bdrv_file_open(&bs, filename, BDRV_O_RDONLY);
>> +    ret = bdrv_file_open(&bs, filename, 0);
>>      if (ret < 0)
>>          return NULL;
>>      ret = bdrv_pread(bs, 0, buf, sizeof(buf));
>> @@ -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];
>>  
>> @@ -446,19 +446,23 @@ int bdrv_open2(BlockDriverState *bs, const char *filename, int flags,
>>  
>>      /* 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
>> +    bs->read_only = (flags & BDRV_O_RDWR) == 0;
> 
> !(flags & BDRV_O_RDWR) is a standard idiom, and it's more readable IMO.
> 
>> +    if (!(flags & BDRV_O_FILE)) {
>> +        open_flags = (flags & (BDRV_O_RDWR | BDRV_O_CACHE_MASK|BDRV_O_NATIVE_AIO));
>> +        if (bs->is_temporary) { /* snapshot should be writeable */
>> +            open_flags |= BDRV_O_RDWR;
>> +        }
>> +    } else {
>>          open_flags = flags & ~(BDRV_O_FILE | BDRV_O_SNAPSHOT);
>> -    if (use_bdrv_whitelist && !bdrv_is_whitelisted(drv))
>> +    }
>> +    if (use_bdrv_whitelist && !bdrv_is_whitelisted(drv)) {
>>          ret = -ENOTSUP;
>> -    else
>> +    } 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);
>> -        bs->read_only = 1;
>> +        if ((ret == -EACCES || ret == -EPERM) && !(flags & BDRV_O_FILE)) {
>> +            ret = drv->bdrv_open(bs, filename, open_flags & ~BDRV_O_RDWR);
>> +            bs->read_only = 1;
>> +        }
>>      }
>>      if (ret < 0) {
>>          qemu_free(bs->opaque);
>> @@ -481,14 +485,13 @@ 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);
>>          ret = bdrv_open2(bs->backing_hd, backing_filename, open_flags,
>>                           back_drv);
>> +        bs->backing_hd->read_only =  (open_flags & BDRV_O_RDWR) == 0;
> 
> !(open_flags & BDRV_O_RDWR) is a standard idiom and it's more readable
> IMO.

Sorry, I prefer the more verbose style. The extra bytes on me. Seems more readable for me.

> Further, pls don't put two spaces after ==.

Sure, will correct.

> 
>>          if (ret < 0) {
>>              bdrv_close(bs);
>>              return ret;
>> diff --git a/block.h b/block.h
>> index bee9ec5..fd4e8dd 100644
>> --- a/block.h
>> +++ b/block.h
>> @@ -27,9 +27,7 @@ typedef struct QEMUSnapshotInfo {
>>      uint64_t vm_clock_nsec; /* VM clock relative to boot */
>>  } QEMUSnapshotInfo;
>>  
>> -#define BDRV_O_RDONLY      0x0000
>>  #define BDRV_O_RDWR        0x0002
>> -#define BDRV_O_ACCESS      0x0003
>>  #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
>> diff --git a/block/raw-posix.c b/block/raw-posix.c
>> index 5a6a22b..b427211 100644
>> --- a/block/raw-posix.c
>> +++ b/block/raw-posix.c
>> @@ -138,7 +138,7 @@ static int raw_open_common(BlockDriverState *bs, const char *filename,
>>  
>>      s->open_flags = open_flags | O_BINARY;
>>      s->open_flags &= ~O_ACCMODE;
>> -    if ((bdrv_flags & BDRV_O_ACCESS) == BDRV_O_RDWR) {
>> +    if (bdrv_flags & BDRV_O_RDWR) {
>>          s->open_flags |= O_RDWR;
>>      } else {
>>          s->open_flags |= O_RDONLY;
>> diff --git a/block/raw-win32.c b/block/raw-win32.c
>> index 72acad5..01a6d25 100644
>> --- a/block/raw-win32.c
>> +++ b/block/raw-win32.c
>> @@ -81,7 +81,7 @@ static int raw_open(BlockDriverState *bs, const char *filename, int flags)
>>  
>>      s->type = FTYPE_FILE;
>>  
>> -    if ((flags & BDRV_O_ACCESS) == O_RDWR) {
>> +    if (flags & BDRV_O_RDWR) {
>>          access_flags = GENERIC_READ | GENERIC_WRITE;
>>      } else {
>>          access_flags = GENERIC_READ;
>> @@ -337,7 +337,7 @@ static int hdev_open(BlockDriverState *bs, const char *filename, int flags)
>>      }
>>      s->type = find_device_type(bs, filename);
>>  
>> -    if ((flags & BDRV_O_ACCESS) == O_RDWR) {
>> +    if (flags & BDRV_O_RDWR) {
>>          access_flags = GENERIC_READ | GENERIC_WRITE;
>>      } else {
>>          access_flags = GENERIC_READ;
> 
> The above changes seem nothing to do with passing in parameter to the
> function. If the are intentional, pls mention them in changelog or split
> to a separate patch.

Correct, see my preface.

> 
>> diff --git a/block/vmdk.c b/block/vmdk.c
>> index 4e48622..ddc2fcb 100644
>> --- a/block/vmdk.c
>> +++ b/block/vmdk.c
>> @@ -357,7 +357,7 @@ static int vmdk_parent_open(BlockDriverState *bs, const char * filename)
>>              return -1;
>>          }
>>          parent_open = 1;
>> -        if (bdrv_open(bs->backing_hd, parent_img_name, BDRV_O_RDONLY) < 0)
>> +        if (bdrv_open(bs->backing_hd, parent_img_name, 0) < 0)
>>              goto failure;
>>          parent_open = 0;
>>      }
>> @@ -371,9 +371,10 @@ static int vmdk_open(BlockDriverState *bs, const char *filename, int flags)
>>      uint32_t magic;
>>      int l1_size, i, ret;
>>  
>> -    if (parent_open)
>> -        // Parent must be opened as RO.
>> -        flags = BDRV_O_RDONLY;
>> +    if (parent_open) {
>> +        /* Parent must be opened as RO, no RDWR. */
>> +        flags = 0;
>> +    }
>>  
>>      ret = bdrv_file_open(&s->hd, filename, flags);
>>      if (ret < 0)
>> diff --git a/hw/xen_disk.c b/hw/xen_disk.c
>> index 5c55251..beadf90 100644
>> --- a/hw/xen_disk.c
>> +++ b/hw/xen_disk.c
>> @@ -613,7 +613,7 @@ static int blk_init(struct XenDevice *xendev)
>>  	qflags = BDRV_O_RDWR;
>>      } else {
>>  	mode   = O_RDONLY;
>> -	qflags = BDRV_O_RDONLY;
>> +	qflags = 0;
>>  	info  |= VDISK_READONLY;
>>      }
>>  
>> diff --git a/monitor.c b/monitor.c
>> index b824e7c..5bb8872 100644
>> --- a/monitor.c
>> +++ b/monitor.c
>> @@ -916,7 +916,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_RDWR, drv);
> 
> This and below seems to change file from rdwr to readonly.
> Intentional?

Yes, intentioanl. The default used to be read-write, even when passed nothing,
see old code citation from bdrv_open2:
======BEGIN======
    /* Note: for compatibility, we open disk image files as RDWR, and
       RDONLY as fallback */
    if (!(flags & BDRV_O_FILE))
        open_flags = BDRV_O_RDWR | (flags & BDRV_O_CACHE_MASK);
======END======
Now you need to explicitly ask what you want.

> 
>>      monitor_read_bdrv_key_start(mon, bs, NULL, NULL);
>>  }
>>  
>> diff --git a/qemu-img.c b/qemu-img.c
>> index 48b9315..3cea8ce 100644
>> --- a/qemu-img.c
>> +++ b/qemu-img.c
>> @@ -204,7 +204,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_RDWR, drv) < 0) {
>>          error("Could not open '%s'", filename);
>>      }
>>      if (bdrv_is_encrypted(bs)) {
>> @@ -468,7 +468,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);
>> @@ -966,10 +966,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;
>>      /* Parse commandline parameters */
>>      for(;;) {
>>          c = getopt(argc, argv, "la:c:d:h");
>> @@ -985,6 +986,7 @@ static int img_snapshot(int argc, char **argv)
>>                  return 0;
>>              }
>>              action = SNAPSHOT_LIST;
>> +            bdrv_oflags &= ~BDRV_O_RDWR; /* no need for RW */
> 
> bdrv_oflags = BDRV_O_RDONLY would be clearer, and no need
> for comment then?

I had some thoughts about that. I added just few lines above the assignment:
'bdrv_oflags = BDRV_O_RDWR;'
so I could just overwrite it with:
'bdrv_oflags = 0; ' /* no  BDRV_O_RDONLY anymore */
but thought that's clearer.

> 
>>              break;
>>          case 'a':
>>              if (action) {
>> @@ -1022,7 +1024,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) {
>>          error("Could not open '%s'", filename);
>>      }
>>  
>> diff --git a/qemu-io.c b/qemu-io.c
>> index 1c19b92..b159bc9 100644
>> --- a/qemu-io.c
>> +++ b/qemu-io.c
>> @@ -1359,10 +1359,9 @@ open_f(int argc, char **argv)
>>  		}
>>  	}
>>  
>> -	if (readonly)
>> -		flags |= BDRV_O_RDONLY;
>> -	else
>> -		flags |= BDRV_O_RDWR;
>> +	if (!readonly) {
>> +            flags |= BDRV_O_RDWR;
>> +        }
>>  
>>  	if (optind != argc - 1)
>>  		return command_usage(&open_cmd);
>> @@ -1506,10 +1505,9 @@ int main(int argc, char **argv)
>>  	add_check_command(init_check_command);
>>  
>>  	/* open the device */
>> -	if (readonly)
>> -		flags |= BDRV_O_RDONLY;
>> -	else
>> -		flags |= BDRV_O_RDWR;
>> +	if (!readonly) {
>> +            flags |= BDRV_O_RDWR;
>> +        }
> 
> alignment seems broken in 2 places above

Sure, will fix (both).

> 
>>  
>>  	if ((argc - optind) == 1)
>>  		openfile(argv[optind], flags, growable);
>> diff --git a/qemu-nbd.c b/qemu-nbd.c
>> index 6707ea5..4463679 100644
>> --- a/qemu-nbd.c
>> +++ b/qemu-nbd.c
>> @@ -213,7 +213,7 @@ int main(int argc, char **argv)
>>      int opt_ind = 0;
>>      int li;
>>      char *end;
>> -    int flags = 0;
>> +    int flags = BDRV_O_RDWR;
>>      int partition = -1;
>>      int ret;
>>      int shared = 1;
>> diff --git a/vl.c b/vl.c
>> index 76ef8ca..eee59dd 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -2227,19 +2227,19 @@ DriveInfo *drive_init(QemuOpts *opts, void *opaque,
>>      }
>>  
>>      if (ro == 1) {
>> -        if (type == IF_IDE) {
>> -            fprintf(stderr, "qemu: readonly flag not supported for drive with ide interface\n");
>> +        if (type != IF_SCSI && type != IF_VIRTIO && type != IF_FLOPPY) {
> 
> So each new user will have to be added here? Any idea how todo this
> better? Can't relevant drive emulation check read_only flag and report
> an error? Or set a flag in drive info declaring readonly support.

Right. Second options seems better to me. Will do.

> 
>> +            fprintf(stderr, "qemu: readonly flag not supported for drive with this interface\n");
>>              return NULL;
>>          }
>> -        (void)bdrv_set_read_only(dinfo->bdrv, 1);
>>      }
>>      /* 
>>       * cdrom is read-only. Set it now, after above interface checking
>>       * since readonly attribute not explicitly required, so no error.
>>       */
>>      if (media == MEDIA_CDROM) {
>> -        (void)bdrv_set_read_only(dinfo->bdrv, 1);
>> +        ro = 1;
>>      }
>> +    bdrv_flags |= ro ? 0 : BDRV_O_RDWR;
>>  
>>      if (bdrv_open2(dinfo->bdrv, file, bdrv_flags, drv) < 0) {
>>          fprintf(stderr, "qemu: could not open disk image %s: %s\n",
>> -- 
>> 1.6.3.3
>>
>>

#

Re: [Qemu-devel] [PATCH v2 1/4] Make CDROM a read-only drive

[Jamie Lokier]

Naphtali Sprei wrote:
>          }
>          (void)bdrv_set_read_only(dinfo->bdrv, 1);
>      }
> +    /* 
> +     * cdrom is read-only. Set it now, after above interface checking
> +     * since readonly attribute not explicitly required, so no error.
> +     */
> +    if (media == MEDIA_CDROM) {
> +        (void)bdrv_set_read_only(dinfo->bdrv, 1);
> +    }
>  
>      if (bdrv_open2(dinfo->bdrv, file, bdrv_flags, drv) < 0) {
>          fprintf(stderr, "qemu: could not open disk image %s: %s\n",

When I have anything to say, I usually ask for more comments, but in
this case, I think the code would be perfectly clear without it.

-- Jamie

Re: [Qemu-devel] [PATCH v2 1/4] Make CDROM a read-only drive

[Anthony Liguori]

On 01/17/2010 08:48 AM, Naphtali Sprei wrote:
> Signed-off-by: Naphtali Sprei<nsprei@redhat.com>
>    

Applied.  Thanks.

Regards,

Anthony Liguori
> ---
>   vl.c |    7 +++++++
>   1 files changed, 7 insertions(+), 0 deletions(-)
>
> diff --git a/vl.c b/vl.c
> index 06cb40d..76ef8ca 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -2233,6 +2233,13 @@ DriveInfo *drive_init(QemuOpts *opts, void *opaque,
>           }
>           (void)bdrv_set_read_only(dinfo->bdrv, 1);
>       }
> +    /*
> +     * cdrom is read-only. Set it now, after above interface checking
> +     * since readonly attribute not explicitly required, so no error.
> +     */
> +    if (media == MEDIA_CDROM) {
> +        (void)bdrv_set_read_only(dinfo->bdrv, 1);
> +    }
>
>       if (bdrv_open2(dinfo->bdrv, file, bdrv_flags, drv)<  0) {
>           fprintf(stderr, "qemu: could not open disk image %s: %s\n",
>    

Re: [Qemu-devel] [PATCH v2 0/4] Modifications to the drives' readonly attribute

[Christoph Hellwig]

Looking at the version of this that landed in git I don't think the
read-only handling is entirely clean after this.

 - we now normally set the read_only flag from bdrv_open2 when we do
   not have the O_RDWR flag set
 - but the block drivers also mess with it:
   	o raw-posix superflously sets it when BDRV_O_RDWR is not in the
	  open flags
	o bochs, cloop, dmg and parallels set it unconditionally given
	  that they do not support writing at all.  But they do not
	  bother to reject opens without BDRV_O_RDWR
	o vvfat as usual is a complete mess setting and clearing it in
	  various places
 - in addition to that bdrv_open2 also sets it after calling itself for
   the backing hd which seems superflous
 - there also is a now unused bdrv_set_read_only helper to set it from
   outside block.c

Re: [Qemu-devel] [PATCH v2 0/4] Modifications to the drives' readonly attribute

[Naphtali Sprei]

Christoph Hellwig wrote:
> Looking at the version of this that landed in git I don't think the
> read-only handling is entirely clean after this.

I fixed what I could, still I got some questions below.

> 
>  - we now normally set the read_only flag from bdrv_open2 when we do
>    not have the O_RDWR flag set
>  - but the block drivers also mess with it:
>    	o raw-posix superflously sets it when BDRV_O_RDWR is not in the
> 	  open flags

Not sure where exactly is the issue. Can you please point the line ?

> 	o bochs, cloop, dmg and parallels set it unconditionally given
> 	  that they do not support writing at all.  But they do not
> 	  bother to reject opens without BDRV_O_RDWR

I just changed bochs and parallels not to ask for read-write.
Should all of them test the flags for RDWR and returns failure ?

> 	o vvfat as usual is a complete mess setting and clearing it in
> 	  various places

Fixed one occurance. More places ?

>  - in addition to that bdrv_open2 also sets it after calling itself for
>    the backing hd which seems superflous

Is this a problem ? I thought it's safer to mark it read-only, in case a write operation requested somehow.

>  - there also is a now unused bdrv_set_read_only helper to set it from
>    outside block.c

Done. Removed.

> 
> 

Thanks,

  Naphtali

Re: [Qemu-devel] [PATCH v2 0/4] Modifications to the drives' readonly attribute

[Christoph Hellwig]

On Thu, Jan 21, 2010 at 03:19:28PM +0200, Naphtali Sprei wrote:
> 
> > 
> >  - we now normally set the read_only flag from bdrv_open2 when we do
> >    not have the O_RDWR flag set
> >  - but the block drivers also mess with it:
> >    	o raw-posix superflously sets it when BDRV_O_RDWR is not in the
> > 	  open flags
> 
> Not sure where exactly is the issue. Can you please point the line ?

It's really just a now superflous place in the image driver that sets
the read_only flag.  Currently it's not clear who is supposed to set
the flag, we do it both from block.c and the image driver.

> > 	o bochs, cloop, dmg and parallels set it unconditionally given
> > 	  that they do not support writing at all.  But they do not
> > 	  bother to reject opens without BDRV_O_RDWR
> 
> I just changed bochs and parallels not to ask for read-write.
> Should all of them test the flags for RDWR and returns failure ?

That would be most logical, but might cause regressions for existing
setups that did not bother to specify the read-only option on the
command line.  Another options might be to allow the driver to return
EROFS and the retry a read-only open for the block layer for these.

> > 	o vvfat as usual is a complete mess setting and clearing it in
> > 	  various places
> 
> Fixed one occurance. More places ?

I mean the ->read_only flag setting and clearing.  As you've pulled
up the main place for setting it to the block layer the drivers
shouldn't mess with it anymore.

> >  - in addition to that bdrv_open2 also sets it after calling itself for
> >    the backing hd which seems superflous
> 
> Is this a problem ? I thought it's safer to mark it read-only, in case a write operation requested somehow.

It's superflous, bdrv_open2 always does it based on the argument, so
no need to do it a second time for the snapshot.