From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:42032) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bsb4c-0001Uf-1Q for qemu-devel@nongnu.org; Fri, 07 Oct 2016 15:44:51 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bsb4Z-0008M8-Vz for qemu-devel@nongnu.org; Fri, 07 Oct 2016 15:44:49 -0400 References: <1475232808-4852-1-git-send-email-vsementsov@virtuozzo.com> <1475232808-4852-13-git-send-email-vsementsov@virtuozzo.com> From: Max Reitz Message-ID: Date: Fri, 7 Oct 2016 21:44:37 +0200 MIME-Version: 1.0 In-Reply-To: <1475232808-4852-13-git-send-email-vsementsov@virtuozzo.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="OPtPJ8TlWLwRebI9jaDdhSpLBw1bO6Dc5" Subject: Re: [Qemu-devel] [PATCH 12/22] qcow2-bitmap: add IN_USE flag List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Vladimir Sementsov-Ogievskiy , qemu-block@nongnu.org, qemu-devel@nongnu.org Cc: kwolf@redhat.com, armbru@redhat.com, eblake@redhat.com, jsnow@redhat.com, famz@redhat.com, den@openvz.org, stefanha@redhat.com, pbonzini@redhat.com This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --OPtPJ8TlWLwRebI9jaDdhSpLBw1bO6Dc5 From: Max Reitz To: Vladimir Sementsov-Ogievskiy , qemu-block@nongnu.org, qemu-devel@nongnu.org Cc: kwolf@redhat.com, armbru@redhat.com, eblake@redhat.com, jsnow@redhat.com, famz@redhat.com, den@openvz.org, stefanha@redhat.com, pbonzini@redhat.com Message-ID: Subject: Re: [PATCH 12/22] qcow2-bitmap: add IN_USE flag References: <1475232808-4852-1-git-send-email-vsementsov@virtuozzo.com> <1475232808-4852-13-git-send-email-vsementsov@virtuozzo.com> In-Reply-To: <1475232808-4852-13-git-send-email-vsementsov@virtuozzo.com> Content-Type: text/plain; charset=iso-8859-15 Content-Transfer-Encoding: quoted-printable On 30.09.2016 12:53, Vladimir Sementsov-Ogievskiy wrote: > This flag means that the bitmap is now in use by the software or was no= t > successfully saved. In any way, with this flag set the bitmap data must= > be considered inconsistent and should not be loaded. >=20 > With current implementation this flag is never set, as we just remove > bitmaps from the image after loading. But it defined in qcow2 spec and > must be handled. Also, it can be used in future, if async schemes of > bitmap loading/saving are implemented. >=20 > We also remove in-use bitmaps from the image on open. >=20 > Signed-off-by: Vladimir Sementsov-Ogievskiy > --- > block/qcow2-bitmap.c | 17 ++++++++++++++++- > 1 file changed, 16 insertions(+), 1 deletion(-) Don't you want to make use of this flag? It would appear useful to me if you just marked autoload bitmaps as in_use instead of deleting them from the image when it's opened and then overwrite them when the image is clos= ed. > diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c > index a5be25a..8cf40f0 100644 > --- a/block/qcow2-bitmap.c > +++ b/block/qcow2-bitmap.c > @@ -28,6 +28,7 @@ > #include "qemu/osdep.h" > #include "qapi/error.h" > #include "qemu/cutils.h" > +#include "exec/log.h" > =20 > #include "block/block_int.h" > #include "block/qcow2.h" > @@ -44,7 +45,8 @@ > #define BME_MAX_NAME_SIZE 1023 > =20 > /* Bitmap directory entry flags */ > -#define BME_RESERVED_FLAGS 0xfffffffd > +#define BME_RESERVED_FLAGS 0xfffffffc > +#define BME_FLAG_IN_USE 1 This should be (1U << 0) to be consistent with the definition of BME_FLAG_AUTO. > #define BME_FLAG_AUTO (1U << 1) > =20 > /* bits [1, 8] U [56, 63] are reserved */ > @@ -287,6 +289,11 @@ static BdrvDirtyBitmap *load_bitmap(BlockDriverSta= te *bs, > BdrvDirtyBitmap *bitmap =3D NULL; > char *name =3D g_strndup(dir_entry_name_notcstr(entry), entry->nam= e_size); > =20 > + if (entry->flags & BME_FLAG_IN_USE) { > + error_setg(errp, "Bitmap '%s' is in use", name); > + goto fail; > + } > + > ret =3D bitmap_table_load(bs, entry, &bitmap_table); > if (ret < 0) { > error_setg_errno(errp, -ret, > @@ -533,6 +540,14 @@ int qcow2_read_bitmaps(BlockDriverState *bs, Error= **errp) > for_each_bitmap_dir_entry(e, dir, dir_size) { > uint32_t fl =3D e->flags; > =20 > + if (fl & BME_FLAG_IN_USE) { > + qemu_log("qcow2 warning: " > + "removing in_use bitmap '%.*s' for image %s.\n", > + e->name_size, (char *)(e + 1), bdrv_get_device_or= _node_name(bs)); I'm not sure whether qemu_log() is the right way to do this. I think fprintf() to stderr might actually be more appropriate. qemu_log() looks like it's mostly used for debugging purposes. Also, this is not a warning. You are just doing it. You are not warning someone about a cliff when he's already fallen off. But you can actually make it a warning: Just leave the bitmap in the image (maybe someone can do something with it?) and instead let qemu-img check clean it up. The warning should then just be "Warning: Ignoring in_use bitmap %.*s, use qemu-img check -r all to delete it". Max > + > + continue; > + } > + > if (fl & BME_FLAG_AUTO) { > BdrvDirtyBitmap *bitmap =3D load_bitmap(bs, e, errp); > if (bitmap =3D=3D NULL) { >=20 --OPtPJ8TlWLwRebI9jaDdhSpLBw1bO6Dc5 Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- iQEvBAEBCAAZBQJX9/slEhxtcmVpdHpAcmVkaGF0LmNvbQAKCRD0B9sAYdXPQKWm B/4923b56efplyJoVflLBYIAPwuYPAR9fBxIFKolQ6A4Gag5kWjMxBVzWj8qeSt4 CuEerN58krR9Nt+yfY694o6ketUF0oMZHij3IbcK4jTd/NLOiQljGcMi7OJVr6eu fhAWRzTelL6Ma5n7CkpqALVQy8BrZ2iZUm9fgb3kNlyZ00SO3QmutROSlWNdadr3 XAuA+BBr2LYrqYMa7BnlZoBoz/4tqUw4cLCxxuWpC79KVldj5lrNNHZDFPJBN0zr ljlLjAi+lWS2z1gz6WX3qMJQK3l83LP1U+ClDB1bzM5ZOD7Dn/LbLNUwLr6CAfnp IK2KTXE/SheYyvCSrRhYBl4b =l29m -----END PGP SIGNATURE----- --OPtPJ8TlWLwRebI9jaDdhSpLBw1bO6Dc5--