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
On Wed, Jan 20, 2010 at 08:26:56AM +0100, Markus Armbruster wrote: Jamie Lokier ja...@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
Michael S. Tsirkin m...@redhat.com writes: On Wed, Jan 20, 2010 at 08:26:56AM +0100, Markus Armbruster wrote: Jamie Lokier ja...@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
On Wed, Jan 20, 2010 at 01:09:29PM +0100, Markus Armbruster wrote: Michael S. Tsirkin m...@redhat.com writes: On Wed, Jan 20, 2010 at 08:26:56AM +0100, Markus Armbruster wrote: Jamie Lokier ja...@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
Michael S. Tsirkin m...@redhat.com writes: On Wed, Jan 20, 2010 at 01:09:29PM +0100, Markus Armbruster wrote: Michael S. Tsirkin m...@redhat.com writes: On Wed, Jan 20, 2010 at 08:26:56AM +0100, Markus Armbruster wrote: Jamie Lokier ja...@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
Michael S. Tsirkin wrote: On Wed, Jan 20, 2010 at 08:26:56AM +0100, Markus Armbruster wrote: Jamie Lokier ja...@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
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
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
Jamie Lokier ja...@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
Michael S. Tsirkin m...@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 nsp...@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
On Mon, Jan 18, 2010 at 11:34:59AM +0100, Markus Armbruster wrote: Michael S. Tsirkin m...@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 nsp...@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
[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 exp
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 nsp...@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
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
Michael S. Tsirkin m...@redhat.com writes: On Mon, Jan 18, 2010 at 11:34:59AM +0100, Markus Armbruster wrote: Michael S. Tsirkin m...@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 nsp...@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.
[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 exp
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 nsp...@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 0x
#define BDRV_O_RDWR0x0002
-#define BDRV_O_ACCESS 0x0003
#define BDRV_O_CREAT 0x0004 /* create an empty file */
#define BDRV_O_SNAPSHOT0x0008 /* open the file read only and save writes
in a snapshot */
#define BDRV_O_FILE0x0010 /* 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
