From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:44446) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aQMFe-0006iV-VP for qemu-devel@nongnu.org; Mon, 01 Feb 2016 16:43:15 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aQMFd-0004vA-RO for qemu-devel@nongnu.org; Mon, 01 Feb 2016 16:43:14 -0500 References: From: Max Reitz Message-ID: <56AFD166.9000904@redhat.com> Date: Mon, 1 Feb 2016 22:43:02 +0100 MIME-Version: 1.0 In-Reply-To: Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="636PTND8IsAKXFg6pGrs1inWdvOhbhSTE" Subject: Re: [Qemu-devel] [Qemu-block] [PATCH 1/2] block: change parent backing link when *tqe_prev == NULL List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Jeff Cody , qemu-devel@nongnu.org Cc: kwolf@redhat.com, qemu-stable@nongnu.org, qemu-block@nongnu.org This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --636PTND8IsAKXFg6pGrs1inWdvOhbhSTE Content-Type: text/plain; charset=iso-8859-15 Content-Transfer-Encoding: quoted-printable On 30.01.2016 06:17, Jeff Cody wrote: > In change_parent_backing_link(), we only inserted the new > BlockDriverState entry into the device_list if the tqe_prev pointer was= > NULL. However, we must also allow insertion when the BDS pointed > to by the tqe_prev pointer is NULL as well. >=20 > This fixes a bug with external snapshots, and live active layer commits= =2E >=20 > After a live snapshot occurs, the active layer and the base layer both > have a non-NULL tqe_prev field in the device_list, although the base > node's tqe_prev field points to a NULL entry. >=20 > Once the active commit is finished, bdrv_replace_in_backing_chain() is > called to set the base node as the new active node, and remove the > node that was the prior active layer from the device_list. >=20 > If we only check against the tqe_prev pointer field and not the entity > it is pointing to, then we fail to insert base image into the device > list. The previous active layer is still removed from the device_list,= > leaving an empty device_list queue. >=20 > With an empty device_list queue, odd behavior occurs - such as not > allowing any more live snapshots. >=20 > This commit fixes this issue, by checking for a NULL tqe_prev entity > in the devices_list. >=20 > Signed-off-by: Jeff Cody > --- > block.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) >=20 > diff --git a/block.c b/block.c > index 5709d3d..0b8526b 100644 > --- a/block.c > +++ b/block.c > @@ -2272,7 +2272,7 @@ static void change_parent_backing_link(BlockDrive= rState *from, > } > if (from->blk) { > blk_set_bs(from->blk, to); > - if (!to->device_list.tqe_prev) { > + if (!to->device_list.tqe_prev || !*to->device_list.tqe_prev) {= I'm not sure this is the right fix; bdrv_make_anon() clearly states that we do want device_list.tqe_prev to be NULL if and only if the BDS is not part of the device list. So this should not be happening. > QTAILQ_INSERT_BEFORE(from, to, device_list); > } > QTAILQ_REMOVE(&bdrv_states, from, device_list); Inserting a from->device_list.tqe_prev =3D NULL; here makes the iotest happy and looks like the better fix to me. Max >=20 --636PTND8IsAKXFg6pGrs1inWdvOhbhSTE 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 iQEcBAEBCAAGBQJWr9FmAAoJEDuxQgLoOKytQLgIAIZGao7SYdGIbFaoKlznGnu8 V+2dhpzRVLA0q8EWHUP/sGs05SBtHVntFtgZhTYKv5ckdppGU59mlJhtN7qLE9yJ 17O91sDVW0kSBa/AvhsSxrj9DeCTvIhQDzWg2HUW3F1DsmwswIUVhr1KH4PHpxyU RU2Fcb5DCzwy5HGL/+mlk6sAIMnx7grP+Fa9r67CIVYfRucIHDRQd3HGTREpsqAg iA/2cO8SfqEfbG/8YGMq6q6ZDwBnOVr3bmMfYZlMzzt7JRpQQKDBBK8xvStENZPz 0gMJ8yyLnx0LLR6+bgG3/vUCD/nC4IpZcsUawT9+SvA32uaIR/mAyvnu01tvycc= =nVOP -----END PGP SIGNATURE----- --636PTND8IsAKXFg6pGrs1inWdvOhbhSTE--