From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:42157) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aS3eG-0003gr-3f for qemu-devel@nongnu.org; Sat, 06 Feb 2016 09:15:41 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aS3eE-0003QG-Hl for qemu-devel@nongnu.org; Sat, 06 Feb 2016 09:15:40 -0500 References: <20160204173639.GA5772@li141-249.members.linode.com> <56B5EF57.2080301@redhat.com> <20160206134717.GM9530@li141-249.members.linode.com> From: Max Reitz Message-ID: <56B60001.2080509@redhat.com> Date: Sat, 6 Feb 2016 15:15:29 +0100 MIME-Version: 1.0 In-Reply-To: <20160206134717.GM9530@li141-249.members.linode.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="iM0PMCgKeneQGMNhLxmAB77hfRqW7l1jx" Subject: Re: [Qemu-devel] [Qemu-block] [PATCH] blockdev: Unset temporary flag when changing medium. List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alyssa Milburn Cc: Kevin Wolf , qemu-devel@nongnu.org, qemu-block@nongnu.org, Markus Armbruster This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --iM0PMCgKeneQGMNhLxmAB77hfRqW7l1jx Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: quoted-printable On 06.02.2016 14:47, Alyssa Milburn wrote: > On Sat, Feb 06, 2016 at 02:04:23PM +0100, Max Reitz wrote: >> On 04.02.2016 18:36, Alyssa Milburn wrote: >>> This avoids a 'change' command from the monitor unlink()ing the new >>> file if the bdrv was previously snapshotted. >>> >>> Signed-off-by: Alyssa Milburn >>> --- >>> blockdev.c | 1 + >>> 1 file changed, 1 insertion(+) >>> >>> diff --git a/blockdev.c b/blockdev.c >>> index be4ca44..d39c2e6 100644 >>> --- a/blockdev.c >>> +++ b/blockdev.c >>> @@ -2514,6 +2514,7 @@ void qmp_blockdev_change_medium(const char *dev= ice, const char *filename, >>> } >>> =20 >>> bdrv_flags =3D blk_get_open_flags_from_root_state(blk); >>> + bdrv_flags &=3D ~BDRV_O_TEMPORARY; >>> =20 >>> if (!has_read_only) { >>> read_only =3D BLOCKDEV_CHANGE_READ_ONLY_MODE_RETAIN; >>> >> >> This patch is correct (thanks!), but I think we want to unset even mor= e >> flags, namely BDRV_O_SNAPSHOT, BDRV_O_NO_BACKING, and BDRV_O_PROTOCOL.= >=20 > Thanks. I posted an updated patch with this change (but of course, this= is a > trivial patch anyway). It's not entirely clear to me which flags should= (n't) > be preserved and where to do that. Ideally, none would be. You'd use blockdev-add and x-blockdev-{remove,insert}-medium to do the change manually, and specify all the options for the new BlockDriverState using blockdev-add. The BlockBackend's root state is just a legacy hack because the 'change' command preserved some flags and other options of the old medium for the new one that is inserted. So I think we should preserve as many flags as possible, unless something weird might happen which one doesn't want, and so I guessed for each of the flags if they'd do something I'd not expect when issuing a change command: - O_TEMPORARY: Would delete the new medium's file. - O_SNAPSHOT: Would silently create a snapshot over the new medium (actually, apparently it does not, but this is a bug...). - O_NO_BACKING: Would not open the backing chain for the new image. - O_PROTOCOL: Would ignore the format of the new file and just open it raw. While I personally wouldn't like any flag to be inherited from the previous medium, the rest doesn't look like it would do much harm. If one doesn't like that the cache is fully inherited, they just have to use blockdev-add and x-blockdev-{remove,insert}-medium. > Since the snapshotted file is part o= f a > chain (and the original flags are "correct") I wondered if this might n= ot be > the ideal fix, but it seems safe/sane to do it here. Well, another place to do it is in blk_update_root_state() or inside of blk_get_open_flags_from_root_state(). But I don't think it matters because this is the only place that calls that function (and this is not supposed to change, because it is just legacy handling anyway). Max --iM0PMCgKeneQGMNhLxmAB77hfRqW7l1jx 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 iQEcBAEBCAAGBQJWtgABAAoJEDuxQgLoOKytxKoIAITp1/0iVNEMBF5wlKv3uZL7 AdI+3sCnmyU1UE/1d+cFGS0dpBSVvaFUnomJRXZAY7/+Ycw521UrEz8+8iHJCz77 mEKr/gZAfz0P6YG8kVerzAKGwZ/cOAgDdZZsD0ebr97VWPtKeGqw/hHVqCXv60KD 5hGm0aQBTgikzk1EwSS6ANvfhuYpR3IPapLU87KCcEjYKVRlis4uAfl+Jf9j7N0a 1Y92t60+Npu8mCrzYgmU+PAmViqdhk1C/138agnRcOW8r5PJWNXpBo/TwdI7lOuL sIlS/0tIzHTgRLZQ87Lpt9MmcBaOuPR5d4fIpZZ7C4PQSe7+gLeSnZWqeb8n/cs= =MiBP -----END PGP SIGNATURE----- --iM0PMCgKeneQGMNhLxmAB77hfRqW7l1jx--