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