On 2014-05-06 12:19, Kevin Wolf wrote:
> 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 releaved 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 <jan.kis...@web.de>
> Signed-off-by: Kevin Wolf <kw...@redhat.com>
> ---
> 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 b749d31..c90c71a 100644
> --- a/block.c
> +++ b/block.c
> @@ -775,6 +775,16 @@ void bdrv_disable_copy_on_read(BlockDriverState *bs)
> }
>
> /*
> + * 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 467fb2b..2fda81c 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -191,7 +191,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 073dc7a..c4af131 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 01b0384..31e329e 100644
> --- a/tests/qemu-iotests/051.out
> +++ b/tests/qemu-iotests/051.out
> @@ -358,4 +358,14 @@ wrote 4096/4096 bytes at offset 0
>
> 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)
> q[K[Dqe[K[D[Dqem[K[D[D[Dqemu[K[D[D[D[Dqemu-[K[D[D[D[D[Dqemu-i[K[D[D[D[D[D[Dqemu-io[K[D[D[D[D[D[D[Dqemu-io
> [K[D[D[D[D[D[D[D[Dqemu-io i[K[D[D[D[D[D[D[D[D[Dqemu-io
> id[K[D[D[D[D[D[D[D[D[D[Dqemu-io
> ide[K[D[D[D[D[D[D[D[D[D[D[Dqemu-io
> ide0[K[D[D[D[D[D[D[D[D[D[D[D[Dqemu-io
> ide0-[K[D[D[D[D[D[D[D[D[D[D[D[D[Dqemu-io
> ide0-h[K[D[D[D[D[D[D[D[D[D[D[D[D[D[Dqemu-io
> ide0-hd[K[D[D[D[D[D[D[D[D[D[D[D[D[D[D[Dqemu-io
> ide0-hd0[K[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[Dqemu-io ide0-hd0
> [K[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[Dqemu-io ide0-hd0
> "[K[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[Dqemu-io ide0-hd0
> "w[K[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[Dqemu-io
> ide0-hd0
> "wr[K[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[Dqemu-io
> ide0-hd0 "wri[K[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D!
> [D[Dqemu-io ide0-hd0
> "writ[K[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[Dqemu-io
> ide0-hd0
> "write[K[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[Dqemu-io
> ide0-hd0 "write
> [K[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[Dqemu-io
> ide0-hd0 "write
> -[K[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[Dqemu-io
> ide0-hd0 "write
> -P[K[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[Dqemu-io
> ide0-hd0 "write -P
> [K[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[Dqemu-io
> ide0-hd0 "write -P
> 0[K[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[Dqemu-io
> ide0-hd0 "write -P
> 0x[K[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[Dqemu-io
> ide0-hd0 "write -P
> 0x3[K[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[Dqemu-io
> ide0-hd0 "w!
> rite -P 0x33[K[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D
> [D[D[D[D[D[D[D[D[D[D[D[D[Dqemu-io ide0-hd0 "write -P 0x33
> [K[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[Dqemu-io
> ide0-hd0 "write -P 0x33
> 0[K[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[Dqemu-io
> ide0-hd0 "write -P 0x33 0
> [K[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[Dqemu-io
> ide0-hd0 "write -P 0x33 0
> 4[K[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[Dqemu-io
> ide0-hd0 "write -P 0x33 0
> 4k[K[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[D[Dqemu-io
> ide0-hd0 "write -P 0x33 0 4k"[K
> +wrote 4096/4096 bytes at offset 0
> +4 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +(qemu)
> c[K[Dco[K[D[Dcom[K[D[D[Dcomm[K[D[D[D[Dcommi[K[D[D[D[D[Dcommit[K[D[D[D[D[D[Dcommit
> [K[D[D[D[D[D[D[Dcommit i[K[D[D[D[D[D[D[D[Dcommit
> id[K[D[D[D[D[D[D[D[D[Dcommit
> ide[K[D[D[D[D[D[D[D[D[D[Dcommit
> ide0[K[D[D[D[D[D[D[D[D[D[D[Dcommit
> ide0-[K[D[D[D[D[D[D[D[D[D[D[D[Dcommit
> ide0-h[K[D[D[D[D[D[D[D[D[D[D[D[D[Dcommit
> ide0-hd[K[D[D[D[D[D[D[D[D[D[D[D[D[D[Dcommit ide0-hd0[K
> +(qemu) q[K[Dqu[K[D[Dqui[K[D[D[Dquit[K
> +
> +read 4096/4096 bytes at offset 0
> +4 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> *** done
> Works fine here! (For unknown reason, applying the iotest hunk didn't work for me, though.) Thanks, Jan
signature.asc
Description: OpenPGP digital signature
