From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:42575) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XIHqw-0002av-Lx for qemu-devel@nongnu.org; Fri, 15 Aug 2014 09:47:40 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XIHqq-0001CF-1E for qemu-devel@nongnu.org; Fri, 15 Aug 2014 09:47:34 -0400 Received: from mx1.redhat.com ([209.132.183.28]:18301) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XIHqp-0001Aw-QK for qemu-devel@nongnu.org; Fri, 15 Aug 2014 09:47:27 -0400 Message-ID: <53EE0F69.7020702@redhat.com> Date: Fri, 15 Aug 2014 15:47:21 +0200 From: Max Reitz MIME-Version: 1.0 References: <1407963710-4942-1-git-send-email-mreitz@redhat.com> <1407963710-4942-3-git-send-email-mreitz@redhat.com> <20140814120252.GG2009@irqsave.net> <53EDFDAD.4010103@redhat.com> In-Reply-To: <53EDFDAD.4010103@redhat.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH 2/8] qcow2: Factor out refcount comparison for check List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: =?windows-1252?Q?Beno=EEt_Canet?= Cc: Kevin Wolf , qemu-devel@nongnu.org, Stefan Hajnoczi On 15.08.2014 14:31, Max Reitz wrote: > On 14.08.2014 14:02, Beno=EEt Canet wrote: >> The Wednesday 13 Aug 2014 =E0 23:01:44 (+0200), Max Reitz wrote : >>> Put the code for comparing the calculated refcounts against the on-di= sk >>> refcounts during qemu-img check into an own function. >>> >>> Signed-off-by: Max Reitz >>> --- >>> block/qcow2-refcount.c | 91=20 >>> ++++++++++++++++++++++++++++---------------------- >>> 1 file changed, 52 insertions(+), 39 deletions(-) >>> >>> diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c >>> index 9793c27..d1da8d5 100644 >>> --- a/block/qcow2-refcount.c >>> +++ b/block/qcow2-refcount.c >>> @@ -1629,46 +1629,22 @@ static int=20 >>> calculate_refcounts(BlockDriverState *bs, BdrvCheckResult *res, >>> } >>> /* >>> - * Checks an image for refcount consistency. >>> - * >>> - * Returns 0 if no errors are found, the number of errors in case=20 >>> the image is >>> - * detected as corrupted, and -errno when an internal error occurred. >>> + * Compares the actual reference count for each cluster in the=20 >>> image against the >>> + * refcount as reported by the refcount structures on-disk. >>> */ >>> -int qcow2_check_refcounts(BlockDriverState *bs, BdrvCheckResult *res= , >>> - BdrvCheckMode fix) >>> +static void compare_refcounts(BlockDriverState *bs, BdrvCheckResult=20 >>> *res, >>> + BdrvCheckMode fix, int64_t=20 >>> *highest_cluster, >>> + uint16_t *refcount_table, int64_t=20 >>> nb_clusters) >>> { >>> BDRVQcowState *s =3D bs->opaque; >>> - int64_t size, i, highest_cluster, nb_clusters; >>> - int refcount1, refcount2; >>> - uint16_t *refcount_table =3D NULL; >>> - int ret; >>> - >>> - size =3D bdrv_getlength(bs->file); >>> - if (size < 0) { >>> - res->check_errors++; >>> - return size; >>> - } >>> - >>> - nb_clusters =3D size_to_clusters(s, size); >>> - if (nb_clusters > INT_MAX) { >>> - res->check_errors++; >>> - return -EFBIG; >>> - } >>> - >>> - res->bfi.total_clusters =3D >>> - size_to_clusters(s, bs->total_sectors * BDRV_SECTOR_SIZE); >>> - >>> - ret =3D calculate_refcounts(bs, res, fix, &refcount_table,=20 >>> &nb_clusters); >>> - if (ret < 0) { >>> - goto fail; >>> - } >>> + int64_t i; >>> + int refcount1, refcount2, ret; >>> - /* compare ref counts */ >>> - for (i =3D 0, highest_cluster =3D 0; i < nb_clusters; i++) { >>> + for (i =3D 0, *highest_cluster =3D 0; i < nb_clusters; i++) { >>> refcount1 =3D get_refcount(bs, i); >>> if (refcount1 < 0) { >>> fprintf(stderr, "Can't get refcount for cluster %"=20 >>> PRId64 ": %s\n", >>> - i, strerror(-refcount1)); >>> + i, strerror(-refcount1)); >> Free coding fix. >> >>> res->check_errors++; >>> continue; >>> } >>> @@ -1676,11 +1652,10 @@ int qcow2_check_refcounts(BlockDriverState=20 >>> *bs, BdrvCheckResult *res, >>> refcount2 =3D refcount_table[i]; >>> if (refcount1 > 0 || refcount2 > 0) { >>> - highest_cluster =3D i; >>> + *highest_cluster =3D i; >> idem. >>> } >>> if (refcount1 !=3D refcount2) { >>> - >> Spurious blank line removal. >> >>> /* Check if we're allowed to fix the mismatch */ >>> int *num_fixed =3D NULL; >>> if (refcount1 > refcount2 && (fix & BDRV_FIX_LEAKS)) { >>> @@ -1690,10 +1665,10 @@ int qcow2_check_refcounts(BlockDriverState=20 >>> *bs, BdrvCheckResult *res, >>> } >>> fprintf(stderr, "%s cluster %" PRId64 " refcount=3D%d= =20 >>> reference=3D%d\n", >>> - num_fixed !=3D NULL ? "Repairing" : >>> - refcount1 < refcount2 ? "ERROR" : >>> - "Leaked", >>> - i, refcount1, refcount2); >>> + num_fixed !=3D NULL ? "Repairing" : >>> + refcount1 < refcount2 ? "ERROR" : >>> + "Leaked", >>> + i, refcount1, refcount2); >> Gratuitous coding style fix. >> >> Patch 1 have similar issues. >> I think the patchs would be smaller and cleaner without these=20 >> spurious coding style fixes. > > Well, I did them on purpose to avoid an additional patch. I'll rework=20 > it for v2. Okay, if I add an explicit patch for fixing the coding style issues, I=20 probably need to fix all of qcow2-refcount.c. But looking at it, this=20 patch would probably be rather large, so I guess I'll just keep the=20 issues and don't fix them at all. Max > Max > >>> if (num_fixed) { >>> ret =3D update_refcount(bs, i << s->cluster_bits, 1= , >>> @@ -1713,6 +1688,44 @@ int qcow2_check_refcounts(BlockDriverState=20 >>> *bs, BdrvCheckResult *res, >>> } >>> } >>> } >>> +} >>> + >>> +/* >>> + * Checks an image for refcount consistency. >>> + * >>> + * Returns 0 if no errors are found, the number of errors in case=20 >>> the image is >>> + * detected as corrupted, and -errno when an internal error occurred. >>> + */ >>> +int qcow2_check_refcounts(BlockDriverState *bs, BdrvCheckResult *res= , >>> + BdrvCheckMode fix) >>> +{ >>> + BDRVQcowState *s =3D bs->opaque; >>> + int64_t size, highest_cluster, nb_clusters; >>> + uint16_t *refcount_table =3D NULL; >>> + int ret; >>> + >>> + size =3D bdrv_getlength(bs->file); >>> + if (size < 0) { >>> + res->check_errors++; >>> + return size; >>> + } >>> + >>> + nb_clusters =3D size_to_clusters(s, size); >>> + if (nb_clusters > INT_MAX) { >>> + res->check_errors++; >>> + return -EFBIG; >>> + } >>> + >>> + res->bfi.total_clusters =3D >>> + size_to_clusters(s, bs->total_sectors * BDRV_SECTOR_SIZE); >>> + >>> + ret =3D calculate_refcounts(bs, res, fix, &refcount_table,=20 >>> &nb_clusters); >>> + if (ret < 0) { >>> + goto fail; >>> + } >>> + >>> + compare_refcounts(bs, res, fix, &highest_cluster, refcount_table= , >>> + nb_clusters); >>> /* check OFLAG_COPIED */ >>> ret =3D check_oflag_copied(bs, res, fix); >>> --=20 >>> 2.0.3 >>> >>> >