[Qemu-devel] PATCH: v3 Allow control over drive file open mode

["Daniel P. Berrange" <berrange@redhat.com>]

This is version 3 of the patch.

The current block driver code will attempt to open a file backing a drive
for read/write with O_RDWR first, and if that fails, fallback to opening
it readonly with O_RDONLY. So if you set file permissions to readonly on
the underlying drive backing store, QEMU will fallback to opening it read
only, and discard any writes.

Xen has a concept of a read-only disks in its configuration format, and
thus it would be desirable to have an explicit option to request that a
drive operate read-only, regardless of whether underlying file permissions
allow write access or not. We'd like to support this in libvirt too for
QEMU/KVM guests. Finally, in some cases it is desirable to see the failure
if the disk can't be opened read-write, rather than falling back to read
only mode - many guests will be more or less inoperable if their root
filesystem is read-only so there's little point booting.

The current block.h file did already have flags defined

  #define BDRV_O_RDONLY      0x0000
  #define BDRV_O_RDWR        0x0002
  #define BDRV_O_ACCESS      0x0003

However the bdrv_open2() method was treating a 'flags' value of 0, as being
effectively RDWR, and nearly all callers pass in 0 even when they expect
to get a writable file, so the O_RDONLY flag was not usable as is.

So this patch does a couple of things:

 - Change the value of the RDONLY flag to be 0x0001  so it can be
   distinguished from 0, which all callers assume means read-write
   will read-only fallback.

 - Adds a new option to the '-drive' command line parameter to specify
   how the file should be opened, and any fallback

     mode=rw  - try to open read-write and return error if this fails.
     mode=ro  - try to open read-only and return error if this fails

   If no 'mode' is provided, it'll try read-write, and fallback to
   read-only. This matches existing behaviour

 - Extends the monitor 'change' command to allow an optional mode to
   be specified, again defaulting to current behaviour. Takes the same
   values as the 'mode' param

   Signed-off-by: Daniel P. Berrange <berrange@redhat.com>


 block-raw-posix.c |    6 +++---
 block-raw-win32.c |    4 ++--
 block.c           |   21 ++++++++++++++-------
 block.h           |    5 ++---
 monitor.c         |   20 ++++++++++++++------
 qemu-doc.texi     |    4 ++++
 qemu-nbd.c        |    3 +++
 vl.c              |   15 ++++++++++++---
 8 files changed, 54 insertions(+), 24 deletions(-)

NB This is against SVN revision 4975, but will likely conflict with 
the other patch currently being discussed onlist wrt to qcow encryption
and passwords. I can trivially rebase it if required.


Daniel

Index: block-raw-win32.c
===================================================================
--- block-raw-win32.c	(revision 4975)
+++ block-raw-win32.c	(working copy)
@@ -90,7 +90,7 @@
 
     s->type = FTYPE_FILE;
 
