Patchwork Add readonly flag to -drive command

login
register
mail settings
Submitter Naphtali Sprei
Date Oct. 12, 2009, 12:47 p.m.
Message ID <4AD32550.8040901@redhat.com>
Download mbox | patch
Permalink /patch/35758/
State Rejected
Headers show

Comments

Naphtali Sprei - Oct. 12, 2009, 12:47 p.m.
In order to safely share an image between guests (as read only drive), add a 'readonly' flag
to the -drive command (qemu command line and monitor).

Still missing passing the read only attribute to the guest, where possible. I don't know which device types supports
read only, and don't know how to pass this information to guests.

Also not sure what to do when qemu cannot open the file as writeable. Currently it opens it as read only.
We might change it to give a warning or even an error.

From 6e297da79a4c015555e3927e6d28744933a31ebe Mon Sep 17 00:00:00 2001
From: Naphtali Sprei <nsprei@redhat.com>
Date: Mon, 12 Oct 2009 14:25:25 +0200
Subject: [PATCH] Added readonly flag to -drive command.
 This enables sharing same image between guests, with readonly access.
 Implementaion mark the drive as read_only and changes the flags when actually opening the file.

TODO:
1. Pass the readonly attribute to the guest (write-protected drive ??)
2. Re-visit the scheme where qemu open a file (silently) in read only mode when it can't open for write.
   Now that user can specify read only (and didn't), might give a warning when not writeable, or even
   give an error.

Signed-off-by: Naphtali Sprei <nsprei@redhat.com>
---
 block.c       |   14 ++++++++++++--
 block.h       |    1 +
 qemu-config.c |    3 +++
 vl.c          |    6 ++++++
 4 files changed, 22 insertions(+), 2 deletions(-)
Kevin Wolf - Oct. 12, 2009, 1:24 p.m.
Am 12.10.2009 14:47, schrieb Naphtali Sprei:
> In order to safely share an image between guests (as read only drive), add a 'readonly' flag
> to the -drive command (qemu command line and monitor).
> 
> Still missing passing the read only attribute to the guest, where possible. I don't know which device types supports
> read only, and don't know how to pass this information to guests.
> 
> Also not sure what to do when qemu cannot open the file as writeable. Currently it opens it as read only.
> We might change it to give a warning or even an error.
> 
> From 6e297da79a4c015555e3927e6d28744933a31ebe Mon Sep 17 00:00:00 2001
> From: Naphtali Sprei <nsprei@redhat.com>
> Date: Mon, 12 Oct 2009 14:25:25 +0200
> Subject: [PATCH] Added readonly flag to -drive command.
>  This enables sharing same image between guests, with readonly access.
>  Implementaion mark the drive as read_only and changes the flags when actually opening the file.

Is this enough? Basically none of the block drivers know that their
image could be read-only, so we'll likely trigger some unexpected error
cases there. For a simple write I guess we'll be okay (not sure if we'll
return the right error code, though), but I have no idea what, say,
savevm would do with a read-only image.

What cases have you tested?

> TODO:
> 1. Pass the readonly attribute to the guest (write-protected drive ??)

I agree. To be useful the read-only attribute should be exposed to the
guest. I think most devices support some sort of write protection.

