Re: [Qemu-devel] [PATCH v2 2/9] qcow2: Factor out refcount accounting for check

[Benoît Canet]

On Fri, Aug 15, 2014 at 05:16:19PM +0200, Max Reitz wrote:
> Put the code for calculating the reference counts during qemu-img check
> into an own function.
> 
> Also, do not use g_realloc() for increasing the size of the in-memory
> refcount table, but rather g_try_realloc().
> 
> Signed-off-by: Max Reitz <address@hidden>
> ---
>  block/qcow2-refcount.c | 188 
> ++++++++++++++++++++++++++++++-------------------
>  1 file changed, 115 insertions(+), 73 deletions(-)
> 
> diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
> index d60e2fe..b3c4edd 100644
> --- a/block/qcow2-refcount.c
> +++ b/block/qcow2-refcount.c
> @@ -1496,69 +1496,15 @@ done:
>  }
>  
>  /*
> - * 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.
> + * Checks consistency of refblocks and accounts for each refblock in
> + * *refcount_table.
>   */
> -int qcow2_check_refcounts(BlockDriverState *bs, BdrvCheckResult *res,
> -                          BdrvCheckMode fix)
> +static int check_refblocks(BlockDriverState *bs, BdrvCheckResult *res,
> +                           BdrvCheckMode fix, uint16_t **refcount_table,
> +                           int64_t *nb_clusters)
>  {
>      BDRVQcowState *s = bs->opaque;
> -    int64_t size, i, highest_cluster, nb_clusters;
> -    int refcount1, refcount2;
> -    QCowSnapshot *sn;
> -    uint16_t *refcount_table;
> -    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;
> -    }
> -
> -    refcount_table = g_try_malloc0(nb_clusters * sizeof(uint16_t));
> -    if (nb_clusters && refcount_table == NULL) {
> -        res->check_errors++;
> -        return -ENOMEM;
> -    }
> -
> -    res->bfi.total_clusters =
> -        size_to_clusters(s, bs->total_sectors * BDRV_SECTOR_SIZE);
> -
> -    /* header */
> -    inc_refcounts(bs, res, refcount_table, nb_clusters,
> -        0, s->cluster_size);
> -
> -    /* current L1 table */
> -    ret = check_refcounts_l1(bs, res, refcount_table, nb_clusters,
> -                             s->l1_table_offset, s->l1_size, 
> CHECK_FRAG_INFO);
> -    if (ret < 0) {
> -        goto fail;
> -    }
> -
> -    /* snapshots */
> -    for(i = 0; i < s->nb_snapshots; i++) {
> -        sn = s->snapshots + i;
> -        ret = check_refcounts_l1(bs, res, refcount_table, nb_clusters,
> -            sn->l1_table_offset, sn->l1_size, 0);
> -        if (ret < 0) {
> -            goto fail;
> -        }
> -    }
> -    inc_refcounts(bs, res, refcount_table, nb_clusters,
> -        s->snapshots_offset, s->snapshots_size);
> -
> -    /* refcount data */
> -    inc_refcounts(bs, res, refcount_table, nb_clusters,
> -        s->refcount_table_offset,
> -        s->refcount_table_size * sizeof(uint64_t));
> +    int64_t i;
>  
>      for(i = 0; i < s->refcount_table_size; i++) {
>          uint64_t offset, cluster;
> @@ -1573,7 +1519,7 @@ int qcow2_check_refcounts(BlockDriverState *bs, 
> BdrvCheckResult *res,
>              continue;
>          }
>  
> -        if (cluster >= nb_clusters) {
> +        if (cluster >= *nb_clusters) {
>              fprintf(stderr, "ERROR refcount block %" PRId64
>                      " is outside image\n", i);
>              res->corruptions++;
> @@ -1581,14 +1527,14 @@ int qcow2_check_refcounts(BlockDriverState *bs, 
> BdrvCheckResult *res,
>          }
>  
>          if (offset != 0) {
> -            inc_refcounts(bs, res, refcount_table, nb_clusters,
> +            inc_refcounts(bs, res, *refcount_table, *nb_clusters,
>                  offset, s->cluster_size);
> -            if (refcount_table[cluster] != 1) {
> +            if ((*refcount_table)[cluster] != 1) {
>                  fprintf(stderr, "%s refcount block %" PRId64
>                      " refcount=%d\n",
>                      fix & BDRV_FIX_ERRORS ? "Repairing" :
>                                              "ERROR",
> -                    i, refcount_table[cluster]);
> +                    i, (*refcount_table)[cluster]);
>  
>                  if (fix & BDRV_FIX_ERRORS) {
>                      int64_t new_offset;
> @@ -1600,17 +1546,24 @@ int qcow2_check_refcounts(BlockDriverState *bs, 
> BdrvCheckResult *res,
>                      }
>  
>                      /* update refcounts */
> -                    if ((new_offset >> s->cluster_bits) >= nb_clusters) {
> +                    if ((new_offset >> s->cluster_bits) >= *nb_clusters) {
>                          /* increase refcount_table size if necessary */
> -                        int old_nb_clusters = nb_clusters;
> -                        nb_clusters = (new_offset >> s->cluster_bits) + 1;
> -                        refcount_table = g_realloc(refcount_table,
> -                                nb_clusters * sizeof(uint16_t));
> -                        memset(&refcount_table[old_nb_clusters], 0, 
> (nb_clusters
> -                                - old_nb_clusters) * sizeof(uint16_t));
> +                        int old_nb_clusters = *nb_clusters;
> +                        *nb_clusters = (new_offset >> s->cluster_bits) + 1;
> +
> +                        *refcount_table = g_try_realloc(*refcount_table,
> +                                *nb_clusters * sizeof(uint16_t));
> +                        if (!*refcount_table) {
> +                            res->check_errors++;
> +                            return -ENOMEM;
> +                        }
> +
> +                        memset(&(*refcount_table)[old_nb_clusters], 0,
> +                               (*nb_clusters - old_nb_clusters) *
> +                               sizeof(uint16_t));
>                      }
> -                    refcount_table[cluster]--;
> -                    inc_refcounts(bs, res, refcount_table, nb_clusters,
> +                    (*refcount_table)[cluster]--;
> +                    inc_refcounts(bs, res, *refcount_table, *nb_clusters,
>                              new_offset, s->cluster_size);
>  
>                      res->corruptions_fixed++;
> @@ -1621,6 +1574,95 @@ int qcow2_check_refcounts(BlockDriverState *bs, 
> BdrvCheckResult *res,
>          }
>      }
>  
> +    return 0;
> +}
> +
> +/*
> + * Calculates an in-memory refcount table.
> + */
> +static int calculate_refcounts(BlockDriverState *bs, BdrvCheckResult *res,
> +                               BdrvCheckMode fix, uint16_t **refcount_table,
> +                               int64_t *nb_clusters)
> +{
> +    BDRVQcowState *s = bs->opaque;
> +    QCowSnapshot *sn;
> +    int64_t i;
> +    int ret;
> +
> +    if (!*refcount_table) {
> +        *refcount_table = g_try_malloc0(*nb_clusters * sizeof(uint16_t));
> +        if (*nb_clusters && !*refcount_table) {
> +            res->check_errors++;
> +            return -ENOMEM;
> +        }
> +    }
> +
> +    /* header */
> +    inc_refcounts(bs, res, *refcount_table, *nb_clusters,
> +        0, s->cluster_size);
> +
> +    /* current L1 table */
> +    ret = check_refcounts_l1(bs, res, *refcount_table, *nb_clusters,
> +                             s->l1_table_offset, s->l1_size, 
> CHECK_FRAG_INFO);
> +    if (ret < 0) {
> +        return ret;
> +    }
> +
> +    /* snapshots */
> +    for (i = 0; i < s->nb_snapshots; i++) {
> +        sn = s->snapshots + i;
> +        ret = check_refcounts_l1(bs, res, *refcount_table, *nb_clusters,
> +            sn->l1_table_offset, sn->l1_size, 0);
> +        if (ret < 0) {
> +            return ret;
> +        }
> +    }
> +    inc_refcounts(bs, res, *refcount_table, *nb_clusters,
> +        s->snapshots_offset, s->snapshots_size);
> +
> +    /* refcount data */
> +    inc_refcounts(bs, res, *refcount_table, *nb_clusters,
> +        s->refcount_table_offset,
> +        s->refcount_table_size * sizeof(uint64_t));
> +
> +    return check_refblocks(bs, res, fix, refcount_table, nb_clusters);
> +}
> +
> +/*
> + * 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, 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;
> +    }
> +
>      /* compare ref counts */
>      for (i = 0, highest_cluster = 0; i < nb_clusters; i++) {
>          refcount1 = get_refcount(bs, i);
> -- 
> 2.0.3
> 

I tried during half an hour to find a git option (--patience etc) or a tool
(Kompare) showing me that these code motions are right.

The problem is that you extract subfunctions and move them up at the same time.

I will try again tomorow to find something making sense of this patch without
having to spell check every single character.

Best regards

Benoît