From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:41641) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bCp4i-0001LD-5C for qemu-devel@nongnu.org; Tue, 14 Jun 2016 10:12:21 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bCp4d-0007CM-Qm for qemu-devel@nongnu.org; Tue, 14 Jun 2016 10:12:15 -0400 Received: from mx1.redhat.com ([209.132.183.28]:33013) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bCp4d-0007CI-IO for qemu-devel@nongnu.org; Tue, 14 Jun 2016 10:12:11 -0400 References: <1465821535-6664-1-git-send-email-den@openvz.org> From: Max Reitz Message-ID: Date: Tue, 14 Jun 2016 16:12:08 +0200 MIME-Version: 1.0 In-Reply-To: <1465821535-6664-1-git-send-email-den@openvz.org> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="w1Wj53Ii7A4247TMQgqdLQrhvgjlHTLto" Subject: Re: [Qemu-devel] [PATCH 1/1] block: fix libvirt snapshot with existing bitmaps List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Denis V. Lunev" , qemu-devel@nongnu.org Cc: Vladimir Sementsov-Ogievskiy , Kevin Wolf This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --w1Wj53Ii7A4247TMQgqdLQrhvgjlHTLto From: Max Reitz To: "Denis V. Lunev" , qemu-devel@nongnu.org Cc: Vladimir Sementsov-Ogievskiy , Kevin Wolf Message-ID: Subject: Re: [PATCH 1/1] block: fix libvirt snapshot with existing bitmaps References: <1465821535-6664-1-git-send-email-den@openvz.org> In-Reply-To: <1465821535-6664-1-git-send-email-den@openvz.org> Content-Type: text/plain; charset=iso-8859-15 Content-Transfer-Encoding: quoted-printable On 13.06.2016 14:38, Denis V. Lunev wrote: > From: Vladimir Sementsov-Ogievskiy >=20 > Fix the following bug: >=20 > # virsh start test > Domain test started >=20 > # virsh qemu-monitor-command test \ > '{"execute":"block-dirty-bitmap-add",\ > "arguments":{"node":"drive0","name":"ab"}}' > {"return":{},"id":"libvirt-36"}'}' >=20 > # virsh snapshot-create test > error: Unable to read from monitor: Connection reset by peer >=20 > Actually, assert "assert(pos < hb->size)" in hbitmap_iter_init fires, > because qcow2_save_vmstate just writes to bs (not to bs->file->bs) afte= r > the end of the drive. >=20 > Signed-off-by: Vladimir Sementsov-Ogievskiy > Signed-off-by: Denis V. Lunev > CC: Kevin Wolf > CC: Max Reitz > --- > block/dirty-bitmap.c | 6 ++++++ > 1 file changed, 6 insertions(+) >=20 > diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c > index 4902ca5..7b636ee 100644 > --- a/block/dirty-bitmap.c > +++ b/block/dirty-bitmap.c > @@ -364,6 +364,11 @@ void bdrv_set_dirty(BlockDriverState *bs, int64_t = cur_sector, > int nr_sectors) > { > BdrvDirtyBitmap *bitmap; > + > + if (cur_sector >=3D bdrv_nb_sectors(bs)) { > + return; > + } I'd use bitmap->size instead of bdrv_nb_sectors(bs). In addition, I'd put an assert(cur_sector + nr_sectors <=3D bitmap->size)= after this conditional block; I understand that we will probably never write to both the disk and the VM state in a single operation, therefore asserting this is sufficient. (If we want to do it right, we'd need to truncate nr_sectors in case cur_sector < bdrv_nb_sectors(bs) && cur_sector + nr_sectors > bitmap->siz= e) > + > QLIST_FOREACH(bitmap, &bs->dirty_bitmaps, list) { > if (!bdrv_dirty_bitmap_enabled(bitmap)) { > continue; >=20 Apart from that: I see that this is the only place where it's really relevant. However, I feel somewhat bad putting it just here and not in the other functions (bdrv_{re,}set_dirty_bitmap() and bdrv_get_dirty()). I just want to bring this to your attention. Technically, putting the check into this function is completely sufficient, so if you think it's too much work to put it into the other ones, I'm fine with that (although I think we should at least put an assertion into those other functions). Max --w1Wj53Ii7A4247TMQgqdLQrhvgjlHTLto 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 iQEcBAEBCAAGBQJXYBC4AAoJEDuxQgLoOKyteGAH/1qAg1cauYJ3ICjF/f/Ayry+ Hljh3ujWvWmLZIHsxwsmpZHUYdbLJSYAUawoXsKhRNKyGS+9ZlkNmxU7SKOm4UvP q4h/utwVw8sgEVh1cQfBRNCVr24YKFbYeuK+M04C5SRfxxEtayG8aDARAGpIArxU y77aApFwbrTCAxkZJKRZfGMXt/fGoKe9LHVA1D3bMH0faGj/BrlOSIlKi5r63Fvk kC5mrQSxsblbLNHdRdxXox6PmNh2ulOywFs8i23HDRTV2wxukbVW5zMjP4mqhxDv 1LZnWn12YZsRn7qw8+sjNOGZNCIvsdfouSL7v7fWzxjzf6J8aMQpk8bSWBxT2YA= =rIu3 -----END PGP SIGNATURE----- --w1Wj53Ii7A4247TMQgqdLQrhvgjlHTLto--