> 2. Re-visit the scheme where qemu open a file (silently) in read only mode when it can't open for write.
>    Now that user can specify read only (and didn't), might give a warning when not writeable, or even
>    give an error.
> 
> Signed-off-by: Naphtali Sprei <nsprei@redhat.com>
> ---
>  block.c       |   14 ++++++++++++--
>  block.h       |    1 +
>  qemu-config.c |    3 +++
>  vl.c          |    6 ++++++
>  4 files changed, 22 insertions(+), 2 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 33f3d65..01fd289 100644
> --- a/block.c
> +++ b/block.c
> @@ -335,7 +335,8 @@ int bdrv_open2(BlockDriverState *bs, const char *filename, int flags,
>      char tmp_filename[PATH_MAX];
>      char backing_filename[PATH_MAX];
>  
> -    bs->read_only = 0;
> +    /* don't mess with it, should have been zeros, anyway */
> +    /* bs->read_only = 0; */

Why leave that comment instead of just removing it if it's not necessary?

Kevin
Jamie Lokier - Oct. 12, 2009, 1:50 p.m.
Kevin Wolf wrote:
> Am 12.10.2009 14:47, schrieb Naphtali Sprei:
> > In order to safely share an image between guests (as read only drive), add a 'readonly' flag
> > to the -drive command (qemu command line and monitor).

Heh.  I've been sharing images between guests for ages - using "chmod -r" :-)

> > Still missing passing the read only attribute to the guest, where possible. I don't know which device types supports
> > read only, and don't know how to pass this information to guests.

This was discussed in a thread some months ago.  You might look it up.
SCSI, USB, and floppy can pass the information to guests - see the
Linux kernel for how the flag is read.  CD-ROMs are read-only already
of course. I don't know if virtio-blk can; if not, it would be good to
add it.  Not sure about the latest iterations of PATA/SATA.

Read-only images are particularly useful for the backing file for a
qcow2 image or -snapshot.  Then the guest doesn't need to know
anything, but 'savevm', 'delvm' and 'commit' need to be stopped from
trying to modify the backing file.  (But it's fine for -snapshot +
commit to modify a writable qcow2 file which _itself_ has a read-only
backing file, as long as 'commit' doesn't try to push the commit
further back than one level).

> > Also not sure what to do when qemu cannot open the file as
> > writeable. Currently it opens it as read only.  We might change it
> > to give a warning or even an error.

I think that's a good plan, but please don't break the current
situation, where it's possible to "chmod -r" an image file and then
share it safely, until the 'readonly' flag is a usable replacement.

> Is this enough? Basically none of the block drivers know that their
> image could be read-only, so we'll likely trigger some unexpected error
> cases there. For a simple write I guess we'll be okay (not sure if we'll
> return the right error code, though), but I have no idea what, say,
> savevm would do with a read-only image.

Right now there is already a 'readonly' flag called 'chmod -r image',
because QEMU opens a file read-only if it can't open it writable, so
it's not a new case.  Just moving it from the filesystem into QEMU.

When opened read-only, it would be better for the block drivers to
return an error themselves, instead of trying to write and (hopefully)
getting a host OS error.

I doubt if read-only errors have been tested or accomodated, or if
they are reported well to guests as the right sort of error.  I'm
pretty sure the read-only _flag_ isn't passed through guest interfaces
for the guest OS to use yet, but that should be quite easy.

-- Jamie
Anthony Liguori - Oct. 12, 2009, 2:06 p.m.
Kevin Wolf wrote:
> Am 12.10.2009 14:47, schrieb Naphtali Sprei:
>   
>> In order to safely share an image between guests (as read only drive), add a 'readonly' flag
>> to the -drive command (qemu command line and monitor).
>>
>> Still missing passing the read only attribute to the guest, where possible. I don't know which device types supports
>> read only, and don't know how to pass this information to guests.
>>
>> Also not sure what to do when qemu cannot open the file as writeable. Currently it opens it as read only.
>> We might change it to give a warning or even an error.
>>
>> From 6e297da79a4c015555e3927e6d28744933a31ebe Mon Sep 17 00:00:00 2001
>> From: Naphtali Sprei <nsprei@redhat.com>
>> Date: Mon, 12 Oct 2009 14:25:25 +0200
>> Subject: [PATCH] Added readonly flag to -drive command.
>>  This enables sharing same image between guests, with readonly access.
>>  Implementaion mark the drive as read_only and changes the flags when actually opening the file.
>>     
>
> Is this enough? Basically none of the block drivers know that their
> image could be read-only, so we'll likely trigger some unexpected error
> cases there. For a simple write I guess we'll be okay (not sure if we'll
> return the right error code, though), but I have no idea what, say,
> savevm would do with a read-only image.
>   

Yes, this is why patches like this have been rejected in the past.  This 
concept needs to be plumbed to the device emulation so that a guest 
realizes it has a read-only disk.  Throwing away writes without telling 
the guest is a recipe for disaster.

Regards,

Anthony Liguori
Kevin Wolf - Oct. 12, 2009, 2:07 p.m.
Am 12.10.2009 15:50, schrieb Jamie Lokier:
> Right now there is already a 'readonly' flag called 'chmod -r image',
> because QEMU opens a file read-only if it can't open it writable, so
> it's not a new case.  Just moving it from the filesystem into QEMU.

Right, but I'm not sure how well it is tested. I think, when moving it
into qemu we should take the opportunity to do it right. If everything
works right now, the right description of it is probably "pure luck".
And I assume you didn't do stupid things like savevm on a read-only
image which we still should be able to handle.

> When opened read-only, it would be better for the block drivers to
> return an error themselves, instead of trying to write and (hopefully)
> getting a host OS error.

For simple read/write operations, the generic block layer is doing the
check already.

