From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:45951) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1btera-0008Dn-3t for qemu-devel@nongnu.org; Mon, 10 Oct 2016 13:59:47 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bterY-0006Vv-PR for qemu-devel@nongnu.org; Mon, 10 Oct 2016 13:59:46 -0400 References: <1475232808-4852-1-git-send-email-vsementsov@virtuozzo.com> <1475232808-4852-21-git-send-email-vsementsov@virtuozzo.com> From: Max Reitz Message-ID: <45d25503-97e1-8629-8731-3ae64c4bf12c@redhat.com> Date: Mon, 10 Oct 2016 19:59:33 +0200 MIME-Version: 1.0 In-Reply-To: <1475232808-4852-21-git-send-email-vsementsov@virtuozzo.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="4hTQH8PKVwMM9bSICoepi29iB6tfwUAN2" Subject: Re: [Qemu-devel] [PATCH 20/22] qcow2-dirty-bitmap: refcounts 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) --4hTQH8PKVwMM9bSICoepi29iB6tfwUAN2 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: <45d25503-97e1-8629-8731-3ae64c4bf12c@redhat.com> Subject: Re: [PATCH 20/22] qcow2-dirty-bitmap: refcounts References: <1475232808-4852-1-git-send-email-vsementsov@virtuozzo.com> <1475232808-4852-21-git-send-email-vsementsov@virtuozzo.com> In-Reply-To: <1475232808-4852-21-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: > Calculate refcounts for dirty bitmaps. The commit message should mention that this is for qcow2's qemu-img check implementation. >=20 > Signed-off-by: Vladimir Sementsov-Ogievskiy > --- > block/qcow2-bitmap.c | 60 ++++++++++++++++++++++++++++++++++++++++++= ++++++++ > block/qcow2-refcount.c | 14 +++++++----- > block/qcow2.h | 5 +++++ > 3 files changed, 74 insertions(+), 5 deletions(-) >=20 > diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c > index 76f7e2b..6d9394a 100644 > --- a/block/qcow2-bitmap.c > +++ b/block/qcow2-bitmap.c > @@ -901,3 +901,63 @@ out: > g_free(new_dir); > g_free(dir); > } > + > +int qcow2_dirty_bitmaps_refcounts(BlockDriverState *bs, > + BdrvCheckResult *res, > + void **refcount_table, > + int64_t *refcount_table_size) I'd rename this function to make clear that this is for checking the refcounts, e.g. to "qcow2_check_dirty_bitmaps_refcounts" or "qcow2_count_dirty_bitmaps_refcounts" or just "qcow2_check_dirty_bitmaps". Probably the last one is the best because this function should ideally perform a full check of the dirty bitmaps extension. > +{ > + BDRVQcow2State *s =3D bs->opaque; > + uint8_t *dir; > + Qcow2BitmapDirEntry *e; > + Error *local_err =3D NULL; > + > + int i; > + int ret =3D inc_refcounts(bs, res, refcount_table, refcount_table_= size, > + s->bitmap_directory_offset, > + s->bitmap_directory_size); > + if (ret < 0) { > + return ret; > + } > + > + dir =3D directory_read(bs, s->bitmap_directory_offset, > + s->bitmap_directory_size, &local_err); > + if (dir =3D=3D NULL) { > + error_report_err(local_err); I think you should increment res->corruptions here. > + return -EINVAL; > + } > + > + for_each_bitmap_dir_entry(e, dir, s->bitmap_directory_size) { > + uint64_t *bitmap_table =3D NULL; I think you should call check_constraints() here. > + > + ret =3D inc_refcounts(bs, res, refcount_table, refcount_table_= size, > + e->bitmap_table_offset, > + e->bitmap_table_size); Probably rather e->bitmap_table_size * sizeof(uint64_t). > + if (ret < 0) { > + return ret; > + } > + > + ret =3D bitmap_table_load(bs, e, &bitmap_table); > + if (ret < 0) { Again, it would make sense to increment res->corruptions here. > + return ret; > + } > + > + for (i =3D 0; i < e->bitmap_table_size; ++i) { > + uint64_t off =3D be64_to_cpu(bitmap_table[i]); > + if (off <=3D 1) { > + continue; > + } As I said in some other patch, I'd write this condition differently (with an offset mask, etc.). Also, you should check that the offset is aligned to a cluster boundary and that no unknown flags are set. > + > + ret =3D inc_refcounts(bs, res, refcount_table, refcount_ta= ble_size, > + off, s->cluster_size); > + if (ret < 0) { > + g_free(bitmap_table); > + return ret; > + } > + } > + > + g_free(bitmap_table); > + } > + > + return 0; dir is leaked here. > +} > diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c > index cbfb3fe..05bcc57 100644 > --- a/block/qcow2-refcount.c > +++ b/block/qcow2-refcount.c > @@ -1309,11 +1309,9 @@ static int realloc_refcount_array(BDRVQcow2State= *s, void **array, > * > * Modifies the number of errors in res. > */ > -static int inc_refcounts(BlockDriverState *bs, > - BdrvCheckResult *res, > - void **refcount_table, > - int64_t *refcount_table_size, > - int64_t offset, int64_t size) > +int inc_refcounts(BlockDriverState *bs, BdrvCheckResult *res, > + void **refcount_table, int64_t *refcount_table_size,= > + int64_t offset, int64_t size) First, if you make this function public, you have to give it a qcow2_ prefix. Second, if this function is public, it should have a name that makes sense. inc_refcounts() sounds as if it's the same as update_refcount() with an addend of 1. I'd rename it qcow2_inc_refcounts_imrt(), because that's probably the shortest name I can come up with (and qcow2-refcount.c explains what an IMRT is in some comment). > { > BDRVQcow2State *s =3D bs->opaque; > uint64_t start, last, cluster_offset, k, refcount; > @@ -1843,6 +1841,12 @@ static int calculate_refcounts(BlockDriverState = *bs, BdrvCheckResult *res, > return ret; > } > =20 > + /* dirty bitmaps */ > + ret =3D qcow2_dirty_bitmaps_refcounts(bs, res, refcount_table, nb_= clusters); > + if (ret < 0) { > + return ret; > + } > + > return check_refblocks(bs, res, fix, rebuild, refcount_table, nb_c= lusters); > } > =20 > diff --git a/block/qcow2.h b/block/qcow2.h > index a5e7592..0a050ea 100644 > --- a/block/qcow2.h > +++ b/block/qcow2.h > @@ -563,6 +563,9 @@ int qcow2_check_metadata_overlap(BlockDriverState *= bs, int ign, int64_t offset, > int64_t size); > int qcow2_pre_write_overlap_check(BlockDriverState *bs, int ign, int64= _t offset, > int64_t size); > +int inc_refcounts(BlockDriverState *bs, BdrvCheckResult *res, > + void **refcount_table, int64_t *refcount_table_size,= > + int64_t offset, int64_t size); > =20 > int qcow2_change_refcount_order(BlockDriverState *bs, int refcount_ord= er, > BlockDriverAmendStatusCB *status_cb, > @@ -616,6 +619,8 @@ int qcow2_read_snapshots(BlockDriverState *bs); > int qcow2_read_bitmaps(BlockDriverState *bs, Error **errp); > =20 > int qcow2_delete_bitmaps(BlockDriverState *bs); > +int qcow2_dirty_bitmaps_refcounts(BlockDriverState *bs, BdrvCheckResul= t *res, > + void **refcount_table, int64_t *refcount_table_size); Normally we try to align parameters along the opening parenthesis as long as there is enough space, and there is enough space here. Max > =20 > /* qcow2-cache.c functions */ > Qcow2Cache *qcow2_cache_create(BlockDriverState *bs, int num_tables); >=20 --4hTQH8PKVwMM9bSICoepi29iB6tfwUAN2 Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- iQEvBAEBCAAZBQJX+9cFEhxtcmVpdHpAcmVkaGF0LmNvbQAKCRD0B9sAYdXPQHPm B/9/Qi3+7kKSK3yNeDAADjKpEyvj5Waj5l+gVpf1BJ9rovjJwydTJnHGwiiVJL/C qQt0OCeaV0hQPvg/Jw7TaRpsIAfo44tDZ7hoHQgQopgjKGjmLOOzq/cXemUNxIUl MEHVYEug8yr5PWA1NU8xEt1b30Ndwj2TiEVfD+H4jBZ7d4jk74MIjBFffDZX0Wdb hrm3j+fqsthEn/tKfspa8wDCDBzghWVZP5VFvDP0v0FFT66P0UpRZbj/8+l5cS8h yocYv/1W1cGv4H5pP6wwGWXzc0d/earnIcGY6CG6dECfmE+iiK4b/mVYWkIdWNZL /bZ/8MGVfgZWg4ODWYTMqTNn =kome -----END PGP SIGNATURE----- --4hTQH8PKVwMM9bSICoepi29iB6tfwUAN2--