Patchwork [v2] Added 'access' option to -drive flag

login
register
mail settings
Submitter Naphtali Sprei
Date Dec. 24, 2009, 3:02 p.m.
Message ID <4B338293.4000306@redhat.com>
Download mbox | patch
Permalink /patch/41781/
State New
Headers show

Comments

Naphtali Sprei - Dec. 24, 2009, 3:02 p.m.
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 replaces 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.

The BDRV_O_RDWR/RDONLY becomes just one bit, BDRV_O_RDONLY gone.
---
 block.c           |   27 +++++++++++++++------------
 block.h           |    6 ++++--
 block/raw-posix.c |    2 +-
 block/raw-win32.c |    4 ++--
 block/vmdk.c      |    4 ++--
 hw/xen_disk.c     |    5 +++--
 monitor.c         |    2 +-
 qemu-config.c     |    4 ++--
 qemu-img.c        |   14 ++++++++------
 qemu-nbd.c        |    2 +-
 qemu-options.hx   |    2 +-
 vl.c              |   48 +++++++++++++++++++++++++++++++++++++++---------
 12 files changed, 79 insertions(+), 41 deletions(-)
Anthony Liguori - Jan. 7, 2010, 7:59 p.m.
On 12/24/2009 09:02 AM, Naphtali Sprei wrote:
> 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 replaces 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.
>
> The BDRV_O_RDWR/RDONLY becomes just one bit, BDRV_O_RDONLY gone.
>    

Users shouldn't be involved in deciding how an image is open.

Instead, marking a drive read only (via readonly) should cause a file to 
be open read-only.  By default, when media=cdrom, we should toggle the 
readonly flag.

As for the behaviour with trying read-write and then read-only, let's 
just drop it.  It's a bug and I've seen it reported half a dozen times 
as such.

Regards,

Anthony Liguori

Patch

diff --git a/block.c b/block.c
index 3f3496e..bdf140a 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, ~BDRV_O_RDWR);
     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];
 
@@ -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_RDWR, drv);
         if (ret < 0) {
             bdrv_delete(bs1);
             return ret;
@@ -445,18 +445,22 @@  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)) {
+    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);
         bs->read_only = 1;
     }
@@ -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/block/vmdk.c b/block/vmdk.c
index 4e48622..8a24084 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, ~BDRV_O_RDWR) < 0)
             goto failure;
         parent_open = 0;
     }
@@ -373,7 +373,7 @@  static int vmdk_open(BlockDriverState *bs, const char *filename, int flags)
 
     if (parent_open)
         // Parent must be opened as RO.
-        flags = BDRV_O_RDONLY;
+        flags = ~BDRV_O_RDWR;
 
     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..0a45ae3 100644
--- a/hw/xen_disk.c
+++ b/hw/xen_disk.c
@@ -610,10 +610,11 @@  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;
+	qflags = ~BDRV_O_RDWR;
 	info  |= VDISK_READONLY;
     }
 
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..6ced3f0 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_RDWR, 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_RDWR, 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_RDWR; /* 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 6707ea5..0f0e2fc 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_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..86836bf 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;
@@ -2435,17 +2460,22 @@  DriveInfo *drive_init(QemuOpts *opts, void *opaque,
     } else {
         bdrv_flags &= ~BDRV_O_NATIVE_AIO;
     }
+    
+    if (read_write) {
+        bdrv_flags |= BDRV_O_RDWR;
+    } else {
+        bdrv_flags &= ~BDRV_O_RDWR;
+    }
 
-    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);
+    if (ro_fallback) {
+        bdrv_flags |= BDRV_O_RO_FALLBACK;
+    } else {
+        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;
     }