Re: [Qemu-devel] [PATCH v4 6/9] block: Add bdrv_copy_dirty_bitmap and bdrv_reset_dirty_bitmap

[Stefan Hajnoczi <stefanha@gmail.com>]

On Thu, Mar 27, 2014 at 05:09:45PM +0800, Fam Zheng wrote:
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  block.c               | 30 ++++++++++++++++++++++++++++--
>  include/block/block.h |  4 ++++
>  2 files changed, 32 insertions(+), 2 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 6b82bf0..0abc593 100644
> --- a/block.c
> +++ b/block.c
> @@ -52,6 +52,8 @@
>  
>  struct BdrvDirtyBitmap {
>      HBitmap *bitmap;
> +    int64_t size;
> +    int64_t granularity;

HBitmap *hbitmap_alloc(uint64_t size, int granularity)

Please use the same types as hbitmap_alloc() for size and granularity.
This way there's less chance of truncation, signedness, or
overflow/underflow problems later.  Code becomes messy if types are
inconsistent.

> +void bdrv_reset_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap)
> +{
> +    HBitmap *original = bitmap->bitmap;
> +
> +    bitmap->bitmap = hbitmap_alloc(bitmap->size, bitmap->granularity);
> +    hbitmap_free(original);
> +}

hbitmap_reset() exists, why allocate a new instance?  If speed is a
concern then add a special case to hbitmap_reset() for clearing the
entire bitmap quickly.  That way all callers benefit and don't have to
work around the missing functionality like you did here.