| Message ID | 1263739695-13043-3-git-send-email-nsprei@redhat.com |
|---|---|
| State | New |
| Headers | show |
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 > >
"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". [...]
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.
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 >> >> #
"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.
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
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.
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?
"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".
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.
"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 :)
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
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",
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(-)