Re: [Qemu-devel] [PATCH 2/8] qcow2: Factor out refcount comparison for check

["Benoît Canet" <benoit.canet@irqsave.net>]

The Wednesday 13 Aug 2014 à 23:01:44 (+0200), Max Reitz wrote :
> Put the code for comparing the calculated refcounts against the on-disk
> refcounts during qemu-img check into an own function.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  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(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.
> + * 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_cluster,
> +                              uint16_t *refcount_table, int64_t nb_clusters)
>  {
>      BDRVQcowState *s = bs->opaque;
> -    int64_t size, i, highest_cluster, nb_clusters;
> -    int refcount1, refcount2;
> -    uint16_t *refcount_table = NULL;
> -    int ret;
> -
> -    size = bdrv_getlength(bs->file);
> -    if (size < 0) {
> -        res->check_errors++;
> -        return size;
> -    }
> -
> -    nb_clusters = size_to_clusters(s, size);
> -    if (nb_clusters > INT_MAX) {
> -        res->check_errors++;
> -        return -EFBIG;
> -    }
> -
> -    res->bfi.total_clusters =
> -        size_to_clusters(s, bs->total_sectors * BDRV_SECTOR_SIZE);
> -
> -    ret = calculate_refcounts(bs, res, fix, &refcount_table, &nb_clusters);
> -    if (ret < 0) {
> -        goto fail;
> -    }
> +    int64_t i;
> +    int refcount1, refcount2, ret;
>  
> -    /* compare ref counts */
> -    for (i = 0, highest_cluster = 0; i < nb_clusters; i++) {
> +    for (i = 0, *highest_cluster = 0; i < nb_clusters; i++) {
>          refcount1 = get_refcount(bs, i);
>          if (refcount1 < 0) {
>              fprintf(stderr, "Can't get refcount for cluster %" 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 *bs, BdrvCheckResult *res,
>          refcount2 = refcount_table[i];
>  
>          if (refcount1 > 0 || refcount2 > 0) {
> -            highest_cluster = i;
> +            *highest_cluster = i;

idem.
>          }
>  
>          if (refcount1 != refcount2) {
> -

Spurious blank line removal.

>              /* Check if we're allowed to fix the mismatch */
>              int *num_fixed = NULL;
>              if (refcount1 > refcount2 && (fix & BDRV_FIX_LEAKS)) {
> @@ -1690,10 +1665,10 @@ int qcow2_check_refcounts(BlockDriverState *bs, BdrvCheckResult *res,
>              }
>  
>              fprintf(stderr, "%s cluster %" PRId64 " refcount=%d reference=%d\n",
> -                   num_fixed != NULL     ? "Repairing" :
> -                   refcount1 < refcount2 ? "ERROR" :
> -                                           "Leaked",
> -                   i, refcount1, refcount2);
> +                    num_fixed != 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.

>  
>              if (num_fixed) {
>                  ret = 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 = bs->opaque;
> +    int64_t size, highest_cluster, nb_clusters;
> +    uint16_t *refcount_table = NULL;
> +    int ret;
> +
> +    size = bdrv_getlength(bs->file);
> +    if (size < 0) {
> +        res->check_errors++;
> +        return size;
> +    }
> +
> +    nb_clusters = size_to_clusters(s, size);
> +    if (nb_clusters > INT_MAX) {
> +        res->check_errors++;
> +        return -EFBIG;
> +    }
> +
> +    res->bfi.total_clusters =
> +        size_to_clusters(s, bs->total_sectors * BDRV_SECTOR_SIZE);
> +
> +    ret = calculate_refcounts(bs, res, fix, &refcount_table, &nb_clusters);
> +    if (ret < 0) {
> +        goto fail;
> +    }
> +
> +    compare_refcounts(bs, res, fix, &highest_cluster, refcount_table,
> +                      nb_clusters);
>  
>      /* check OFLAG_COPIED */
>      ret = check_oflag_copied(bs, res, fix);
> -- 
> 2.0.3
> 
>