From: Jan Kiszka <jan.kiszka@web.de>
To: Kevin Wolf <kwolf@redhat.com>, qemu-devel@nongnu.org
Cc: stefanha@redhat.com, mreitz@redhat.com
Subject: Re: [Qemu-devel] [PATCH] block: Fix open flags with BDRV_O_SNAPSHOT
Date: Tue, 06 May 2014 13:10:42 +0200 [thread overview]
Message-ID: <5368C332.3040302@web.de> (raw)
In-Reply-To: <1399371550-5212-1-git-send-email-kwolf@redhat.com>
[-- Attachment #1: Type: text/plain, Size: 10133 bytes --]
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.kiszka@web.de>
> Signed-off-by: Kevin Wolf <kwolf@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
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]
next prev parent reply other threads:[~2014-05-06 11:16 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-05-06 10:19 [Qemu-devel] [PATCH] block: Fix open flags with BDRV_O_SNAPSHOT Kevin Wolf
2014-05-06 11:10 ` Jan Kiszka [this message]
2014-05-06 21:03 ` Max Reitz
2014-05-07 8:24 ` Kevin Wolf
2014-05-09 8:14 ` Stefan Hajnoczi
2014-05-06 21:01 ` Max Reitz
2014-05-07 8:31 ` Stefan Hajnoczi
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=5368C332.3040302@web.de \
--to=jan.kiszka@web.de \
--cc=kwolf@redhat.com \
--cc=mreitz@redhat.com \
--cc=qemu-devel@nongnu.org \
--cc=stefanha@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).