Kevin
Anthony Liguori - Oct. 12, 2009, 2:30 p.m.
Jamie Lokier wrote:
> Kevin Wolf wrote:
>   
>> Am 12.10.2009 14:47, schrieb Naphtali Sprei:
>>     
>>> In order to safely share an image between guests (as read only drive), add a 'readonly' flag
>>> to the -drive command (qemu command line and monitor).
>>>       
>
> Heh.  I've been sharing images between guests for ages - using "chmod -r" :-)
>   

Were it not for backwards compatibility, I would remove that and have it 
error out.

I've had multiple people end up with strangely broken guests because 
they didn't realize the image was read only.

>>> Still missing passing the read only attribute to the guest, where possible. I don't know which device types supports
>>> read only, and don't know how to pass this information to guests.
>>>       
>
> This was discussed in a thread some months ago.  You might look it up.
> SCSI, USB, and floppy can pass the information to guests - see the
> Linux kernel for how the flag is read.  CD-ROMs are read-only already
> of course. I don't know if virtio-blk can; if not, it would be good to
> add it.
>   

virtio-blk can support read-only images now.

Regards,

Anthony Liguori
Jamie Lokier - Oct. 12, 2009, 3:16 p.m.
Anthony Liguori wrote:
> Jamie Lokier wrote:
> >Kevin Wolf wrote:
> >  
> >>Am 12.10.2009 14:47, schrieb Naphtali Sprei:
> >>    
> >>>In order to safely share an image between guests (as read only drive), 
> >>>add a 'readonly' flag
> >>>to the -drive command (qemu command line and monitor).
> >>>      
> >
> >Heh.  I've been sharing images between guests for ages - using "chmod -r" 
> >:-)
> >  
> 
> Were it not for backwards compatibility, I would remove that and have it 
> error out.
> 
> I've had multiple people end up with strangely broken guests because 
> they didn't realize the image was read only.

I agree, that's ugly - I've wasted time on a strangely broken guest in
that situation too!

Backward compatibility might be an issue with the idea to change QEMU
to complain if it can't open a file writable.  Not with regular disk
images so much, but floppy/USB images.  Perhaps the answer to that is
'readonly=auto', 'readonly=yes', 'readonly=no'?

Sharing images with -snapshot, and backing images with qcow2, is a
very useful feature for launching multiple test VMs in parallel, and
for making "forked" images for testing different guest OS variations,
so I'm glad to see the 'readonly' flag appearing as an option.

(You can't use qcow2 snapshots to implement "forked" images running in
parallel because you can't safely open different snapshots in the same
qcow2 file at the same time).

Taking a read-only (shared) or writeable (exclusive) lock on the file
would be a useful addition I think, to catch mistakes.

-- Jamie
Michael Tokarev - Oct. 12, 2009, 4:15 p.m.
Anthony Liguori wrote:
> Jamie Lokier wrote:
>> Kevin Wolf wrote:
>>  
>>> Am 12.10.2009 14:47, schrieb Naphtali Sprei:
>>>    
>>>> In order to safely share an image between guests (as read only 
>>>> drive), add a 'readonly' flag
>>>> to the -drive command (qemu command line and monitor).
>>
>> Heh.  I've been sharing images between guests for ages - using "chmod -r" :-)
> 
> Were it not for backwards compatibility, I would remove that and have it 
> error out.
> 
> I've had multiple people end up with strangely broken guests because 
> they didn't realize the image was read only.

And I had this issue too.

But for me it's still not clear.  Some disk drives out there can be
switched to read-only mode at runtime by using appropriate command.
Certain (USB and especially SD) flash drives can be read-only too
(usually a tiny swith does that).  Even old good floppy drives had
a "write protect" switch.  How the system determines if such devices
are read-only?  For a HDD or a flash drive, I *guess* the drive return
appropriate error message, no?

But qemu - apparently - just ignores writes in this case, which
results in stalled guest which expects some answer to the write
command..  I guess, because it was always the stalls.

/mjt
Kevin Wolf - Oct. 13, 2009, 7:36 a.m.
Am 12.10.2009 18:15, schrieb Michael Tokarev:
> Anthony Liguori wrote:
>> Jamie Lokier wrote:
>>> Kevin Wolf wrote:
>>>  
>>>> Am 12.10.2009 14:47, schrieb Naphtali Sprei:
>>>>    
>>>>> In order to safely share an image between guests (as read only 
>>>>> drive), add a 'readonly' flag
>>>>> to the -drive command (qemu command line and monitor).
>>>
>>> Heh.  I've been sharing images between guests for ages - using "chmod -r" :-)
>>
>> Were it not for backwards compatibility, I would remove that and have it 
>> error out.
>>
>> I've had multiple people end up with strangely broken guests because 
>> they didn't realize the image was read only.
> 
> And I had this issue too.
> 
> But for me it's still not clear.  Some disk drives out there can be
> switched to read-only mode at runtime by using appropriate command.
> Certain (USB and especially SD) flash drives can be read-only too
> (usually a tiny swith does that).  Even old good floppy drives had
> a "write protect" switch.  How the system determines if such devices
> are read-only?  For a HDD or a flash drive, I *guess* the drive return
> appropriate error message, no?
> 
> But qemu - apparently - just ignores writes in this case, which
> results in stalled guest which expects some answer to the write
> command..  I guess, because it was always the stalls.

