From b1e6fc0817dca14a3581d7b0979a5885608981f2 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Tue, 6 May 2014 12:11:42 +0200 Subject: block: Fix open flags with BDRV_O_SNAPSHOT The immediately visible effect of this patch is that it fixes committing a temporary snapshot to its backing file. Previously, it would fail with a "permission denied" error because bdrv_inherited_flags() forced the backing file to be read-only, ignoring the r/w reopen of bdrv_commit(). The bigger problem this revealed is that the original open flags must actually only be applied to the temporary snapshot, and the original image file must be treated as a backing file of the temporary snapshot and get the right flags for that. Reported-by: Jan Kiszka Signed-off-by: Kevin Wolf Reviewed-by: Max Reitz Signed-off-by: Stefan Hajnoczi --- block.c | 34 +++++++++++++++++++--------------- 1 file changed, 19 insertions(+), 15 deletions(-) (limited to 'block.c') diff --git a/block.c b/block.c index b749d31ad4..c90c71aa32 100644 --- a/block.c +++ b/block.c @@ -774,6 +774,16 @@ void bdrv_disable_copy_on_read(BlockDriverState *bs) bs->copy_on_read--; } +/* + * Returns the flags that a temporary snapshot should get, based on the + * originally requested flags (the originally requested image will have flags + * like a backing file) + */ +static int bdrv_temp_snapshot_flags(int flags) +{ + return (flags & ~BDRV_O_SNAPSHOT) | BDRV_O_TEMPORARY; +} + /* * Returns the flags that bs->file should get, based on the given flags for * the parent BDS @@ -787,11 +797,6 @@ static int bdrv_inherited_flags(int flags) * so we can enable both unconditionally on lower layers. */ flags |= BDRV_O_CACHE_WB | BDRV_O_UNMAP; - /* The backing file of a temporary snapshot is read-only */ - if (flags & BDRV_O_SNAPSHOT) { - flags &= ~BDRV_O_RDWR; - } - /* Clear flags that only apply to the top layer */ flags &= ~(BDRV_O_SNAPSHOT | BDRV_O_NO_BACKING | BDRV_O_COPY_ON_READ); @@ -817,11 +822,6 @@ static int bdrv_open_flags(BlockDriverState *bs, int flags) { int open_flags = flags | BDRV_O_CACHE_WB; - /* The backing file of a temporary snapshot is read-only */ - if (flags & BDRV_O_SNAPSHOT) { - open_flags &= ~BDRV_O_RDWR; - } - /* * Clear flags that are internal to the block layer before opening the * image. @@ -1206,7 +1206,7 @@ done: return ret; } -void bdrv_append_temp_snapshot(BlockDriverState *bs, Error **errp) +void bdrv_append_temp_snapshot(BlockDriverState *bs, int flags, Error **errp) { /* TODO: extra byte is a hack to ensure MAX_PATH space on Windows. */ char *tmp_filename = g_malloc0(PATH_MAX + 1); @@ -1262,8 +1262,7 @@ void bdrv_append_temp_snapshot(BlockDriverState *bs, Error **errp) bs_snapshot = bdrv_new("", &error_abort); ret = bdrv_open(&bs_snapshot, NULL, NULL, snapshot_options, - (bs->open_flags & ~BDRV_O_SNAPSHOT) | BDRV_O_TEMPORARY, - bdrv_qcow2, &local_err); + flags, bdrv_qcow2, &local_err); if (ret < 0) { error_propagate(errp, local_err); goto out; @@ -1298,6 +1297,7 @@ int bdrv_open(BlockDriverState **pbs, const char *filename, BlockDriverState *file = NULL, *bs; const char *drvname; Error *local_err = NULL; + int snapshot_flags = 0; assert(pbs); @@ -1358,6 +1358,10 @@ int bdrv_open(BlockDriverState **pbs, const char *filename, if (flags & BDRV_O_RDWR) { flags |= BDRV_O_ALLOW_RDWR; } + if (flags & BDRV_O_SNAPSHOT) { + snapshot_flags = bdrv_temp_snapshot_flags(flags); + flags = bdrv_backing_flags(flags); + } assert(file == NULL); ret = bdrv_open_image(&file, filename, options, "file", @@ -1417,8 +1421,8 @@ int bdrv_open(BlockDriverState **pbs, const char *filename, /* For snapshot=on, create a temporary qcow2 overlay. bs points to the * temporary snapshot afterwards. */ - if (flags & BDRV_O_SNAPSHOT) { - bdrv_append_temp_snapshot(bs, &local_err); + if (snapshot_flags) { + bdrv_append_temp_snapshot(bs, snapshot_flags, &local_err); if (local_err) { error_propagate(errp, local_err); goto close_and_fail; -- cgit v1.2.3