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