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

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

[Daniel P. Berrange]

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 <berrange@redhat.com>

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_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 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, 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 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 = 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,15 +381,20 @@
     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 == -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) {
@@ -416,7 +421,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 :|

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

[Jamie Lokier]

Daniel P. Berrange wrote:
>  - Adds a new option to the '-drive' command line parameter to specify
>    how the file should be opened, and any fallback

I would like to open the main file read-write, but the backing file
read-only, as the backing file is shared among many images.

Can your options choose that?

The monitor 'commit' command implies that the (first) backing file is
opened writable, if the main file is.

-- Jamie

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

[Daniel P. Berrange]

On Thu, Jul 31, 2008 at 01:15:37PM +0100, Jamie Lokier wrote:
> Daniel P. Berrange wrote:
> >  - Adds a new option to the '-drive' command line parameter to specify
> >    how the file should be opened, and any fallback
> 
> I would like to open the main file read-write, but the backing file
> read-only, as the backing file is shared among many images.
> 
> Can your options choose that?

No, you'd need to add further options for that - the existing code
opens the backing files with same mode as the primary files, and my
change maintains that behaviour.

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

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 <berrange@redhat.com>
> 
> 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 <berrange@redhat.com>

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

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

[Paul Brook]

> +#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
> */

This is IMHO really misleading.  Normal O_* are not bitflags. The code uses 
these as bitflags sometimes, which means your descriptions are contradictory.

Paul

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

[Daniel P. Berrange]

On Thu, Jul 31, 2008 at 02:46:55PM +0100, Paul Brook wrote:
> > +#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
> > */
> 
> This is IMHO really misleading.  Normal O_* are not bitflags. The code uses 
> these as bitflags sometimes, which means your descriptions are contradictory.

One alternative approach I considered would be to not have an explicit
flag for writable, and instead have a flag to explicitly indicate that
fallback to read-only shouldn't be attempted.

   #define BDRV_O_RDONLY            0x0001
   #define BDRV_O_NO_RO_FALLBACK    0x0002

This would probably make the patch smaller because I won't need to update
all the callers which assume flags of '0'  gives a writable file, falling
back to RO.

Other suggestions welcome too...

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

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

[Blue Swirl]

On 7/31/08, Daniel P. Berrange <berrange@redhat.com> wrote:
> On Thu, Jul 31, 2008 at 02:46:55PM +0100, Paul Brook wrote:
>  > > +#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
>  > > */
>  >
>  > This is IMHO really misleading.  Normal O_* are not bitflags. The code uses
>  > these as bitflags sometimes, which means your descriptions are contradictory.
>
>
> One alternative approach I considered would be to not have an explicit
>  flag for writable, and instead have a flag to explicitly indicate that
>  fallback to read-only shouldn't be attempted.
>
>
>    #define BDRV_O_RDONLY            0x0001
>
>    #define BDRV_O_NO_RO_FALLBACK    0x0002
>
>  This would probably make the patch smaller because I won't need to update
>  all the callers which assume flags of '0'  gives a writable file, falling
>  back to RO.
>
>  Other suggestions welcome too...

Write-only should mean that only writing is allowed, read access
should not be needed.

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

[Jamie Lokier]

Blue Swirl wrote:
> On 7/31/08, Daniel P. Berrange <berrange@redhat.com> wrote:
> > On Thu, Jul 31, 2008 at 02:46:55PM +0100, Paul Brook wrote:
> >  > > +#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
> >  > > */
> >  >
> >  > This is IMHO really misleading.  Normal O_* are not bitflags. The code uses
> >  > these as bitflags sometimes, which means your descriptions are contradictory.
> >
> >
> > One alternative approach I considered would be to not have an explicit
> >  flag for writable, and instead have a flag to explicitly indicate that
> >  fallback to read-only shouldn't be attempted.
> >
> >
> >    #define BDRV_O_RDONLY            0x0001
> >
> >    #define BDRV_O_NO_RO_FALLBACK    0x0002
> >
> >  This would probably make the patch smaller because I won't need to update
> >  all the callers which assume flags of '0'  gives a writable file, falling
> >  back to RO.
> >
> >  Other suggestions welcome too...
> 
> Write-only should mean that only writing is allowed, read access
> should not be needed.

You can't write to most formats unless you can read the metadata.
Flat is the exception, but WRONLY doesn't seem particularly useful.

Whereas read-only floppy images, for example, resemble real hardware.

I would suggest: read-only, read-write, and try-write (traditional behaviour).
Or maybe get rid of the last one.

-- Jamie

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

[Daniel P. Berrange]

On Thu, Jul 31, 2008 at 05:01:13PM +0100, Jamie Lokier wrote:
> Blue Swirl wrote:
> > On 7/31/08, Daniel P. Berrange <berrange@redhat.com> wrote:
> > > On Thu, Jul 31, 2008 at 02:46:55PM +0100, Paul Brook wrote:
> > >  > > +#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
> > >  > > */
> > >  >
> > >  > This is IMHO really misleading.  Normal O_* are not bitflags. The code uses
> > >  > these as bitflags sometimes, which means your descriptions are contradictory.
> > >
> > >
> > > One alternative approach I considered would be to not have an explicit
> > >  flag for writable, and instead have a flag to explicitly indicate that
> > >  fallback to read-only shouldn't be attempted.
> > >
> > >
> > >    #define BDRV_O_RDONLY            0x0001
> > >
> > >    #define BDRV_O_NO_RO_FALLBACK    0x0002
> > >
> > >  This would probably make the patch smaller because I won't need to update
> > >  all the callers which assume flags of '0'  gives a writable file, falling
> > >  back to RO.
> > >
> > >  Other suggestions welcome too...
> > 
> > Write-only should mean that only writing is allowed, read access
> > should not be needed.
> 
> You can't write to most formats unless you can read the metadata.
> Flat is the exception, but WRONLY doesn't seem particularly useful.
> 
> Whereas read-only floppy images, for example, resemble real hardware.
> 
> I would suggest: read-only, read-write, and try-write (traditional behaviour).

Functionally, that's what the implementation actually does - its just bad
flag naming that's confusing things here.

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

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

[Blue Swirl]

On 7/31/08, Jamie Lokier <jamie@shareable.org> wrote:
> Blue Swirl wrote:
>  > On 7/31/08, Daniel P. Berrange <berrange@redhat.com> wrote:
>  > > On Thu, Jul 31, 2008 at 02:46:55PM +0100, Paul Brook wrote:
>  > >  > > +#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
>  > >  > > */
>  > >  >
>  > >  > This is IMHO really misleading.  Normal O_* are not bitflags. The code uses
>  > >  > these as bitflags sometimes, which means your descriptions are contradictory.
>  > >
>  > >
>  > > One alternative approach I considered would be to not have an explicit
>  > >  flag for writable, and instead have a flag to explicitly indicate that
>  > >  fallback to read-only shouldn't be attempted.
>  > >
>  > >
>  > >    #define BDRV_O_RDONLY            0x0001
>  > >
>  > >    #define BDRV_O_NO_RO_FALLBACK    0x0002
>  > >
>  > >  This would probably make the patch smaller because I won't need to update
>  > >  all the callers which assume flags of '0'  gives a writable file, falling
>  > >  back to RO.
>  > >
>  > >  Other suggestions welcome too...
>  >
>  > Write-only should mean that only writing is allowed, read access
>  > should not be needed.
>
>
> You can't write to most formats unless you can read the metadata.
>  Flat is the exception, but WRONLY doesn't seem particularly useful.

I also could not think of any use cases of write-only, but maybe
write-only+append could be used for system logs. Presenting this
through block device layers could be difficult, maybe a tape device
with non-working seek? Not very useful even then.

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

[Chris Wedgwood]

On Thu, Jul 31, 2008 at 02:46:55PM +0100, Paul Brook wrote:

> > +#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
> > */
> 
> This is IMHO really misleading.  Normal O_* are not bitflags. The code uses 
> these as bitflags sometimes, which means your descriptions are contradictory.

Also the names aren't very intuitive, since BDRV_O_WRONLY really is
read-write not write-only.

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

[Anthony Liguori]

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

I'm not sure I agree that this patch is really that useful to an actual 
user.  I think we'll eventually need a read-only flag as paravirtual 
devices do support read-only block devices.  Let's consider a scenario:

A user has multiple block devices including a secondary device that is 
read-only to the guest.  With qcow2 and today's behavior, savevm will 
just work.  With your patch, it will not work.

This is a scenario where just because the block device cannot be written 
to, we still would want to write to the metadata of the image.

So while I think it's valid to have a "read-only disk" exposed to the 
guest, I don't think the user should have anything to do with how we 
open the file.

Is there some specific circumstance you are trying to support?

Regards,

Anthony Liguori

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

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

[Jamie Lokier]

Anthony Liguori wrote:
> So while I think it's valid to have a "read-only disk" exposed to the 
> guest, I don't think the user should have anything to do with how we 
> open the file.

All good points.

My concern (and it may be different to others) is when I branch an
image using a qcow2 with backing qcow (perhaps more than two deep).

The backing image is sometimes shared with other branches.

E.g. I might have a full install of an OS, and then have a branch
where I install software package A, and another branch with software
package B.  Both share the same base image.

In these, it's very important that I don't accidentally modify the
base image.  E.g. if I foolishly entered the 'commit' monitor command
in VM A, I'd *corrupt* the disk of VM B.

So it would be nice to open the base image read-only.

(Microsoft have a similar description in their documentation on 'disk
image differencing for maintaining test systems' for Virtual PC).

Now savevm snapshots (as opposed to -snapshot) confuse this picture.

Can I arbitrarily branch off different snapshots within a single qcow2
file?  Can I delete the 'base' snapshots safely?  Most important: what
happens if I have snapshots in a qcow2 file which has a base file, and
I type 'commit'?  Does it corrupt all the snapshots, effectively?

I find the snapshot facility a bit unclear as to what _exactly_ it
does to be honest, and am therefore wary of it.

Thanks,
-- Jamie

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

[Anthony Liguori]

Jamie Lokier wrote:
> Anthony Liguori wrote:
>   
>> So while I think it's valid to have a "read-only disk" exposed to the 
>> guest, I don't think the user should have anything to do with how we 
>> open the file.
>>     
>
> All good points.
>
> My concern (and it may be different to others) is when I branch an
> image using a qcow2 with backing qcow (perhaps more than two deep).
>
> The backing image is sometimes shared with other branches.
>
> E.g. I might have a full install of an OS, and then have a branch
> where I install software package A, and another branch with software
> package B.  Both share the same base image.
>
> In these, it's very important that I don't accidentally modify the
> base image.  E.g. if I foolishly entered the 'commit' monitor command
> in VM A, I'd *corrupt* the disk of VM B.
>   

Introducing an option to keep you from misusing another option is a 
little silly.  You're just as likely to forget to use the first option 
as you are to "accidentally" issue a commit.

If you want to protect yourself, chmod the base image.  Then you have 
nothing to worry about.

> So it would be nice to open the base image read-only.
>
> (Microsoft have a similar description in their documentation on 'disk
> image differencing for maintaining test systems' for Virtual PC).
>
> Now savevm snapshots (as opposed to -snapshot) confuse this picture.
>
> Can I arbitrarily branch off different snapshots within a single qcow2
> file?  Can I delete the 'base' snapshots safely?  Most important: what
> happens if I have snapshots in a qcow2 file which has a base file, and
> I type 'commit'?  Does it corrupt all the snapshots, effectively?
>   

snapshots are linear, not branching.  And they are only every 
saved/loaded from the highest level qcow2 file.

Regards,

Anthony Liguori

> I find the snapshot facility a bit unclear as to what _exactly_ it
> does to be honest, and am therefore wary of it.
>
> Thanks,
> -- Jamie
>
>
>   

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

[Jamie Lokier]

Anthony Liguori wrote:
> >Can I arbitrarily branch off different snapshots within a single qcow2
> >file?  Can I delete the 'base' snapshots safely?  Most important: what
> >happens if I have snapshots in a qcow2 file which has a base file, and
> >I type 'commit'?  Does it corrupt all the snapshots, effectively?
> 
> snapshots are linear, not branching.  And they are only every 
> saved/loaded from the highest level qcow2 file.

So if I understand that right, the 'commit' command (to write to the
base image) _will_ corrupt all the snapshots since they depend on the
base image and it just changed??

-- Jamie

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

[Anthony Liguori]

Jamie Lokier wrote:
> Anthony Liguori wrote:
>   
>>> Can I arbitrarily branch off different snapshots within a single qcow2
>>> file?  Can I delete the 'base' snapshots safely?  Most important: what
>>> happens if I have snapshots in a qcow2 file which has a base file, and
>>> I type 'commit'?  Does it corrupt all the snapshots, effectively?
>>>       
>> snapshots are linear, not branching.  And they are only every 
>> saved/loaded from the highest level qcow2 file.
>>     
>
> So if I understand that right, the 'commit' command (to write to the
> base image) _will_ corrupt all the snapshots since they depend on the
> base image and it just changed??
>   

Yes, so don't do that.

BTW, commit is a pretty useless command IMHO.  If you use it to save a 
snapshot of a disk image while the OS has it mounted, you're not getting 
a very good backup.

Regards,

Anthony Liguori

Regards,

Anthony Liguori

> -- Jamie
>
>
>   

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

[Daniel P. Berrange]

On Thu, Jul 31, 2008 at 01:26:17PM -0500, Anthony Liguori wrote:
> 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.
> >  
> 
> I'm not sure I agree that this patch is really that useful to an actual 
> user.  I think we'll eventually need a read-only flag as paravirtual 
> devices do support read-only block devices.  Let's consider a scenario:
> 
> A user has multiple block devices including a secondary device that is 
> read-only to the guest.  With qcow2 and today's behavior, savevm will 
> just work.  With your patch, it will not work.
> 
> This is a scenario where just because the block device cannot be written 
> to, we still would want to write to the metadata of the image.

Sure, the admin of the guest has the option to make it read only or not
depending on whether they need to use this capability.

> So while I think it's valid to have a "read-only disk" exposed to the 
> guest, I don't think the user should have anything to do with how we 
> open the file.
> 
> Is there some specific circumstance you are trying to support?

The scenario is that the admin wants to assign a read only disk to the
virtual machine - typically the same disk to multiple machines - and
thus want to guarentee that no one VM can write to it, since bad things
happen if you do that with non-cluster filesystems.

Controlling this based on the underlying permissions of the file backing
the drive is not practical. Things like udev happy set permissions on
devices in /dev/ behind your back, so you'd have to edit the horrible udev
config files to make /dev/sdXX readonly.  It is a far simpler task to
simply add  ,mode=ro  to the QEMU command line for -drive to accomplish
this, than finding the obscure file to edit to make the underling file
have read only permissions

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

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

[Anthony Liguori]

Daniel P. Berrange wrote:
> On Thu, Jul 31, 2008 at 01:26:17PM -0500, Anthony Liguori wrote:
>   
>> So while I think it's valid to have a "read-only disk" exposed to the 
>> guest, I don't think the user should have anything to do with how we 
>> open the file.
>>
>> Is there some specific circumstance you are trying to support?
>>     
>
> The scenario is that the admin wants to assign a read only disk to the
> virtual machine - typically the same disk to multiple machines - and
> thus want to guarentee that no one VM can write to it, since bad things
> happen if you do that with non-cluster filesystems.
>
> Controlling this based on the underlying permissions of the file backing
> the drive is not practical. Things like udev happy set permissions on
> devices in /dev/ behind your back, so you'd have to edit the horrible udev
> config files to make /dev/sdXX readonly.  It is a far simpler task to
> simply add  ,mode=ro  to the QEMU command line for -drive to accomplish
> this, than finding the obscure file to edit to make the underling file
> have read only permissions
>   

Right, but my point is that ,mode=ro does not have to force QEMU to open 
the file O_RDONLY.  It simply needs to prevent writes from happening.

But it's important to be able to expose this property to the guest, so 
,mode=ro should not be allowed for disks that do not support exposing 
their read-only-ness to the guest.

Regards,

Anthony Liguori

> Daniel
>   

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

[Ian Jackson]

Anthony Liguori writes ("Re: [Qemu-devel] PATCH: Control over drive open modes for backing file"):
> Right, but my point is that ,mode=ro does not have to force QEMU to open 
> the file O_RDONLY.  It simply needs to prevent writes from happening.

Well, yes, but actually it's probably most reliable to do it that way.
Given that this is a security feature we want to avoid accidentally
`missing' a case.  So we should definitely open the underlying file(s)
O_RDONLY.

If we do that then the guest definitely won't be able to write as if
it manages to persuade qemu to try qemu will just get an error.  This
is fine I think, if we can expose the read-only status to the guest.

> But it's important to be able to expose this property to the guest, so 
> ,mode=ro should not be allowed for disks that do not support exposing 
> their read-only-ness to the guest.

I agree that it would be an unusual thing to do, to expose a ro disk
in a way that doesn't support advertising the ro flag.  But I think it
should still be possible perhaps with some kind of force option.

Ian.

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

[Anthony Liguori]

Ian Jackson wrote:
> Anthony Liguori writes ("Re: [Qemu-devel] PATCH: Control over drive open modes for backing file"):
>   
>> Right, but my point is that ,mode=ro does not have to force QEMU to open 
>> the file O_RDONLY.  It simply needs to prevent writes from happening.
>>     
>
> Well, yes, but actually it's probably most reliable to do it that way.
> Given that this is a security feature we want to avoid accidentally
> `missing' a case.  So we should definitely open the underlying file(s)
> O_RDONLY.
>   

That is an implementation detail, and should be dependent on the 
underlying file format.  For instance, I completely agree that this 
should be the behaviour for raw images.  I don't agree that this should 
be the behaviour with qcow2 though.

FWIW, this isn't a major security improvement.  It's pretty darn easy to 
audit the single calling location where a file can be written to :-)

> If we do that then the guest definitely won't be able to write as if
> it manages to persuade qemu to try qemu will just get an error.  This
> is fine I think, if we can expose the read-only status to the guest.
>
>   
>> But it's important to be able to expose this property to the guest, so 
>> ,mode=ro should not be allowed for disks that do not support exposing 
>> their read-only-ness to the guest.
>>     
>
> I agree that it would be an unusual thing to do, to expose a ro disk
> in a way that doesn't support advertising the ro flag.  But I think it
> should still be possible perhaps with some kind of force option.
>   

There's no good reason to do this and it's just going to result in 
confusing users.  You can approximate the same functionality by using 
,snapshot=on while giving the guest predictable behaviour.

Regards,

Anthony Liguori

> Ian.
>
>
>   

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

[Jamie Lokier]

Ian Jackson wrote:
> > Right, but my point is that ,mode=ro does not have to force QEMU to open 
> > the file O_RDONLY.  It simply needs to prevent writes from happening.
> 
> Well, yes, but actually it's probably most reliable to do it that way.
> Given that this is a security feature we want to avoid accidentally
> `missing' a case.  So we should definitely open the underlying file(s)
> O_RDONLY.

Also, the point about qcow2 metadata.  If you have multiple QEMU
instances using the same qcow2 image, it is not acceptable to have
them both trying to write qcow2 metadata - unless they are interlocked
in some way (as fas as I know they're not).

Personally I use 'chattr +i' on such files.  But it's not very
userspace friendy, e.g. for VM management scripts run by other people.
You need root access to do 'chattr +i'.

> If we do that then the guest definitely won't be able to write as if
> it manages to persuade qemu to try qemu will just get an error.  This
> is fine I think, if we can expose the read-only status to the guest.
> 
> > But it's important to be able to expose this property to the guest, so 
> > ,mode=ro should not be allowed for disks that do not support exposing 
> > their read-only-ness to the guest.
> 
> I agree that it would be an unusual thing to do, to expose a ro disk
> in a way that doesn't support advertising the ro flag.  But I think it
> should still be possible perhaps with some kind of force option.

I would think for environments where a single image is launched many
times, e.g. 'start OS foo and tell it to run my test program', it's
important that the *file* be open read-only regardless of what the
guest can see.

In that usage, -snapshot or creating an explicit qcow2 derived file
would be appropriate.  But it's still important that the base file is
opened read-only, and that 'commit' to it is forbidden.

-- Jamie