From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55247) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XIGfl-0006pl-2g for qemu-devel@nongnu.org; Fri, 15 Aug 2014 08:32:03 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XIGfc-0002rC-St for qemu-devel@nongnu.org; Fri, 15 Aug 2014 08:31:57 -0400 Received: from mx1.redhat.com ([209.132.183.28]:56357) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XIGfc-0002r3-KR for qemu-devel@nongnu.org; Fri, 15 Aug 2014 08:31:48 -0400 Message-ID: <53EDFDAD.4010103@redhat.com> Date: Fri, 15 Aug 2014 14:31:41 +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> In-Reply-To: <20140814120252.GG2009@irqsave.net> 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 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-dis= k >> refcounts during qemu-img check into an own function. >> >> Signed-off-by: Max Reitz >> --- >> block/qcow2-refcount.c | 91 ++++++++++++++++++++++++++++------------= ---------- >> 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 calculate_refcounts(BlockDriverStat= e *bs, BdrvCheckResult *res, >> } >> =20 >> /* >> - * Checks an image for refcount consistency. >> - * >> - * Returns 0 if no errors are found, the number of errors in case the= image is >> - * detected as corrupted, and -errno when an internal error occurred. >> + * Compares the actual reference count for each cluster in the 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 *= res, >> + BdrvCheckMode fix, int64_t *highest_clu= ster, >> + uint16_t *refcount_table, int64_t nb_cl= usters) >> { >> 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, &nb_cl= usters); >> - if (ret < 0) { >> - goto fail; >> - } >> + int64_t i; >> + int refcount1, refcount2, ret; >> =20 >> - /* 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 %" PRId6= 4 ": %s\n", >> - i, strerror(-refcount1)); >> + i, strerror(-refcount1)); > Free coding fix. > >> res->check_errors++; >> continue; >> } >> @@ -1676,11 +1652,10 @@ int qcow2_check_refcounts(BlockDriverState *bs= , BdrvCheckResult *res, >> refcount2 =3D refcount_table[i]; >> =20 >> if (refcount1 > 0 || refcount2 > 0) { >> - highest_cluster =3D i; >> + *highest_cluster =3D i; > idem. >> } >> =20 >> 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 *bs= , BdrvCheckResult *res, >> } >> =20 >> fprintf(stderr, "%s cluster %" PRId64 " refcount=3D%d re= ference=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 spurious = coding style fixes. Well, I did them on purpose to avoid an additional patch. I'll rework it=20 for v2. Max >> =20 >> if (num_fixed) { >> ret =3D update_refcount(bs, i << s->cluster_bits, 1, >> @@ -1713,6 +1688,44 @@ int qcow2_check_refcounts(BlockDriverState *bs,= BdrvCheckResult *res, >> } >> } >> } >> +} >> + >> +/* >> + * Checks an image for refcount consistency. >> + * >> + * Returns 0 if no errors are found, the number of errors in case 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, &nb_cl= usters); >> + if (ret < 0) { >> + goto fail; >> + } >> + >> + compare_refcounts(bs, res, fix, &highest_cluster, refcount_table, >> + nb_clusters); >> =20 >> /* check OFLAG_COPIED */ >> ret =3D check_oflag_copied(bs, res, fix); >> --=20 >> 2.0.3 >> >>