Re: [PATCH v2 09/20] parallels: Make mark_used() and mark_unused() global functions

[Mike Maslenkin <mike.maslenkin@gmail.com>]

On Thu, Oct 19, 2023 at 5:23 PM Alexander Ivanov
<alexander.ivanov@virtuozzo.com> wrote:
>
> We will need these functions in parallels-ext.c too. Let them be global
> functions parallels_mark_used() and parallels_mark_unused().
>
> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
> ---
>  block/parallels.c | 22 ++++++++++++----------
>  block/parallels.h |  5 +++++
>  2 files changed, 17 insertions(+), 10 deletions(-)
>
> diff --git a/block/parallels.c b/block/parallels.c
> index a22ab7f2fc..2ee2b42038 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c
> @@ -178,8 +178,8 @@ static void parallels_set_bat_entry(BDRVParallelsState *s,
>      bitmap_set(s->bat_dirty_bmap, bat_entry_off(index) / s->bat_dirty_block, 1);
>  }
>
> -static int mark_used(BlockDriverState *bs, unsigned long *bitmap,
> -                     uint32_t bitmap_size, int64_t off, uint32_t count)
> +int parallels_mark_used(BlockDriverState *bs, unsigned long *bitmap,
> +                        uint32_t bitmap_size, int64_t off, uint32_t count)
>  {
>      BDRVParallelsState *s = bs->opaque;
>      uint32_t cluster_index = host_cluster_index(s, off);
> @@ -195,8 +195,8 @@ static int mark_used(BlockDriverState *bs, unsigned long *bitmap,
>      return 0;
>  }
>
> -static int mark_unused(BlockDriverState *bs, unsigned long *bitmap,
> -                       uint32_t bitmap_size, int64_t off, uint32_t count)
> +int parallels_mark_unused(BlockDriverState *bs, unsigned long *bitmap,
> +                          uint32_t bitmap_size, int64_t off, uint32_t count)
>  {
>      BDRVParallelsState *s = bs->opaque;
>      uint32_t cluster_index = host_cluster_index(s, off);
> @@ -249,7 +249,8 @@ static int parallels_fill_used_bitmap(BlockDriverState *bs)
>              continue;
>          }
>
> -        err2 = mark_used(bs, s->used_bmap, s->used_bmap_size, host_off, 1);
> +        err2 = parallels_mark_used(bs, s->used_bmap, s->used_bmap_size,
> +                                   host_off, 1);
>          if (err2 < 0 && err == 0) {
>              err = err2;
>          }
> @@ -326,7 +327,8 @@ int64_t parallels_allocate_host_clusters(BlockDriverState *bs,
>          }
>      }
>
> -    ret = mark_used(bs, s->used_bmap, s->used_bmap_size, host_off, *clusters);
> +    ret = parallels_mark_used(bs, s->used_bmap, s->used_bmap_size,
> +                              host_off, *clusters);
>      if (ret < 0) {
>          /* Image consistency is broken. Alarm! */
>          return ret;
> @@ -393,8 +395,8 @@ allocate_clusters(BlockDriverState *bs, int64_t sector_num,
>
>          qemu_vfree(buf);
>          if (ret < 0) {
> -            mark_unused(bs, s->used_bmap, s->used_bmap_size,
> -                        host_off, to_allocate);
> +            parallels_mark_unused(bs, s->used_bmap, s->used_bmap_size,
> +                                  host_off, to_allocate);
>              return ret;
>          }
>      }
> @@ -868,7 +870,7 @@ parallels_check_duplicate(BlockDriverState *bs, BdrvCheckResult *res,
>              continue;
>          }
>
> -        ret = mark_used(bs, bitmap, bitmap_size, host_off, 1);
> +        ret = parallels_mark_used(bs, bitmap, bitmap_size, host_off, 1);
>          assert(ret != -E2BIG);
>          if (ret == 0) {
>              continue;
> @@ -928,7 +930,7 @@ parallels_check_duplicate(BlockDriverState *bs, BdrvCheckResult *res,
>           * considered, and the bitmap size doesn't change. This specifically
>           * means that -E2BIG is OK.
>           */
> -        ret = mark_used(bs, bitmap, bitmap_size, host_off, 1);
> +        ret = parallels_mark_used(bs, bitmap, bitmap_size, host_off, 1);
>          if (ret == -EBUSY) {
>              res->check_errors++;
>              goto out_repair_bat;
> diff --git a/block/parallels.h b/block/parallels.h
> index 3e4f397502..4e7aa6b80f 100644
> --- a/block/parallels.h
> +++ b/block/parallels.h
> @@ -90,6 +90,11 @@ typedef struct BDRVParallelsState {
>      Error *migration_blocker;
>  } BDRVParallelsState;
>
> +int parallels_mark_used(BlockDriverState *bs, unsigned long *bitmap,
> +                        uint32_t bitmap_size, int64_t off, uint32_t count);
> +int parallels_mark_unused(BlockDriverState *bs, unsigned long *bitmap,
> +                          uint32_t bitmap_size, int64_t off, uint32_t count);
> +
>  int64_t parallels_allocate_host_clusters(BlockDriverState *bs,
>                                           int64_t *clusters);
>
> --
> 2.34.1
>
>

Just a note: parallels_mark_unused() could be initially declared as
global just because after patch 3/20 there can be compilation warning:
warning: unused function 'mark_unused' [-Wunused-function]
:)

I do not have strong opinion about how to avoid such compilation
warning in the middle of the patch series.
The simplest and straightforward way is to declare this function as
static in patch 4.

I do not have any other objections for the series except misplaced
NULL assignment.

Regards,
Mike.