Modifications to the drives' readonly attribute

Signed-off-by Naphtali Sprei <[email protected]>

---
 block.c |    6 ------
 1 files changed, 0 insertions(+), 6 deletions(-)

block.c +0 -2

index 8def3c4..f90e983 100644
@@ -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));

block.c +0 -0

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

Signature · 1 line

1.6.3.3

Quoted 13 lines · Naphtali Sprei

Signed-off-by Naphtali Sprei <[email protected]>

---
 block.c |    6 ------
 1 files changed, 0 insertions(+), 6 deletions(-)

block.c +0 -0

index 8def3c4..f90e983 100644
@@ -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;
 

block.c int bdrv_open2(BlockDriverState *bs, const char *filename, int flags, · line 465

Quoted 13 lines

block.c +0 -6

-    /* 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?

Quoted 8 lines

block.c +0 -0

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

Signature · 1 line

1.6.3.3

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 <[email protected]>

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

block.c +2 -2

index 115e591..8def3c4 100644
@@ -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];
 

block.c +13 -5

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

block.c +0 -0

         if (ret < 0) {
             bdrv_close(bs);
             return ret;

block.h +0 -2

index bee9ec5..fd4e8dd 100644
@@ -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

block/raw-posix.c +1 -1

index 5a6a22b..b427211 100644
@@ -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;

block/raw-win32.c +1 -1

index 72acad5..01a6d25 100644
@@ -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;

block/vmdk.c +5 -4

index 4e48622..ddc2fcb 100644
@@ -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)

hw/xen_disk.c +1 -1

index 5c55251..beadf90 100644
@@ -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;
     }
 

monitor.c +0 -0

index b824e7c..5bb8872 100644

monitor.c +0 -0

     monitor_read_bdrv_key_start(mon, bs, NULL, NULL);
 }
 

qemu-img.c +4 -3

index 48b9315..3cea8ce 100644
@@ -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");

qemu-img.c +1 -1

             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);
     }
 

qemu-io.c +3 -4

index 1c19b92..b159bc9 100644
@@ -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);

qemu-io.c +0 -0

 	if ((argc - optind) == 1)
 		openfile(argv[optind], flags, growable);

qemu-nbd.c +1 -1

index 6707ea5..4463679 100644
@@ -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;

vl.c +0 -0

index 76ef8ca..eee59dd 100644

vl.c +3 -2

+            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",

Signature · 1 line

1.6.3.3

Quoted 4 lines · 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 <[email protected]>

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.

qemu-img.c static int img_snapshot(int argc, char **argv) · line 989

Quoted 5 lines

qemu-img.c +1 -0

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

Quoted 10 lines · Michael S. Tsirkin

Quoted 4 lines · 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 <[email protected]>

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.

Quoted 3 lines

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

[...]

Quoted 8 lines

qemu-img.c static int img_snapshot(int argc, char **argv) · line 989

Quoted 5 lines

qemu-img.c +1 -0

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

[...]

Quoted 16 lines · Markus Armbruster

Quoted 13 lines · Michael S. Tsirkin

Quoted 4 lines · 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 <[email protected]>

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.

Quoted 2 lines

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?

Quoted 27 lines

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

[...]

Quoted 8 lines

qemu-img.c static int img_snapshot(int argc, char **argv) · line 989

Quoted 5 lines

qemu-img.c +1 -0

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

Quoted 5 lines

    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.

Quoted 2 lines · Markus Armbruster

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?

Quoted 6 lines · Michael S. Tsirkin

Quoted 2 lines · Markus Armbruster

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.

Quoted 5 lines · Jamie Lokier

Quoted 4 lines · Michael S. Tsirkin

Quoted 2 lines · Markus Armbruster

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

Quoted 5 lines

Quoted 2 lines

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.

Quoted 10 lines · Markus Armbruster

Quoted 8 lines · Jamie Lokier

Quoted 6 lines · Michael S. Tsirkin

Quoted 2 lines · Markus Armbruster

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?

Quoted 4 lines

Quoted 2 lines

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?

Quoted 14 lines · Michael S. Tsirkin

Quoted 13 lines · Markus Armbruster

Quoted 10 lines · Jamie Lokier

Quoted 6 lines · Michael S. Tsirkin

Quoted 2 lines · Markus Armbruster

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.

Quoted 3 lines

Quoted 1 line

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

Quoted 16 lines · Markus Armbruster

Quoted 14 lines · Michael S. Tsirkin

Quoted 13 lines · Markus Armbruster

Quoted 10 lines · Jamie Lokier

Quoted 6 lines · Michael S. Tsirkin

Quoted 2 lines · Markus Armbruster

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.

Quoted 6 lines

Quoted 3 lines

Quoted 1 line

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.

Quoted 10 lines · Markus Armbruster

Quoted 8 lines · Jamie Lokier

Quoted 6 lines · Michael S. Tsirkin

Quoted 2 lines · Markus Armbruster

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?

Quoted 4 lines

Quoted 2 lines

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?

Quoted 4 lines · 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 <[email protected]>

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.

Quoted 38 lines

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

block.c +2 -2

index 115e591..8def3c4 100644
@@ -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];
 

block.c int bdrv_open2(BlockDriverState *bs, const char *filename, int flags, · line 449

Quoted 9 lines

block.c +1 -5

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

Quoted 32 lines

block.c +13 -7

+    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);

block.c int bdrv_open2(BlockDriverState *bs, const char *filename, int flags, · line 494

Quoted 5 lines

block.c +1 -0

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

Quoted 48 lines

block.c +0 -0

         if (ret < 0) {
             bdrv_close(bs);
             return ret;

block.h +0 -2

index bee9ec5..fd4e8dd 100644
@@ -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

block/raw-posix.c +1 -1

index 5a6a22b..b427211 100644
@@ -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;

block/raw-win32.c +1 -1

index 72acad5..01a6d25 100644
@@ -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);
 

block/raw-win32.c static int raw_open(BlockDriverState *bs, const char *filename, int flags) · line 87

Quoted 5 lines

block/raw-win32.c +1 -1

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

Quoted 41 lines

block/vmdk.c +5 -4

index 4e48622..ddc2fcb 100644
@@ -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)

hw/xen_disk.c +1 -1

index 5c55251..beadf90 100644
@@ -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;
     }
 

monitor.c static void do_change_block(Monitor *mon, const char *device, · line 919

Quoted 10 lines

monitor.c +1 -1

index b824e7c..5bb8872 100644
@@ -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?

Quoted 35 lines

monitor.c +0 -0

     monitor_read_bdrv_key_start(mon, bs, NULL, NULL);
 }
 

qemu-img.c +3 -3

index 48b9315..3cea8ce 100644
@@ -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;
 

qemu-img.c static int img_snapshot(int argc, char **argv) · line 989

Quoted 9 lines

qemu-img.c +2 -0

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

Quoted 34 lines

qemu-img.c +1 -1

             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);
     }
 

qemu-io.c +3 -4

index 1c19b92..b159bc9 100644
@@ -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);
 

qemu-io.c int main(int argc, char **argv) · line 1510

Quoted 8 lines

qemu-io.c +3 -4

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

Quoted 24 lines

qemu-io.c +0 -0

 	if ((argc - optind) == 1)
 		openfile(argv[optind], flags, growable);

qemu-nbd.c +1 -1

index 6707ea5..4463679 100644
@@ -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;

vl.c +0 -0

index 76ef8ca..eee59dd 100644
@@ -2227,19 +2227,19 @@ DriveInfo *drive_init(QemuOpts *opts, void *opaque,
     }
 

vl.c DriveInfo *drive_init(QemuOpts *opts, void *opaque, · line 2230

Quoted 4 lines

vl.c +1 -2

     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.

Quoted 16 lines

vl.c +3 -2

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

Quoted 6 lines

vl.c +0 -0

     if (bdrv_open2(dinfo->bdrv, file, bdrv_flags, drv) < 0) {
         fprintf(stderr, "qemu: could not open disk image %s: %s\n",

Signature · 1 line

1.6.3.3

Signed-off-by Naphtali Sprei<[email protected]>

---
  vl.c |    7 +++++++
  1 files changed, 7 insertions(+), 0 deletions(-)

vl.c +7 -0

index 06cb40d..76ef8ca 100644
@@ -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",

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

Quoted 7 lines

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

Quoted 3 lines

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 ?

Quoted 2 lines

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

Fixed one occurance. More places ?

Quoted 2 lines

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