block: Enable fall-back to read-only for backing file

PATCH In review Naphtali Sprei 5 participants 7 messages Jan 31, 2010 – Feb 1, 2010

Share & tools

Naphtali Sprei

author

2010-01-31 16:50 UTC #0 lore raw

PATCH 1/1 block: Enable fall-back to read-only for backing file

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

Quoted 1 line

From 4a10750f5c91b1383118e4421f6b8d3ff3e79b2f Mon Sep 17 00:00:00 2001

From: Naphtali Sprei <[email protected]>
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 <[email protected]>

---
 block.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

block.c +5 -0

index 1919d19..d1b0f3d 100644
@@ -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);

Signature · 1 line

1.6.3.3

Kevin Wolf

2010-02-01 08:47 UTC #1 lore raw

Am 31.01.2010 17:49, schrieb Naphtali Sprei:

Quoted 10 lines

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

Christoph Hellwig

2010-02-01 09:07 UTC #2 lore raw

Quoted 5 lines · Kevin Wolf

Quoted 1 line
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.

Alexander Graf

2010-02-01 09:25 UTC #3 lore raw

Am 01.02.2010 um 10:06 schrieb Christoph Hellwig <[email protected]>:

Quoted 10 lines · Kevin Wolf

Quoted 5 lines
Quoted 1 line
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

Quoted 1 line

Christoph Hellwig

2010-02-01 09:30 UTC #4 lore raw

Quoted 6 lines · Alexander Graf

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

Alexander Graf

2010-02-01 10:05 UTC #5 lore raw

Quoted 5 lines · Christoph Hellwig

Quoted 3 lines · Alexander Graf
We don't know beforehand if a user will use the commit comnand during tje runtime of the vm.

Quoted 9 lines

Quoted 4 lines
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

Jamie Lokier

2010-02-01 12:32 UTC #6 lore raw

In reply to Christoph Hellwig

Quoted 11 lines · Christoph Hellwig

Quoted 6 lines · Alexander Graf
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