-    if ((flags & BDRV_O_ACCESS) == O_RDWR) {
+    if (!(flags & BDRV_O_RDONLY)) {
         access_flags = GENERIC_READ | GENERIC_WRITE;
     } else {
         access_flags = GENERIC_READ;
@@ -463,7 +463,7 @@
     }
     s->type = find_device_type(bs, filename);
 
-    if ((flags & BDRV_O_ACCESS) == O_RDWR) {
+    if (!(flags & BDRV_O_RDONLY)) {
         access_flags = GENERIC_READ | GENERIC_WRITE;
     } else {
         access_flags = GENERIC_READ;
Index: vl.c
===================================================================
--- vl.c	(revision 4975)
+++ vl.c	(working copy)
@@ -5399,11 +5399,12 @@
     int max_devs;
     int index;
     int cache;
+    int mode = 0;  /* Default to writable, with fallback to readonly for back-compat */
     int bdrv_flags;
     char *str = arg->opt;
     char *params[] = { "bus", "unit", "if", "index", "cyls", "heads",
                        "secs", "trans", "media", "snapshot", "file",
-                       "cache", "format", NULL };
+                       "cache", "format", "mode", NULL };
 
     if (check_params(buf, sizeof(buf), params, str) < 0) {
          fprintf(stderr, "qemu: unknown parameter '%s' in '%s'\n",
@@ -5585,6 +5586,14 @@
         }
     }
 
+    if (get_param_value(buf, sizeof(buf), "mode", str)) {
+        if (strcmp(buf, "ro") == 0)
+            mode = BDRV_O_RDONLY;
+        else if (strcmp(buf, "rw") == 0)
+            mode = BDRV_O_RDWR;
+    }
+
+
     if (arg->file == NULL)
         get_param_value(file, sizeof(file), "file", str);
     else
@@ -5682,7 +5691,7 @@
     }
     if (!file[0])
         return 0;
-    bdrv_flags = 0;
+    bdrv_flags = mode;
     if (snapshot)
         bdrv_flags |= BDRV_O_SNAPSHOT;
     if (!cache)
@@ -7605,7 +7614,7 @@
            "-cdrom file     use 'file' as IDE cdrom image (cdrom is ide1 master)\n"
 	   "-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=on|off][,format=f]\n"
+           "       [,cache=on|off][,format=f][,mode=ro|rw]\n"
 	   "                use 'file' as a drive image\n"
            "-mtdblock file  use 'file' as on-board Flash memory image\n"
            "-sd file        use 'file' as SecureDigital card image\n"
Index: block-raw-posix.c
===================================================================
--- block-raw-posix.c	(revision 4975)
+++ block-raw-posix.c	(working copy)
@@ -103,7 +103,7 @@
     s->lseek_err_cnt = 0;
 
     open_flags = O_BINARY;
-    if ((flags & BDRV_O_ACCESS) == O_RDWR) {
+    if (!(flags & BDRV_O_RDONLY)) {
         open_flags |= O_RDWR;
     } else {
         open_flags |= O_RDONLY;
@@ -117,7 +117,7 @@
 #endif
 
     s->type = FTYPE_FILE;
-
+    fprintf(stderr, "Open raw %d %d %s\n", open_flags, flags, filename);
     fd = open(filename, open_flags, 0644);
     if (fd < 0) {
         ret = -errno;
@@ -896,7 +896,7 @@
     }
 #endif
     open_flags = O_BINARY;
-    if ((flags & BDRV_O_ACCESS) == O_RDWR) {
+    if (!(flags & BDRV_O_RDONLY)) {
         open_flags |= O_RDWR;
     } else {
         open_flags |= O_RDONLY;
Index: qemu-nbd.c
===================================================================
--- qemu-nbd.c	(revision 4975)
+++ qemu-nbd.c	(working copy)
@@ -332,6 +332,9 @@
     if (bs == NULL)
         return 1;
 
+    if (readonly)
+        flags |= BDRV_O_RDONLY;
+
     if (bdrv_open(bs, argv[optind], flags) == -1)
         return 1;
 
Index: qemu-doc.texi
===================================================================
--- qemu-doc.texi	(revision 4975)
+++ qemu-doc.texi	(working copy)
@@ -268,6 +268,10 @@
 @var{snapshot} is "on" or "off" and allows to enable snapshot for given drive (see @option{-snapshot}).
 @item cache=@var{cache}
 @var{cache} is "on" or "off" and allows to disable host cache to access data.
+@item mode=@var{mode}
+@var{mode} is "rw" to open the file read-write, or "ro" to open the file read-only.
+If neither is specified, an attempt will be made to open read-write, falling back
+to read-only if this fails.
 @item format=@var{format}
 Specify which disk @var{format} will be used rather than detecting
 the format.  Can be used to specifiy format=raw to avoid interpreting
Index: monitor.c
===================================================================
--- monitor.c	(revision 4975)
+++ monitor.c	(working copy)
@@ -396,11 +396,19 @@
     eject_device(bs, force);
 }
 
-static void do_change_block(const char *device, const char *filename, const char *fmt)
+static void do_change_block(const char *device, const char *filename, const char *fmt, const char *mode)
 {
     BlockDriverState *bs;
     BlockDriver *drv = NULL;
+    int flags = 0;
 
+    if (mode) {
+        if (strcmp(mode, "ro") == 0)
+            flags = BDRV_O_RDONLY;
+        else if (strcmp(mode, "rw") == 0)
+            flags = BDRV_O_RDWR;
+    }
+
     bs = bdrv_find(device);
     if (!bs) {
         term_printf("device not found\n");
@@ -415,7 +423,7 @@
     }
     if (eject_device(bs, 0) < 0)
         return;
-    bdrv_open2(bs, filename, 0, drv);
+    bdrv_open2(bs, filename, flags, drv);
     qemu_key_check(bs, filename);
 }
 
@@ -434,12 +442,12 @@
     }
 }
 
-static void do_change(const char *device, const char *target, const char *fmt)
+static void do_change(const char *device, const char *target, const char *fmt, const char *mode)
 {
     if (strcmp(device, "vnc") == 0) {
 	do_change_vnc(target);
     } else {
-	do_change_block(device, target, fmt);
+	do_change_block(device, target, fmt, mode);
     }
 }
 
@@ -1374,8 +1382,8 @@
       "", "quit the emulator" },
     { "eject", "-fB", do_eject,
       "[-f] device", "eject a removable medium (use -f to force it)" },
-    { "change", "BFs?", do_change,
-      "device filename [format]", "change a removable medium, optional format" },
+    { "change", "BFs?s?", do_change,
+      "device filename [format] [mode]", "change a removable medium, optional format and mode" },
     { "screendump", "F", do_screen_dump,
       "filename", "save screen into PPM image 'filename'" },
     { "logfile", "F", do_logfile,
Index: block.c
===================================================================
--- block.c	(revision 4975)
+++ block.c	(working copy)
@@ -381,17 +381,24 @@
     bs->opaque = qemu_mallocz(drv->instance_size);
     if (bs->opaque == NULL && drv->instance_size > 0)
         return -1;
-    /* Note: for compatibility, we open disk image files as RDWR, and
-       RDONLY as fallback */
+    /* If BDRV_O_RDONLY is set open in read only mode
+     * If BDRV_O_RDWR is set open in read write mode
+     * If neither is set, try read-write and fallback to read only
+     */
     if (!(flags & BDRV_O_FILE))
-        open_flags = BDRV_O_RDWR | (flags & BDRV_O_DIRECT);
+        open_flags = (flags & (BDRV_O_DIRECT | BDRV_O_RDONLY | BDRV_O_RDWR));
     else
         open_flags = flags & ~(BDRV_O_FILE | BDRV_O_SNAPSHOT);
     ret = drv->bdrv_open(bs, filename, open_flags);
-    if (ret == -EACCES && !(flags & BDRV_O_FILE)) {
-        ret = drv->bdrv_open(bs, filename, BDRV_O_RDONLY);
+
+    /* Trying fallback to read only mode here.. */
+    if (ret == -EACCES &&
+	!(flags & (BDRV_O_RDWR | BDRV_O_RDONLY))) {
+        open_flags |= BDRV_O_RDONLY;
+        ret = drv->bdrv_open(bs, filename, open_flags);
+    }
+    if (ret == 0 && (flags & BDRV_O_RDONLY))
         bs->read_only = 1;
-    }
     if (ret < 0) {
         qemu_free(bs->opaque);
         bs->opaque = NULL;
@@ -416,7 +423,7 @@
         }
         path_combine(backing_filename, sizeof(backing_filename),
                      filename, bs->backing_file);
-        if (bdrv_open(bs->backing_hd, backing_filename, 0) < 0)
+        if (bdrv_open(bs->backing_hd, backing_filename, flags & (BDRV_O_RDONLY | BDRV_O_RDWR)) < 0)
             goto fail;
     }
 
Index: block.h
===================================================================
--- block.h	(revision 4975)
+++ block.h	(working copy)
@@ -36,9 +36,8 @@
     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_RDONLY      0x0001 /* Open as read-only */
+#define BDRV_O_RDWR        0x0002 /* Open as read-write */
 #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


-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|