From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:39751) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WhdMM-0001Wd-5M for qemu-devel@nongnu.org; Tue, 06 May 2014 07:16:37 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WhdME-0001Xy-KB for qemu-devel@nongnu.org; Tue, 06 May 2014 07:16:30 -0400 Received: from mout.web.de ([212.227.15.14]:65140) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WhdME-0001Xp-AC for qemu-devel@nongnu.org; Tue, 06 May 2014 07:16:22 -0400 Message-ID: <5368C332.3040302@web.de> Date: Tue, 06 May 2014 13:10:42 +0200 From: Jan Kiszka MIME-Version: 1.0 References: <1399371550-5212-1-git-send-email-kwolf@redhat.com> In-Reply-To: <1399371550-5212-1-git-send-email-kwolf@redhat.com> Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="l6IWtR3L13Xhwneun3eLxlucGvibOJWLi" Subject: Re: [Qemu-devel] [PATCH] block: Fix open flags with BDRV_O_SNAPSHOT List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf , qemu-devel@nongnu.org Cc: stefanha@redhat.com, mreitz@redhat.com This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --l6IWtR3L13Xhwneun3eLxlucGvibOJWLi Content-Type: text/plain; charset=ISO-8859-15 Content-Transfer-Encoding: quoted-printable On 2014-05-06 12:19, Kevin Wolf wrote: > The immediately visible effect of this patch is that it fixes committin= g > a temporary snapshot to its backing file. Previously, it would fail wit= h > a "permission denied" error because bdrv_inherited_flags() forced the > backing file to be read-only, ignoring the r/w reopen of bdrv_commit().= >=20 > 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. >=20 > Reported-by: Jan Kiszka > Signed-off-by: Kevin Wolf > --- > 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(-) >=20 > 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 *b= s) > } > =20 > /* > + * Returns the flags that a temporary snapshot should get, based on th= e > + * originally requested flags (the originally requested image will hav= e 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 flag= s 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 |=3D BDRV_O_CACHE_WB | BDRV_O_UNMAP; > =20 > - /* The backing file of a temporary snapshot is read-only */ > - if (flags & BDRV_O_SNAPSHOT) { > - flags &=3D ~BDRV_O_RDWR; > - } > - > /* Clear flags that only apply to the top layer */ > flags &=3D ~(BDRV_O_SNAPSHOT | BDRV_O_NO_BACKING | BDRV_O_COPY_ON_= READ); > =20 > @@ -817,11 +822,6 @@ static int bdrv_open_flags(BlockDriverState *bs, i= nt flags) > { > int open_flags =3D flags | BDRV_O_CACHE_WB; > =20 > - /* The backing file of a temporary snapshot is read-only */ > - if (flags & BDRV_O_SNAPSHOT) { > - open_flags &=3D ~BDRV_O_RDWR; > - } > - > /* > * Clear flags that are internal to the block layer before opening= the > * image. > @@ -1206,7 +1206,7 @@ done: > return ret; > } > =20 > -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 =3D g_malloc0(PATH_MAX + 1); > @@ -1262,8 +1262,7 @@ void bdrv_append_temp_snapshot(BlockDriverState *= bs, Error **errp) > bs_snapshot =3D bdrv_new("", &error_abort); > =20 > ret =3D bdrv_open(&bs_snapshot, NULL, NULL, snapshot_options, > - (bs->open_flags & ~BDRV_O_SNAPSHOT) | BDRV_O_TEMPO= RARY, > - 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 =3D NULL, *bs; > const char *drvname; > Error *local_err =3D NULL; > + int snapshot_flags =3D 0; > =20 > assert(pbs); > =20 > @@ -1358,6 +1358,10 @@ int bdrv_open(BlockDriverState **pbs, const char= *filename, > if (flags & BDRV_O_RDWR) { > flags |=3D BDRV_O_ALLOW_RDWR; > } > + if (flags & BDRV_O_SNAPSHOT) { > + snapshot_flags =3D bdrv_temp_snapshot_flags(flags); > + flags =3D bdrv_backing_flags(flags); > + } > =20 > assert(file =3D=3D NULL); > ret =3D bdrv_open_image(&file, filename, options, "file", > @@ -1417,8 +1421,8 @@ int bdrv_open(BlockDriverState **pbs, const char = *filename, > =20 > /* For snapshot=3Don, 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 c= har *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=3D"$TEST_IMG", > =20 > $QEMU_IO -c "read -P 0x22 0 4k" "$TEST_IMG" | _filter_qemu_io > =20 > +echo -e 'qemu-io ide0-hd0 "write -P 0x33 0 4k"\ncommit ide0-hd0' | run= _qemu -drive file=3D"$TEST_IMG",snapshot=3Don | _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 > =20 > 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=3DTEST_DIR/t.qcow2,snapshot=3Don > +QEMU X.Y.Z monitor - type 'help' for more information > +(qemu) q=1B[K=1B[Dqe=1B[K=1B[D=1B[Dqem=1B[K=1B[D=1B[D=1B[Dqemu=1B[K=1B= [D=1B[D=1B[D=1B[Dqemu-=1B[K=1B[D=1B[D=1B[D=1B[D=1B[Dqemu-i=1B[K=1B[D=1B[D= =1B[D=1B[D=1B[D=1B[Dqemu-io=1B[K=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[Dqemu-i= o =1B[K=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[Dqemu-io i=1B[K=1B[D=1B[D=1B= [D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[Dqemu-io id=1B[K=1B[D=1B[D=1B[D=1B[D=1B[D=1B= [D=1B[D=1B[D=1B[D=1B[Dqemu-io ide=1B[K=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D= =1B[D=1B[D=1B[D=1B[Dqemu-io ide0=1B[K=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B= [D=1B[D=1B[D=1B[D=1B[Dqemu-io ide0-=1B[K=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B= [D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[Dqemu-io ide0-h=1B[K=1B[D=1B[D=1B[D=1B[D=1B= [D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[Dqemu-io ide0-hd=1B[K=1B[D=1B= [D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[Dqemu-i= o ide0-hd0=1B[K=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B= [D=1B[D=1B[D=1B[D=1B[Dqemu-io ide0-hd0 =1B[K=1B[D=1B[D=1B[D=1B[D=1B[D=1B[= D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[Dqemu-io ide0-hd0 = "=1B[K=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B= [D=1B[D=1B[D=1B[D=1B[Dqemu-io ide0-hd0 "w=1B[K=1B[D=1B[D=1B[D=1B[D=1B[D=1B= [D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[Dqemu-i= o ide0-hd0 "wr=1B[K=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[= D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[Dqemu-io ide0-hd0 "wri=1B[K=1B= [D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B= [D=1B[D=1B[D=1B[D! > =1B[D=1B[Dqemu-io ide0-hd0 "writ=1B[K=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B= [D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B= [Dqemu-io ide0-hd0 "write=1B[K=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B= [D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[Dq= emu-io ide0-hd0 "write =1B[K=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D= =1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B= [Dqemu-io ide0-hd0 "write -=1B[K=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B= [D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B= [D=1B[Dqemu-io ide0-hd0 "write -P=1B[K=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D= =1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B= [D=1B[D=1B[D=1B[D=1B[Dqemu-io ide0-hd0 "write -P =1B[K=1B[D=1B[D=1B[D=1B[= D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B= [D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[Dqemu-io ide0-hd0 "write -P 0=1B= [K=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B= [D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[Dqemu-i= o ide0-hd0 "write -P 0x=1B[K=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D= =1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B= [D=1B[D=1B[D=1B[D=1B[D=1B[Dqemu-io ide0-hd0 "write -P 0x3=1B[K=1B[D=1B[D=1B= [D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B= [D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[Dqemu-io ide= 0-hd0 "w! > rite -P 0x33=1B[K=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B= [D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B > [D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[Dqemu-io i= de0-hd0 "write -P 0x33 =1B[K=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D= =1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B= [D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[Dqemu-io ide0-hd0 "write -P 0x33= 0=1B[K=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B= [D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B= [D=1B[D=1B[D=1B[D=1B[Dqemu-io ide0-hd0 "write -P 0x33 0 =1B[K=1B[D=1B[D=1B= [D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B= [D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B= [D=1B[Dqemu-io ide0-hd0 "write -P 0x33 0 4=1B[K=1B[D=1B[D=1B[D=1B[D=1B[D=1B= [D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B= [D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[Dq= emu-io ide0-hd0 "write -P 0x33 0 4k=1B[K=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B= [D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B= [D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[Dq= emu-io ide0-hd0 "write -P 0x33 0 4k"=1B[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=1B[K=1B[Dco=1B[K=1B[D=1B[Dcom=1B[K=1B[D=1B[D=1B[Dcomm=1B[K=1B= [D=1B[D=1B[D=1B[Dcommi=1B[K=1B[D=1B[D=1B[D=1B[D=1B[Dcommit=1B[K=1B[D=1B[D= =1B[D=1B[D=1B[D=1B[Dcommit =1B[K=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[Dcommit= i=1B[K=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[Dcommit id=1B[K=1B[D=1B[D=1B= [D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[Dcommit ide=1B[K=1B[D=1B[D=1B[D=1B[D=1B[D=1B= [D=1B[D=1B[D=1B[D=1B[Dcommit ide0=1B[K=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D= =1B[D=1B[D=1B[D=1B[Dcommit ide0-=1B[K=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B= [D=1B[D=1B[D=1B[D=1B[Dcommit ide0-h=1B[K=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B= [D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[Dcommit ide0-hd=1B[K=1B[D=1B[D=1B[D=1B[D=1B= [D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[D=1B[Dcommit ide0-hd0=1B[K > +(qemu) q=1B[K=1B[Dqu=1B[K=1B[D=1B[Dqui=1B[K=1B[D=1B[D=1B[Dquit=1B[K > + > +read 4096/4096 bytes at offset 0 > +4 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) > *** done >=20 Works fine here! (For unknown reason, applying the iotest hunk didn't work for me, though.= ) Thanks, Jan --l6IWtR3L13Xhwneun3eLxlucGvibOJWLi Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.19 (GNU/Linux) Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iEYEARECAAYFAlNowzgACgkQitSsb3rl5xR5KQCfU0zO9xiERAR3xtvKRV2NZyzs czoAn35+vgm0uRM58pRWaQsBSLmUi/6P =FpU5 -----END PGP SIGNATURE----- --l6IWtR3L13Xhwneun3eLxlucGvibOJWLi--