| Message ID | 4B320927.8020702@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Wed, Dec 23, 2009 at 02:12:23PM +0200, Naphtali Sprei wrote: > Added 'access' option to -drive flag > > The new option is: access=[rw|ro|auto] > rw: open the drive's file with Read and Write permission, don't continue if failed > ro: open the file only with Read permission > auto: open the file with Read and Write permission, if failed, try only Read permision > > For compatibility reasons, the default is 'auto'. Should be changed later on. What is the plan for changing the default later? In particular, I'm curious to know why it will be able to be done later but not now. > This option is to replace the 'readonly' options added lately. > > 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 | 27 +++++++++++++++------------ > block.h | 6 ++++-- > block/raw-posix.c | 2 +- > block/raw-win32.c | 4 ++-- > hw/xen_disk.c | 3 ++- > monitor.c | 2 +- > qemu-config.c | 4 ++-- > qemu-img.c | 14 ++++++++------ > qemu-nbd.c | 2 +- > qemu-options.hx | 2 +- > vl.c | 42 +++++++++++++++++++++++++++++++++--------- > 11 files changed, 70 insertions(+), 38 deletions(-) > [snip] > index e881e45..79b8c27 100644 > --- a/vl.c > +++ b/vl.c > @@ -2090,6 +2090,7 @@ DriveInfo *drive_init(QemuOpts *opts, void *opaque, > int *fatal_error) > { > const char *buf; > + const char *access_buf = 0; > const char *file = NULL; > char devname[128]; > const char *serial; > @@ -2104,12 +2105,12 @@ DriveInfo *drive_init(QemuOpts *opts, void *opaque, > int index; > int cache; > int aio = 0; > - int ro = 0; > int bdrv_flags; > int on_read_error, on_write_error; > const char *devaddr; > DriveInfo *dinfo; > int snapshot = 0; > + int read_write, ro_fallback; > > *fatal_error = 1; > > @@ -2137,7 +2138,6 @@ DriveInfo *drive_init(QemuOpts *opts, void *opaque, > secs = qemu_opt_get_number(opts, "secs", 0); > > snapshot = qemu_opt_get_bool(opts, "snapshot", 0); > - ro = qemu_opt_get_bool(opts, "readonly", 0); > > file = qemu_opt_get(opts, "file"); > serial = qemu_opt_get(opts, "serial"); > @@ -2420,6 +2420,31 @@ DriveInfo *drive_init(QemuOpts *opts, void *opaque, > *fatal_error = 0; > return NULL; > } > + > + read_write = 1; > + ro_fallback = 1; > + access_buf = qemu_opt_get(opts, "access"); > + if (access_buf) { > + if (!strcmp(access_buf, "ro")) { > + read_write = 0; > + ro_fallback = 0; > + } else if (!strcmp(access_buf, "rw")) { > + read_write = 1; > + ro_fallback = 0; > + } else if (!strcmp(access_buf, "auto")) { /* default, but keep it explicit */ > + read_write = 1; > + ro_fallback = 1; > + } else { > + fprintf(stderr, "qemu: '%s' invalid 'access' option (valid values: [rw|ro|auto] )\n", access_buf); > + return NULL; > + } > + if (type != IF_SCSI && type != IF_VIRTIO && type != IF_FLOPPY && > + ( !strcmp(access_buf, "ro") || !strcmp(access_buf, "auto") )) { > + fprintf(stderr, "qemu: access=%s flag not supported for drive of this interface\n", access_buf); > + return NULL; > + } > + } > + I wonder if it would be easier for the parsing code to use flags directly rather than abstracting things out to read_write, ro_fallback. e.g. (Sorry if I mucked up the logic) int access_flags = BDRV_O_RDWR | BDRV_O_RO_FALLBACK; access_buf = qemu_opt_get(opts, "access"); if (access_buf) { if (!strcmp(access_buf, "ro")) access_flags = BDRV_O_RDONLY; else if (!strcmp(access_buf, "rw")) access_flags = BDRV_O_RDWR; else if (!strcmp(access_buf, "auto")) { /* default, but keep it explicit */ access_flags = BDRV_O_RDWR | BDRV_O_RO_FALLBACK; ... bdrv_flags |= access_flags; > diff --git a/vl.c b/vl.c > bdrv_flags = 0; > if (snapshot) { > bdrv_flags |= BDRV_O_SNAPSHOT; > @@ -2436,16 +2461,15 @@ DriveInfo *drive_init(QemuOpts *opts, void *opaque, > bdrv_flags &= ~BDRV_O_NATIVE_AIO; > } > > - if (ro == 1) { > - if (type == IF_IDE) { > - fprintf(stderr, "qemu: readonly flag not supported for drive with ide interface\n"); > - return NULL; > - } > - (void)bdrv_set_read_only(dinfo->bdrv, 1); > + bdrv_flags |= read_write ? BDRV_O_RDWR : BDRV_O_RDONLY; > + > + if (ro_fallback) { > + bdrv_flags |= BDRV_O_RO_FALLBACK; > } > + > > if (bdrv_open2(dinfo->bdrv, file, bdrv_flags, drv) < 0) { > - fprintf(stderr, "qemu: could not open disk image %s: %s\n", > + fprintf(stderr, "qemu: could not open disk image %s with requested permission: %s\n", > file, strerror(errno)); > return NULL; > } > -- > 1.6.3.3 > > >
Naphtali Sprei <nsprei@redhat.com> writes: > Added 'access' option to -drive flag > > The new option is: access=[rw|ro|auto] > rw: open the drive's file with Read and Write permission, don't continue if failed > ro: open the file only with Read permission > auto: open the file with Read and Write permission, if failed, try only Read permision > > For compatibility reasons, the default is 'auto'. Should be changed later on. > > This option is to replace the 'readonly' options added lately. Can we take the readonly parameter away? It's undocumented, for whatever that's worth... > Instead of using the field 'readonly' of the BlockDriverState struct for passing the request, > pass the request in the flags parameter to the function. You went half way from "we have a number of access modes stored in the bits BDRV_O_ACCESS, and you shouldn't assume anything on how they're encoded" to a simple, straightforward "writable" bit. I think the single bit leads to simpler clean code than the access mode, but half way is worse than either of them. Let me illustrate. Access mode: * Test writable: (flags & ~ BDRV_O_ACCESS) == BDRV_O_RDWR * Make writable: flags = (flags & ~ BDRV_O_ACCESS) | BDRV_O_RDWR; * Make read-only: flags = (flags & ~ BDRV_O_ACCESS) | BDRV_O_RDONLY; Single bit: * Test writable: flags & BDRV_O_WRITABLE * Make writable: flags |= BDRV_O_WRITABLE; * Make read-only: flags &= ~BDRV_O_WRITABLE; Your code can't quite decide which of the two methods to adopt. You still have BDRV_O_RDONLY and BDRV_O_RDWR. Clean users shouldn't assume anything about how the two are encoded. Consider: ret = drv->bdrv_open(bs, filename, (open_flags & ~BDRV_O_RDWR) | BDRV_O_RDONLY); Clean, but awkward. In another place, you opt for less awkward, but unclean: open_flags = (flags & (BDRV_O_RDWR | BDRV_O_CACHE_MASK|BDRV_O_NATIVE_AIO)); This assumes that BDRV_O_RDONLY is 0. 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.
Simon Horman wrote: > On Wed, Dec 23, 2009 at 02:12:23PM +0200, Naphtali Sprei wrote: >> Added 'access' option to -drive flag >> >> The new option is: access=[rw|ro|auto] >> rw: open the drive's file with Read and Write permission, don't continue if failed >> ro: open the file only with Read permission >> auto: open the file with Read and Write permission, if failed, try only Read permision >> >> For compatibility reasons, the default is 'auto'. Should be changed later on. > > What is the plan for changing the default later? I don't have a plan, this is a product management issue. > In particular, I'm curious to know why it will > be able to be done later but not now. There must be a way to give users advanced warning of a change coming, so they can get prepared for it. Maybe also some deprecation process needed, don't know how is it done in QEMU. > >> This option is to replace the 'readonly' options added lately. >> >> 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 | 27 +++++++++++++++------------ >> block.h | 6 ++++-- >> block/raw-posix.c | 2 +- >> block/raw-win32.c | 4 ++-- >> hw/xen_disk.c | 3 ++- >> monitor.c | 2 +- >> qemu-config.c | 4 ++-- >> qemu-img.c | 14 ++++++++------ >> qemu-nbd.c | 2 +- >> qemu-options.hx | 2 +- >> vl.c | 42 +++++++++++++++++++++++++++++++++--------- >> 11 files changed, 70 insertions(+), 38 deletions(-) >> > > [snip] > >> index e881e45..79b8c27 100644 >> --- a/vl.c >> +++ b/vl.c >> @@ -2090,6 +2090,7 @@ DriveInfo *drive_init(QemuOpts *opts, void *opaque, >> int *fatal_error) >> { >> const char *buf; >> + const char *access_buf = 0; >> const char *file = NULL; >> char devname[128]; >> const char *serial; >> @@ -2104,12 +2105,12 @@ DriveInfo *drive_init(QemuOpts *opts, void *opaque, >> int index; >> int cache; >> int aio = 0; >> - int ro = 0; >> int bdrv_flags; >> int on_read_error, on_write_error; >> const char *devaddr; >> DriveInfo *dinfo; >> int snapshot = 0; >> + int read_write, ro_fallback; >> >> *fatal_error = 1; >> >> @@ -2137,7 +2138,6 @@ DriveInfo *drive_init(QemuOpts *opts, void *opaque, >> secs = qemu_opt_get_number(opts, "secs", 0); >> >> snapshot = qemu_opt_get_bool(opts, "snapshot", 0); >> - ro = qemu_opt_get_bool(opts, "readonly", 0); >> >> file = qemu_opt_get(opts, "file"); >> serial = qemu_opt_get(opts, "serial"); >> @@ -2420,6 +2420,31 @@ DriveInfo *drive_init(QemuOpts *opts, void *opaque, >> *fatal_error = 0; >> return NULL; >> } >> + >> + read_write = 1; >> + ro_fallback = 1; >> + access_buf = qemu_opt_get(opts, "access"); >> + if (access_buf) { >> + if (!strcmp(access_buf, "ro")) { >> + read_write = 0; >> + ro_fallback = 0; >> + } else if (!strcmp(access_buf, "rw")) { >> + read_write = 1; >> + ro_fallback = 0; >> + } else if (!strcmp(access_buf, "auto")) { /* default, but keep it explicit */ >> + read_write = 1; >> + ro_fallback = 1; >> + } else { >> + fprintf(stderr, "qemu: '%s' invalid 'access' option (valid values: [rw|ro|auto] )\n", access_buf); >> + return NULL; >> + } >> + if (type != IF_SCSI && type != IF_VIRTIO && type != IF_FLOPPY && >> + ( !strcmp(access_buf, "ro") || !strcmp(access_buf, "auto") )) { >> + fprintf(stderr, "qemu: access=%s flag not supported for drive of this interface\n", access_buf); >> + return NULL; >> + } >> + } >> + > I wonder if it would be easier for the parsing > code to use flags directly rather than abstracting > things out to read_write, ro_fallback. I felt this is more readable, even though it's a bit longer. > > e.g. > > (Sorry if I mucked up the logic) > > int access_flags = BDRV_O_RDWR | BDRV_O_RO_FALLBACK; > access_buf = qemu_opt_get(opts, "access"); > if (access_buf) { > if (!strcmp(access_buf, "ro")) > access_flags = BDRV_O_RDONLY; > else if (!strcmp(access_buf, "rw")) > access_flags = BDRV_O_RDWR; > else if (!strcmp(access_buf, "auto")) { /* default, but keep it explicit */ > access_flags = BDRV_O_RDWR | BDRV_O_RO_FALLBACK; > ... > > bdrv_flags |= access_flags; > >> diff --git a/vl.c b/vl.c >> bdrv_flags = 0; >> if (snapshot) { >> bdrv_flags |= BDRV_O_SNAPSHOT; >> @@ -2436,16 +2461,15 @@ DriveInfo *drive_init(QemuOpts *opts, void *opaque, >> bdrv_flags &= ~BDRV_O_NATIVE_AIO; >> } >> >> - if (ro == 1) { >> - if (type == IF_IDE) { >> - fprintf(stderr, "qemu: readonly flag not supported for drive with ide interface\n"); >> - return NULL; >> - } >> - (void)bdrv_set_read_only(dinfo->bdrv, 1); >> + bdrv_flags |= read_write ? BDRV_O_RDWR : BDRV_O_RDONLY; >> + >> + if (ro_fallback) { >> + bdrv_flags |= BDRV_O_RO_FALLBACK; >> } >> + >> >> if (bdrv_open2(dinfo->bdrv, file, bdrv_flags, drv) < 0) { >> - fprintf(stderr, "qemu: could not open disk image %s: %s\n", >> + fprintf(stderr, "qemu: could not open disk image %s with requested permission: %s\n", >> file, strerror(errno)); >> return NULL; >> } >> -- >> 1.6.3.3 >> >> >>
On 12/24/2009 03:09 AM, Markus Armbruster wrote: > Naphtali Sprei<nsprei@redhat.com> writes: > >> Added 'access' option to -drive flag >> >> The new option is: access=[rw|ro|auto] >> rw: open the drive's file with Read and Write permission, don't continue if failed >> ro: open the file only with Read permission >> auto: open the file with Read and Write permission, if failed, try only Read permision >> >> For compatibility reasons, the default is 'auto'. Should be changed later on. >> >> This option is to replace the 'readonly' options added lately. > > Can we take the readonly parameter away? It's undocumented, for > whatever that's worth... readonly made 0.12. Semantics, readonly makes it to the disk emulation whereas this effects how the file is opened. Regards, Anthony Liguori
Anthony Liguori wrote: > On 12/24/2009 03:09 AM, Markus Armbruster wrote: > >Naphtali Sprei<nsprei@redhat.com> writes: > > > >>Added 'access' option to -drive flag > >> > >>The new option is: access=[rw|ro|auto] > >>rw: open the drive's file with Read and Write permission, don't continue > >>if failed > >>ro: open the file only with Read permission > >>auto: open the file with Read and Write permission, if failed, try only > >>Read permision > >> > >>For compatibility reasons, the default is 'auto'. Should be changed later > >>on. > >> > >>This option is to replace the 'readonly' options added lately. > > > >Can we take the readonly parameter away? It's undocumented, for > >whatever that's worth... > > readonly made 0.12. Semantics, readonly makes it to the disk emulation > whereas this effects how the file is opened. With readonly in 0.12, if you _don't specify readonly, and the file is opened readonly because it applies qemu's fallback behaviour - does *that* read-only property make it to the disk emulation? Or do guests still see unexplained I/O errors in that case? Btw, wasn't the access=[rw|ro|auto] option supposed to affect disk emulation too? -- Jamie
Jamie Lokier wrote: > Anthony Liguori wrote: >> On 12/24/2009 03:09 AM, Markus Armbruster wrote: >>> Naphtali Sprei<nsprei@redhat.com> writes: >>> >>>> Added 'access' option to -drive flag >>>> >>>> The new option is: access=[rw|ro|auto] >>>> rw: open the drive's file with Read and Write permission, don't continue >>>> if failed >>>> ro: open the file only with Read permission >>>> auto: open the file with Read and Write permission, if failed, try only >>>> Read permision >>>> >>>> For compatibility reasons, the default is 'auto'. Should be changed later >>>> on. >>>> >>>> This option is to replace the 'readonly' options added lately. >>> Can we take the readonly parameter away? It's undocumented, for >>> whatever that's worth... >> readonly made 0.12. Semantics, readonly makes it to the disk emulation >> whereas this effects how the file is opened. I'm not sure I understand this semantic difference. The implementation of both versions (readonly and access) affects both the disk emulation and the file access/open. I did meant that 'access' to replace the 'readonly', and I do understand that I did it in bad timing. > > With readonly in 0.12, if you _don't specify readonly, and the file is > opened readonly because it applies qemu's fallback behaviour - does > *that* read-only property make it to the disk emulation? Or do guests > still see unexplained I/O errors in that case? The implementation of both 'readonly' and 'access' pass the information to the Guest, through the device API. Indeed, only for supporting devices. > > Btw, wasn't the access=[rw|ro|auto] option supposed to affect disk > emulation too? > > -- Jamie
diff --git a/block.c b/block.c index 3f3496e..e8a48a4 100644 --- a/block.c +++ b/block.c @@ -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]; @@ -378,7 +378,7 @@ int bdrv_open2(BlockDriverState *bs, const char *filename, int flags, /* if there is a backing file, use it */ bs1 = bdrv_new(""); - ret = bdrv_open2(bs1, filename, 0, drv); + ret = bdrv_open2(bs1, filename, BDRV_O_RDONLY, drv); if (ret < 0) { bdrv_delete(bs1); return ret; @@ -445,19 +445,23 @@ int bdrv_open2(BlockDriverState *bs, const char *filename, int flags, bs->enable_write_cache = 1; /* 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 + RDONLY as fallback (if flag BDRV_O_RO_FALLBACK is on) */ + bs->read_only = BDRV_FLAGS_IS_RO(flags); + if (!(flags & BDRV_O_FILE)) { + open_flags = (flags & (BDRV_O_RDWR | BDRV_O_CACHE_MASK|BDRV_O_NATIVE_AIO)); + if (bs->is_temporary) { /* snapshot */ + open_flags |= BDRV_O_RDWR; + } + } else { open_flags = flags & ~(BDRV_O_FILE | BDRV_O_SNAPSHOT); + } if (use_bdrv_whitelist && !bdrv_is_whitelisted(drv)) ret = -ENOTSUP; else ret = drv->bdrv_open(bs, filename, open_flags); - if ((ret == -EACCES || ret == -EPERM) && !(flags & BDRV_O_FILE)) { - ret = drv->bdrv_open(bs, filename, open_flags & ~BDRV_O_RDWR); + if ((ret == -EACCES || ret == -EPERM) && !(flags & BDRV_O_FILE) && + (flags & BDRV_O_RO_FALLBACK)) { + ret = drv->bdrv_open(bs, filename, (open_flags & ~BDRV_O_RDWR) | BDRV_O_RDONLY); bs->read_only = 1; } if (ret < 0) { @@ -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 = BDRV_FLAGS_IS_RO(open_flags); if (ret < 0) { bdrv_close(bs); return ret; diff --git a/block.h b/block.h index fa51ddf..f2501be 100644 --- a/block.h +++ b/block.h @@ -28,8 +28,8 @@ typedef struct QEMUSnapshotInfo { } QEMUSnapshotInfo; #define BDRV_O_RDONLY 0x0000 -#define BDRV_O_RDWR 0x0002 -#define BDRV_O_ACCESS 0x0003 +#define BDRV_O_RDWR 0x0001 +#define BDRV_O_RO_FALLBACK 0x0002 #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 @@ -41,6 +41,8 @@ typedef struct QEMUSnapshotInfo { #define BDRV_O_NATIVE_AIO 0x0080 /* use native AIO instead of the thread pool */ #define BDRV_O_CACHE_MASK (BDRV_O_NOCACHE | BDRV_O_CACHE_WB) +#define BDRV_O_DEFAULT_OPEN (BDRV_O_RDWR | BDRV_O_RO_FALLBACK) +#define BDRV_FLAGS_IS_RO(flags) ((flags & BDRV_O_RDWR) == 0) #define BDRV_SECTOR_BITS 9 #define BDRV_SECTOR_SIZE (1 << BDRV_SECTOR_BITS) 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/hw/xen_disk.c b/hw/xen_disk.c index 5c55251..7c39986 100644 --- a/hw/xen_disk.c +++ b/hw/xen_disk.c @@ -610,7 +610,8 @@ static int blk_init(struct XenDevice *xendev) /* read-only ? */ if (strcmp(blkdev->mode, "w") == 0) { mode = O_RDWR; - qflags = BDRV_O_RDWR; + /* for compatibility, also allow readonly fallback, for now */ + qflags = BDRV_O_RDWR | BDRV_O_RO_FALLBACK; } else { mode = O_RDONLY; qflags = BDRV_O_RDONLY; diff --git a/monitor.c b/monitor.c index c0dc48e..57b1aba 100644 --- a/monitor.c +++ b/monitor.c @@ -915,7 +915,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_DEFAULT_OPEN, drv); monitor_read_bdrv_key_start(mon, bs, NULL, NULL); } diff --git a/qemu-config.c b/qemu-config.c index c3203c8..f7dd4a0 100644 --- a/qemu-config.c +++ b/qemu-config.c @@ -76,8 +76,8 @@ QemuOptsList qemu_drive_opts = { .type = QEMU_OPT_STRING, .help = "pci address (virtio only)", },{ - .name = "readonly", - .type = QEMU_OPT_BOOL, + .name = "access", + .type = QEMU_OPT_STRING, }, { /* end if list */ } }, diff --git a/qemu-img.c b/qemu-img.c index 1d97f2e..cb04d2a 100644 --- a/qemu-img.c +++ b/qemu-img.c @@ -201,7 +201,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_DEFAULT_OPEN, drv) < 0) { error("Could not open '%s'", filename); } if (bdrv_is_encrypted(bs)) { @@ -406,7 +406,7 @@ static int img_check(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_RDONLY, drv) < 0) { error("Could not open '%s'", filename); } ret = bdrv_check(bs); @@ -465,7 +465,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); @@ -884,7 +884,7 @@ static int img_info(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_RDONLY, drv) < 0) { error("Could not open '%s'", filename); } bdrv_get_format(bs, fmt_name, sizeof(fmt_name)); @@ -932,10 +932,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; /* but no read-only fallback */ /* Parse commandline parameters */ for(;;) { c = getopt(argc, argv, "la:c:d:h"); @@ -951,6 +952,7 @@ static int img_snapshot(int argc, char **argv) return 0; } action = SNAPSHOT_LIST; + bdrv_oflags = BDRV_O_RDONLY; /* no need for RW */ break; case 'a': if (action) { @@ -988,7 +990,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-nbd.c b/qemu-nbd.c index 6cdb834..07e2134 100644 --- a/qemu-nbd.c +++ b/qemu-nbd.c @@ -212,7 +212,7 @@ int main(int argc, char **argv) int opt_ind = 0; int li; char *end; - int flags = 0; + int flags = BDRV_O_DEFAULT_OPEN; int partition = -1; int ret; int shared = 1; diff --git a/qemu-options.hx b/qemu-options.hx index b8cc375..a83628c 100644 --- a/qemu-options.hx +++ b/qemu-options.hx @@ -103,7 +103,7 @@ DEF("drive", HAS_ARG, QEMU_OPTION_drive, "-drive [file=file][,if=type][,bus=n][,unit=m][,media=d][,index=i]\n" " [,cyls=c,heads=h,secs=s[,trans=t]][,snapshot=on|off]\n" " [,cache=writethrough|writeback|none][,format=f][,serial=s]\n" - " [,addr=A][,id=name][,aio=threads|native]\n" + " [,addr=A][,id=name][,aio=threads|native][,access=rw|ro|auto]\n" " use 'file' as a drive image\n") DEF("set", HAS_ARG, QEMU_OPTION_set, "-set group.id.arg=value\n" diff --git a/vl.c b/vl.c index e881e45..79b8c27 100644 --- a/vl.c +++ b/vl.c @@ -2090,6 +2090,7 @@ DriveInfo *drive_init(QemuOpts *opts, void *opaque, int *fatal_error) { const char *buf; + const char *access_buf = 0; const char *file = NULL; char devname[128]; const char *serial; @@ -2104,12 +2105,12 @@ DriveInfo *drive_init(QemuOpts *opts, void *opaque, int index; int cache; int aio = 0; - int ro = 0; int bdrv_flags; int on_read_error, on_write_error; const char *devaddr; DriveInfo *dinfo; int snapshot = 0; + int read_write, ro_fallback; *fatal_error = 1; @@ -2137,7 +2138,6 @@ DriveInfo *drive_init(QemuOpts *opts, void *opaque, secs = qemu_opt_get_number(opts, "secs", 0); snapshot = qemu_opt_get_bool(opts, "snapshot", 0); - ro = qemu_opt_get_bool(opts, "readonly", 0); file = qemu_opt_get(opts, "file"); serial = qemu_opt_get(opts, "serial"); @@ -2420,6 +2420,31 @@ DriveInfo *drive_init(QemuOpts *opts, void *opaque, *fatal_error = 0; return NULL; } + + read_write = 1; + ro_fallback = 1; + access_buf = qemu_opt_get(opts, "access"); + if (access_buf) { + if (!strcmp(access_buf, "ro")) { + read_write = 0; + ro_fallback = 0; + } else if (!strcmp(access_buf, "rw")) { + read_write = 1; + ro_fallback = 0; + } else if (!strcmp(access_buf, "auto")) { /* default, but keep it explicit */ + read_write = 1; + ro_fallback = 1; + } else { + fprintf(stderr, "qemu: '%s' invalid 'access' option (valid values: [rw|ro|auto] )\n", access_buf); + return NULL; + } + if (type != IF_SCSI && type != IF_VIRTIO && type != IF_FLOPPY && + ( !strcmp(access_buf, "ro") || !strcmp(access_buf, "auto") )) { + fprintf(stderr, "qemu: access=%s flag not supported for drive of this interface\n", access_buf); + return NULL; + } + } + bdrv_flags = 0; if (snapshot) { bdrv_flags |= BDRV_O_SNAPSHOT; @@ -2436,16 +2461,15 @@ DriveInfo *drive_init(QemuOpts *opts, void *opaque, bdrv_flags &= ~BDRV_O_NATIVE_AIO; } - if (ro == 1) { - if (type == IF_IDE) { - fprintf(stderr, "qemu: readonly flag not supported for drive with ide interface\n"); - return NULL; - } - (void)bdrv_set_read_only(dinfo->bdrv, 1); + bdrv_flags |= read_write ? BDRV_O_RDWR : BDRV_O_RDONLY; + + if (ro_fallback) { + bdrv_flags |= BDRV_O_RO_FALLBACK; } + if (bdrv_open2(dinfo->bdrv, file, bdrv_flags, drv) < 0) { - fprintf(stderr, "qemu: could not open disk image %s: %s\n", + fprintf(stderr, "qemu: could not open disk image %s with requested permission: %s\n", file, strerror(errno)); return NULL; }
Added 'access' option to -drive flag The new option is: access=[rw|ro|auto] rw: open the drive's file with Read and Write permission, don't continue if failed ro: open the file only with Read permission auto: open the file with Read and Write permission, if failed, try only Read permision For compatibility reasons, the default is 'auto'. Should be changed later on. This option is to replace the 'readonly' options added lately. 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 | 27 +++++++++++++++------------ block.h | 6 ++++-- block/raw-posix.c | 2 +- block/raw-win32.c | 4 ++-- hw/xen_disk.c | 3 ++- monitor.c | 2 +- qemu-config.c | 4 ++-- qemu-img.c | 14 ++++++++------ qemu-nbd.c | 2 +- qemu-options.hx | 2 +- vl.c | 42 +++++++++++++++++++++++++++++++++--------- 11 files changed, 70 insertions(+), 38 deletions(-)