Re: [PATCH v3 07/19] block/dirty-bitmap: introduce bdrv_dirty_bitmap_status()

[Hanna Reitz <hreitz@redhat.com>]

On 22.12.21 18:40, Vladimir Sementsov-Ogievskiy wrote:
> Add a convenient function similar with bdrv_block_status() to get
> status of dirty bitmap.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>   include/block/dirty-bitmap.h |  2 ++
>   include/qemu/hbitmap.h       | 11 +++++++++++
>   block/dirty-bitmap.c         |  6 ++++++
>   util/hbitmap.c               | 36 ++++++++++++++++++++++++++++++++++++
>   4 files changed, 55 insertions(+)

[...]

> diff --git a/include/qemu/hbitmap.h b/include/qemu/hbitmap.h
> index 5e71b6d6f7..845fda12db 100644
> --- a/include/qemu/hbitmap.h
> +++ b/include/qemu/hbitmap.h
> @@ -340,6 +340,17 @@ bool hbitmap_next_dirty_area(const HBitmap *hb, int64_t start, int64_t end,
>                                int64_t max_dirty_count,
>                                int64_t *dirty_start, int64_t *dirty_count);
>   
> +/*
> + * bdrv_dirty_bitmap_status:
> + * @hb: The HBitmap to operate on
> + * @start: the offset to start from
> + * @end: end of requested area
> + * @is_dirty: is bitmap dirty at @offset
> + * @pnum: how many bits has same value starting from @offset
> + */
> +void hbitmap_status(const HBitmap *hb, int64_t offset, int64_t bytes,

In addition to the comment not fitting the parameter names, I also don’t 
find it ideal that the parameter names here don’t match the ones in the 
function’s definition.

I don’t have a preference between `start` or `offset` (although most 
other bitmap functions seem to prefer `start`), but I do prefer `count` 
over `bytes`, because...  Well, it’s a bit count, not a byte count, 
right?  (And from the bitmap user’s perspective, those bits might stand 
for any arbitrary unit.)

Apart from that, looks nice to me.  I am wondering a bit why this 
function doesn’t simply return the dirty bit status (like, well, the 
block-status functions do it), but I presume you simply found this 
interface to be better suited for its callers.

> +                    bool *is_dirty, int64_t *pnum);
> +
>   /**
>    * hbitmap_iter_next:
>    * @hbi: HBitmapIter to operate on.