| Message ID | 4B65B4B7.2030700@redhat.com |
|---|---|
| State | New |
| Headers | show |
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
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.
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 >
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().
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
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
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);