From b1e6fc0817dca14a3581d7b0979a5885608981f2 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Tue, 6 May 2014 12:11:42 +0200 Subject: [PATCH] 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 +++++++++++++++++++--------------- include/block/block.h | 2 +- tests/qemu-iotests/051 | 4 ++++ tests/qemu-iotests/051.out | 10 ++++++++++ 4 files changed, 34 insertions(+), 16 deletions(-) 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; diff --git a/include/block/block.h b/include/block/block.h index 27d8598eec..1b119aac24 100644 --- a/include/block/block.h +++ b/include/block/block.h @@ -195,7 +195,7 @@ int bdrv_open_image(BlockDriverState **pbs, const char *filename, QDict *options, const char *bdref_key, int flags, bool allow_none, Error **errp); int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp); -void bdrv_append_temp_snapshot(BlockDriverState *bs, Error **errp); +void bdrv_append_temp_snapshot(BlockDriverState *bs, int flags, Error **errp); int bdrv_open(BlockDriverState **pbs, const char *filename, const char *reference, QDict *options, int flags, BlockDriver *drv, Error **errp); diff --git a/tests/qemu-iotests/051 b/tests/qemu-iotests/051 index 073dc7a2d3..c4af131a66 100755 --- a/tests/qemu-iotests/051 +++ b/tests/qemu-iotests/051 @@ -233,6 +233,10 @@ echo 'qemu-io ide0-hd0 "write -P 0x22 0 4k"' | run_qemu -drive file="$TEST_IMG", $QEMU_IO -c "read -P 0x22 0 4k" "$TEST_IMG" | _filter_qemu_io +echo -e 'qemu-io ide0-hd0 "write -P 0x33 0 4k"\ncommit ide0-hd0' | run_qemu -drive file="$TEST_IMG",snapshot=on | _filter_qemu_io + +$QEMU_IO -c "read -P 0x33 0 4k" "$TEST_IMG" | _filter_qemu_io + # success, all done echo "*** done" rm -f $seq.full diff --git a/tests/qemu-iotests/051.out b/tests/qemu-iotests/051.out index 01b0384472..31e329e893 100644 --- a/tests/qemu-iotests/051.out +++ b/tests/qemu-iotests/051.out @@ -356,6 +356,16 @@ wrote 4096/4096 bytes at offset 0 4 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) (qemu) qququiquit +read 4096/4096 bytes at offset 0 +4 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +Testing: -drive file=TEST_DIR/t.qcow2,snapshot=on +QEMU X.Y.Z monitor - type 'help' for more information +(qemu) qqeqemqemuqemu-qemu-iqemu-ioqemu-io qemu-io iqemu-io idqemu-io ideqemu-io ide0qemu-io ide0-qemu-io ide0-hqemu-io ide0-hdqemu-io ide0-hd0qemu-io ide0-hd0 qemu-io ide0-hd0 "qemu-io ide0-hd0 "wqemu-io ide0-hd0 "wrqemu-io ide0-hd0 "wriqemu-io ide0-hd0 "writqemu-io ide0-hd0 "writeqemu-io ide0-hd0 "write qemu-io ide0-hd0 "write -qemu-io ide0-hd0 "write -Pqemu-io ide0-hd0 "write -P qemu-io ide0-hd0 "write -P 0qemu-io ide0-hd0 "write -P 0xqemu-io ide0-hd0 "write -P 0x3qemu-io ide0-hd0 "write -P 0x33qemu-io ide0-hd0 "write -P 0x33 qemu-io ide0-hd0 "write -P 0x33 0qemu-io ide0-hd0 "write -P 0x33 0 qemu-io ide0-hd0 "write -P 0x33 0 4qemu-io ide0-hd0 "write -P 0x33 0 4kqemu-io ide0-hd0 "write -P 0x33 0 4k" +wrote 4096/4096 bytes at offset 0 +4 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +(qemu) ccocomcommcommicommitcommit commit icommit idcommit idecommit ide0commit ide0-commit ide0-hcommit ide0-hdcommit ide0-hd0 +(qemu) qququiquit + read 4096/4096 bytes at offset 0 4 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) *** done