[Qemu-devel] [PATCH] block: Enable fall-back to read-only for backing file

[Qemu-devel] [PATCH] block: Enable fall-back to read-only for backing file

[Naphtali Sprei]

There's a problem when trying to use an image file based on a read-only image file.
Before this patch, qemu fails to open the base image and stop.
With this patch, qemu tries to open the backing file with same permissions as the "top" file,
but if it fails, qemu tries to open it with read-only permissions. If succeeded it goes on.

This fall-back works both for an image file based on a read-only file
and also for a read-only file opened with the snapshot attribute/mode (where the real file is the backing file
for the snapshot file).

Is it better to always open the backing file with read-only mode ? this will be more consistent/predictable ?
Or is it better not to fall-back to read-only ? Will a warning message help ?

 TIA,

   Naphtali
   

>From 4a10750f5c91b1383118e4421f6b8d3ff3e79b2f Mon Sep 17 00:00:00 2001
From: Naphtali Sprei <nsprei@redhat.com>
Date: Sun, 31 Jan 2010 18:23:44 +0200
Subject: [PATCH] block: Enable fall-back to read-only for backing file

In order to use an image file backed by a read-only file, allow opening
the backing file with read-only permission.

Signed-off-by: Naphtali Sprei <nsprei@redhat.com>
---
 block.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/block.c b/block.c
index 1919d19..d1b0f3d 100644
--- a/block.c
+++ b/block.c
@@ -483,6 +483,11 @@ int bdrv_open2(BlockDriverState *bs, const char *filename, int flags,
             back_drv = bdrv_find_format(bs->backing_format);
         ret = bdrv_open2(bs->backing_hd, backing_filename, open_flags,
                          back_drv);
+        if (ret < 0) {
+            open_flags &= ~BDRV_O_RDWR;  /* Fall-back to read-only for the backing file */
+            ret = bdrv_open2(bs->backing_hd, backing_filename, open_flags,
+                             back_drv);
+        }
         bs->backing_hd->read_only =  (open_flags & BDRV_O_RDWR) == 0;
         if (ret < 0) {
             bdrv_close(bs);
-- 
1.6.3.3

[Qemu-devel] Re: [PATCH] block: Enable fall-back to read-only for backing file

[Kevin Wolf]

Am 31.01.2010 17:49, schrieb Naphtali Sprei:
> There's a problem when trying to use an image file based on a read-only image file.
> Before this patch, qemu fails to open the base image and stop.
> With this patch, qemu tries to open the backing file with same permissions as the "top" file,
> but if it fails, qemu tries to open it with read-only permissions. If succeeded it goes on.
> 
> This fall-back works both for an image file based on a read-only file
> and also for a read-only file opened with the snapshot attribute/mode (where the real file is the backing file
> for the snapshot file).
> 
> Is it better to always open the backing file with read-only mode ? this will be more consistent/predictable ?

I would love to open them read-only unconditionally, but we can't. It
would break the commit monitor command. I think the read-only fallback
is appropriate for backing files.

Kevin

Re: [Qemu-devel] Re: [PATCH] block: Enable fall-back to read-only for backing file

[Christoph Hellwig]

On Mon, Feb 01, 2010 at 09:46:00AM +0100, Kevin Wolf wrote:
> > Is it better to always open the backing file with read-only mode ? this will be more consistent/predictable ?
> 
> I would love to open them read-only unconditionally, but we can't. It
> would break the commit monitor command. I think the read-only fallback
> is appropriate for backing files.

The clean way would be to require an explicit BACKING_WRITEABLE flag
if we want to commit to it.  But I suspect it's too late to refit that
now.

Re: [Qemu-devel] Re: [PATCH] block: Enable fall-back to read-only for backing file

[Alexander Graf]

Am 01.02.2010 um 10:06 schrieb Christoph Hellwig <hch@lst.de>:

> On Mon, Feb 01, 2010 at 09:46:00AM +0100, Kevin Wolf wrote:
>>> Is it better to always open the backing file with read-only mode ?  
>>> this will be more consistent/predictable ?
>>
>> I would love to open them read-only unconditionally, but we can't. It
>> would break the commit monitor command. I think the read-only  
>> fallback
>> is appropriate for backing files.
>
> The clean way would be to require an explicit BACKING_WRITEABLE flag
> if we want to commit to it.  But I suspect it's too late to refit that
> now.

We don't know beforehand if a user will use the commit comnand during  
tje runtime of the vm.

IMHO it'd be best to always open backing files read only and try to  
open them for write access on the commit command. That command can  
then fail gaciously.

Alex
>

Re: [Qemu-devel] Re: [PATCH] block: Enable fall-back to read-only for backing file

[Christoph Hellwig]

On Mon, Feb 01, 2010 at 10:25:52AM +0100, Alexander Graf wrote:
> We don't know beforehand if a user will use the commit comnand during  
> tje runtime of the vm.
> 
> IMHO it'd be best to always open backing files read only and try to  
> open them for write access on the commit command. That command can  
> then fail gaciously.

That's another option.  Basically this means opening another
BlockDriverState for the backing device that is writeable inside
bdrv_commit().

Re: [Qemu-devel] Re: [PATCH] block: Enable fall-back to read-only for backing file

[Alexander Graf]

Christoph Hellwig wrote:
> On Mon, Feb 01, 2010 at 10:25:52AM +0100, Alexander Graf wrote:
>   
>> We don't know beforehand if a user will use the commit comnand during  
>> tje runtime of the vm.
>>
>> IMHO it'd be best to always open backing files read only and try to  
>> open them for write access on the commit command. That command can  
>> then fail gaciously.
>>     
>
> That's another option.  Basically this means opening another
> BlockDriverState for the backing device that is writeable inside
> bdrv_commit().
>   

Or another function call hook to change the backing device state from RO
to RW.

Alex

Re: [Qemu-devel] Re: [PATCH] block: Enable fall-back to read-only for backing file

[Jamie Lokier]

Christoph Hellwig wrote:
> On Mon, Feb 01, 2010 at 10:25:52AM +0100, Alexander Graf wrote:
> > We don't know beforehand if a user will use the commit comnand during  
> > tje runtime of the vm.
> > 
> > IMHO it'd be best to always open backing files read only and try to  
> > open them for write access on the commit command. That command can  
> > then fail gaciously.
> 
> That's another option.  Basically this means opening another
> BlockDriverState for the backing device that is writeable inside
> bdrv_commit().

I'd prefer that.  I like having higher confidence that random bugs in
Qemu are unlikely to accidentally modify precious backing files,
except when I've issued an explicit commit command.

-- Jamie