Re: [Qemu-devel] PATCH: Control over drive open modes for backing file

[Daniel P. Berrange]

On Thu, Jul 31, 2008 at 12:31:20PM +0100, Daniel P. Berrange wrote:
> 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 useless as is.
> 
> So this patch does a couple of things:
> 
>  - Change the access flags to be
> 
>    #define BDRV_O_RDONLY      0x0001 /* Force read-only */
>    #define BDRV_O_WRONLY      0x0002 /* Force writeable, no fallback */
>    #define BDRV_O_RDWR        0x0003 /* Try writeable, fallback to read-only 
> */
> 
>  - Change all callers of bdrv_open/open2/file_open to pass the correct
>    access flags they desire instead of 0.
> 
>  - 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 first, and fallback to read-only
>               if getting EACCESS. This is the default, and matches
>               current behaviour.
>    mode=wo  - try to open read-write and exit if this fails.
>    mode=ro  - try to open read-only and exit if this fails
> 
>  - 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 <address@hidden>
> 
> 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.

There was a minor bug in the previous version - I opened readonly,
but failed to set the internal read_only flag on the block device
so 'info block' reported wrong data. This corrects that flaw:

   Signed-off-by: Daniel P. Berrange <address@hidden>

Regards,
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_WRONLY) {
         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_WRONLY) {
         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 = BDRV_O_RDWR;  /* 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, "wo") == 0)
+            mode = BDRV_O_WRONLY;
+    }
+
+
     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|wo|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_WRONLY) {
         open_flags |= O_RDWR;
     } else {
         open_flags |= O_RDONLY;
@@ -896,7 +896,7 @@
     }
 #endif
     open_flags = O_BINARY;
-    if ((flags & BDRV_O_ACCESS) == O_RDWR) {
+    if (flags & BDRV_O_WRONLY) {
         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,11 @@
     if (bs == NULL)
         return 1;
 
+    if (readonly)
+        flags |= BDRV_O_RDONLY;
+    else
+        flags |= BDRV_O_RDWR;
+
     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 address@hidden
 @var{cache} is "on" or "off" and allows to disable host cache to access data.
address@hidden address@hidden
address@hidden is "rw" to open the file read-write, falling back to read-only 
on failure,
+"wo" to open the file read-write, with no fallback or "ro" to open the file 
read-only.
+The default is "rw".
 @item address@hidden
 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 = BDRV_O_RDWR;
 
+    if (mode) {
+        if (strcmp(mode, "ro") == 0)
+            flags = BDRV_O_RDONLY;
+        else if (strcmp(mode, "wo") == 0)
+            flags = BDRV_O_WRONLY;
+    }
+
     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)
@@ -348,7 +348,7 @@
         if (!bs1) {
             return -ENOMEM;
         }
-        if (bdrv_open(bs1, filename, 0) < 0) {
+        if (bdrv_open(bs1, filename, BDRV_O_RDWR) < 0) {
             bdrv_delete(bs1);
             return -1;
         }
@@ -381,16 +381,23 @@
     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 O_WRONLY and O_RDONLY are set, try read-write and fallback
+     * to readonly
+     * If only O_WRONLY is set, then try read-write with no fallback
+     * If only O_RDONLY is set, then try read-only with no fallback
+     */
     if (!(flags & BDRV_O_FILE))
-        open_flags = BDRV_O_RDWR | (flags & BDRV_O_DIRECT);
+        open_flags = (flags & (BDRV_O_DIRECT | 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);
+    if (ret == 0 && ((flags & BDRV_O_RDWR) == BDRV_O_RDONLY))
         bs->read_only = 1;
+    if (ret == -EACCES && !(flags & BDRV_O_FILE) &&
+       ((flags & BDRV_O_RDWR) == BDRV_O_RDWR)) {
+        open_flags = (flags & (BDRV_O_DIRECT | BDRV_O_RDONLY));
+        ret = drv->bdrv_open(bs, filename, open_flags);
+        bs->read_only = 1;
     }
     if (ret < 0) {
         qemu_free(bs->opaque);
@@ -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_RDWR) < 
0)
             goto fail;
     }
 
Index: block.h
===================================================================
--- block.h     (revision 4975)
+++ block.h     (working copy)
@@ -36,9 +36,9 @@
     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 /* Force read-only */
+#define BDRV_O_WRONLY      0x0002 /* Force writeable, no fallback */
+#define BDRV_O_RDWR        0x0003 /* Try writeable, fallback to read-only */
 #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
Index: hw/usb-msd.c
===================================================================
--- hw/usb-msd.c        (revision 4975)
+++ hw/usb-msd.c        (working copy)
@@ -523,7 +523,7 @@
         return NULL;
 
     bdrv = bdrv_new("usb");
-    if (bdrv_open(bdrv, filename, 0) < 0)
+    if (bdrv_open(bdrv, filename, BDRV_O_RDWR) < 0)
         goto fail;
     if (qemu_key_check(bdrv, filename))
         goto fail;
Index: qemu-img.c
===================================================================
--- qemu-img.c  (revision 4975)
+++ qemu-img.c  (working copy)
@@ -186,7 +186,7 @@
     } else {
         drv = NULL;
     }
-    if (bdrv_open2(bs, filename, 0, drv) < 0) {
+    if (bdrv_open2(bs, filename, BDRV_O_RDWR, drv) < 0) {
         error("Could not open '%s'", filename);
     }
     if (bdrv_is_encrypted(bs)) {
@@ -317,7 +317,7 @@
     } else {
         drv = NULL;
     }
-    if (bdrv_open2(bs, filename, 0, drv) < 0) {
+    if (bdrv_open2(bs, filename, BDRV_O_RDWR, drv) < 0) {
         error("Could not open '%s'", filename);
     }
     ret = bdrv_commit(bs);
@@ -691,7 +691,7 @@
     } else {
         drv = NULL;
     }
-    if (bdrv_open2(bs, filename, 0, drv) < 0) {
+    if (bdrv_open2(bs, filename, BDRV_O_RDWR, drv) < 0) {
         error("Could not open '%s'", filename);
     }
     bdrv_get_format(bs, fmt_name, sizeof(fmt_name));



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