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 <nsp...@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
>>
>>
>>