The qemu block layer returns errors. For good old bdrv_write it's
-EACCES, for bdrv_aio_writev it's NULL (and therefore cannot be
distinguished from other errors). Not sure what the devices make out of
it. Probably not the right thing in all cases.

Kevin

Patch

diff --git a/block.c b/block.c
index 33f3d65..01fd289 100644
--- a/block.c
+++ b/block.c
@@ -335,7 +335,8 @@  int bdrv_open2(BlockDriverState *bs, const char *filename, int flags,
     char tmp_filename[PATH_MAX];
     char backing_filename[PATH_MAX];
 
-    bs->read_only = 0;
+    /* don't mess with it, should have been zeros, anyway */
+    /* bs->read_only = 0; */
     bs->is_temporary = 0;
     bs->encrypted = 0;
     bs->valid_key = 0;
@@ -423,7 +424,7 @@  int bdrv_open2(BlockDriverState *bs, const char *filename, int flags,
     /* Note: for compatibility, we open disk image files as RDWR, and
        RDONLY as fallback */
     if (!(flags & BDRV_O_FILE))
-        open_flags = BDRV_O_RDWR |
+        open_flags = (bs->read_only ? 0 : BDRV_O_RDWR) |
 		(flags & (BDRV_O_CACHE_MASK|BDRV_O_NATIVE_AIO));
     else
         open_flags = flags & ~(BDRV_O_FILE | BDRV_O_SNAPSHOT);
@@ -453,6 +454,8 @@  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')
@@ -925,6 +928,13 @@  int bdrv_is_read_only(BlockDriverState *bs)
     return bs->read_only;
 }
 
+int bdrv_set_read_only(BlockDriverState *bs, int read_only)
+{
+    int ret = bs->read_only;
+    bs->read_only = read_only;
+    return ret;
+}
+
 int bdrv_is_sg(BlockDriverState *bs)
 {
     return bs->sg;
diff --git a/block.h b/block.h
index a966afb..302010d 100644
--- a/block.h
+++ b/block.h
@@ -136,6 +136,7 @@  int bdrv_get_type_hint(BlockDriverState *bs);
 int bdrv_get_translation_hint(BlockDriverState *bs);
 int bdrv_is_removable(BlockDriverState *bs);
 int bdrv_is_read_only(BlockDriverState *bs);
+int bdrv_set_read_only(BlockDriverState *bs, int read_only);
 int bdrv_is_sg(BlockDriverState *bs);
 int bdrv_enable_write_cache(BlockDriverState *bs);
 int bdrv_is_inserted(BlockDriverState *bs);
diff --git a/qemu-config.c b/qemu-config.c
index bafaea2..5eb8838 100644
--- a/qemu-config.c
+++ b/qemu-config.c
@@ -71,6 +71,9 @@  QemuOptsList qemu_drive_opts = {
             .name = "addr",
             .type = QEMU_OPT_STRING,
             .help = "pci address (virtio only)",
+        },{
+            .name = "readonly",
+            .type = QEMU_OPT_BOOL,
         },
         { /* end if list */ }
     },
diff --git a/vl.c b/vl.c
index 374f85b..3763911 100644
--- a/vl.c
+++ b/vl.c
@@ -2007,6 +2007,7 @@  DriveInfo *drive_init(QemuOpts *opts, void *opaque,
     int index;
     int cache;
     int aio = 0;
+    int ro = 0;
     int bdrv_flags, onerror;
     const char *devaddr;
     DriveInfo *dinfo;
@@ -2038,6 +2039,7 @@  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");
@@ -2329,6 +2331,10 @@  DriveInfo *drive_init(QemuOpts *opts, void *opaque,
         bdrv_flags &= ~BDRV_O_NATIVE_AIO;
     }
 
+    if (ro == 1) {
+        (void)bdrv_set_read_only(dinfo->bdrv, 1);
+    }
+
     if (bdrv_open2(dinfo->bdrv, file, bdrv_flags, drv) < 0) {
         fprintf(stderr, "qemu: could not open disk image %s: %s\n",
                         file, strerror(errno));