Re: [Qemu-block] [Qemu-devel] [PATCH 20/25] qcow2-refcount: rename inc_refcounts() and make it public

[John Snow]

On 02/14/2017 11:59 AM, Vladimir Sementsov-Ogievskiy wrote:
> This is needed for the following patch, which will introduce refcounts
> checking for qcow2 bitmaps.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> Reviewed-by: Max Reitz <address@hidden>
> ---
>  block/qcow2-refcount.c | 53 
> ++++++++++++++++++++++++++------------------------
>  block/qcow2.h          |  4 ++++
>  2 files changed, 32 insertions(+), 25 deletions(-)
> 
> diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
> index cbfb3fe..14a736d 100644
> --- a/block/qcow2-refcount.c
> +++ b/block/qcow2-refcount.c
> @@ -1309,11 +1309,10 @@ 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 qcow2_inc_refcounts_imrt(BlockDriverState *bs, BdrvCheckResult *res,
> +                             void **refcount_table,
> +                             int64_t *refcount_table_size,
> +                             int64_t offset, int64_t size)
>  {
>      BDRVQcow2State *s = bs->opaque;
>      uint64_t start, last, cluster_offset, k, refcount;
> @@ -1406,8 +1405,9 @@ static int check_refcounts_l2(BlockDriverState *bs, 
> BdrvCheckResult *res,
>              nb_csectors = ((l2_entry >> s->csize_shift) &
>                             s->csize_mask) + 1;
>              l2_entry &= s->cluster_offset_mask;
> -            ret = inc_refcounts(bs, res, refcount_table, refcount_table_size,
> -                                l2_entry & ~511, nb_csectors * 512);
> +            ret = qcow2_inc_refcounts_imrt(bs, res,
> +                                           refcount_table, 
> refcount_table_size,
> +                                           l2_entry & ~511, nb_csectors * 
> 512);
>              if (ret < 0) {
>                  goto fail;
>              }
> @@ -1445,8 +1445,9 @@ static int check_refcounts_l2(BlockDriverState *bs, 
> BdrvCheckResult *res,
>              }
>  
>              /* Mark cluster as used */
> -            ret = inc_refcounts(bs, res, refcount_table, refcount_table_size,
> -                                offset, s->cluster_size);
> +            ret = qcow2_inc_refcounts_imrt(bs, res,
> +                                           refcount_table, 
> refcount_table_size,
> +                                           offset, s->cluster_size);
>              if (ret < 0) {
>                  goto fail;
>              }
> @@ -1498,8 +1499,8 @@ static int check_refcounts_l1(BlockDriverState *bs,
>      l1_size2 = l1_size * sizeof(uint64_t);
>  
>      /* Mark L1 table as used */
> -    ret = inc_refcounts(bs, res, refcount_table, refcount_table_size,
> -                        l1_table_offset, l1_size2);
> +    ret = qcow2_inc_refcounts_imrt(bs, res, refcount_table, 
> refcount_table_size,
> +                                   l1_table_offset, l1_size2);
>      if (ret < 0) {
>          goto fail;
>      }
> @@ -1528,8 +1529,9 @@ static int check_refcounts_l1(BlockDriverState *bs,
>          if (l2_offset) {
>              /* Mark L2 table as used */
>              l2_offset &= L1E_OFFSET_MASK;
> -            ret = inc_refcounts(bs, res, refcount_table, refcount_table_size,
> -                                l2_offset, s->cluster_size);
> +            ret = qcow2_inc_refcounts_imrt(bs, res,
> +                                           refcount_table, 
> refcount_table_size,
> +                                           l2_offset, s->cluster_size);
>              if (ret < 0) {
>                  goto fail;
>              }
> @@ -1744,14 +1746,15 @@ static int check_refblocks(BlockDriverState *bs, 
> BdrvCheckResult *res,
>                  }
>  
>                  res->corruptions_fixed++;
> -                ret = inc_refcounts(bs, res, refcount_table, nb_clusters,
> -                                    offset, s->cluster_size);
> +                ret = qcow2_inc_refcounts_imrt(bs, res,
> +                                               refcount_table, nb_clusters,
> +                                               offset, s->cluster_size);
>                  if (ret < 0) {
>                      return ret;
>                  }
>                  /* No need to check whether the refcount is now greater than 
> 1:
>                   * This area was just allocated and zeroed, so it can only be
> -                 * exactly 1 after inc_refcounts() */
> +                 * exactly 1 after qcow2_inc_refcounts_imrt() */
>                  continue;
>  
>  resize_fail:
> @@ -1766,8 +1769,8 @@ resize_fail:
>          }
>  
>          if (offset != 0) {
> -            ret = inc_refcounts(bs, res, refcount_table, nb_clusters,
> -                                offset, s->cluster_size);
> +            ret = qcow2_inc_refcounts_imrt(bs, res, refcount_table, 
> nb_clusters,
> +                                           offset, s->cluster_size);
>              if (ret < 0) {
>                  return ret;
>              }
> @@ -1807,8 +1810,8 @@ static int calculate_refcounts(BlockDriverState *bs, 
> BdrvCheckResult *res,
>      }
>  
>      /* header */
> -    ret = inc_refcounts(bs, res, refcount_table, nb_clusters,
> -                        0, s->cluster_size);
> +    ret = qcow2_inc_refcounts_imrt(bs, res, refcount_table, nb_clusters,
> +                                   0, s->cluster_size);
>      if (ret < 0) {
>          return ret;
>      }
> @@ -1829,16 +1832,16 @@ static int calculate_refcounts(BlockDriverState *bs, 
> BdrvCheckResult *res,
>              return ret;
>          }
>      }
> -    ret = inc_refcounts(bs, res, refcount_table, nb_clusters,
> -                        s->snapshots_offset, s->snapshots_size);
> +    ret = qcow2_inc_refcounts_imrt(bs, res, refcount_table, nb_clusters,
> +                                   s->snapshots_offset, s->snapshots_size);
>      if (ret < 0) {
>          return ret;
>      }
>  
>      /* refcount data */
> -    ret = inc_refcounts(bs, res, refcount_table, nb_clusters,
> -                        s->refcount_table_offset,
> -                        s->refcount_table_size * sizeof(uint64_t));
> +    ret = qcow2_inc_refcounts_imrt(bs, res, refcount_table, nb_clusters,
> +                                   s->refcount_table_offset,
> +                                   s->refcount_table_size * 
> sizeof(uint64_t));
>      if (ret < 0) {
>          return ret;
>      }
> diff --git a/block/qcow2.h b/block/qcow2.h
> index 749710d..0a3708d 100644
> --- a/block/qcow2.h
> +++ b/block/qcow2.h
> @@ -543,6 +543,10 @@ 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 qcow2_inc_refcounts_imrt(BlockDriverState *bs, BdrvCheckResult *res,
> +                             void **refcount_table,
> +                             int64_t *refcount_table_size,
> +                             int64_t offset, int64_t size);
>  
>  int qcow2_change_refcount_order(BlockDriverState *bs, int refcount_order,
>                                  BlockDriverAmendStatusCB *status_cb,
> 

Mechanically safe of course. Max and Kevin will be the experts for the
implications in qcow2-land. (And Max already gave his blessings!)


Reviewed-by: John Snow <address@hidden>