Re: [Qemu-devel] [2.3 PATCH v7 05/10] block: Add bdrv_copy_dirty_bitmap and bdrv_reset_dirty_bitmap

[Max Reitz <mreitz@redhat.com>]

On 2014-11-25 at 20:46, John Snow wrote:
> From: Fam Zheng <famz@redhat.com>
>
> Signed-off-by: Fam Zheng <famz@redhat.com>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   block.c               | 35 +++++++++++++++++++++++++++++++----
>   include/block/block.h |  4 ++++
>   2 files changed, 35 insertions(+), 4 deletions(-)
>
> diff --git a/block.c b/block.c
> index ea4c8d8..9582550 100644
> --- a/block.c
> +++ b/block.c
> @@ -53,6 +53,8 @@
>   
>   struct BdrvDirtyBitmap {
>       HBitmap *bitmap;
> +    int64_t size;
> +    int64_t granularity;
>       char *name;
>       QLIST_ENTRY(BdrvDirtyBitmap) list;
>   };
> @@ -5311,6 +5313,26 @@ void bdrv_dirty_bitmap_make_anon(BlockDriverState *bs, BdrvDirtyBitmap *bitmap)
>       bitmap->name = NULL;
>   }
>   
> +BdrvDirtyBitmap *bdrv_copy_dirty_bitmap(BlockDriverState *bs,
> +                                        const BdrvDirtyBitmap *bitmap,
> +                                        const char *name)
> +{
> +    BdrvDirtyBitmap *new_bitmap;
> +
> +    new_bitmap = g_malloc0(sizeof(BdrvDirtyBitmap));
> +    new_bitmap->bitmap = hbitmap_copy(bitmap->bitmap);
> +    new_bitmap->size = bitmap->size;
> +    new_bitmap->granularity = bitmap->granularity;
> +    new_bitmap->name = name ? g_strdup(name) : NULL;

You changed this in patch 1 so you may want to change it here, too. I'm 
fine with it either way.

> +    QLIST_INSERT_HEAD(&bs->dirty_bitmaps, new_bitmap, list);
> +    return new_bitmap;
> +}
> +
> +void bdrv_reset_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap)
> +{
> +    hbitmap_reset(bitmap->bitmap, 0, bitmap->size);
> +}
> +
>   BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs,
>                                             int granularity,
>                                             const char *name,
> @@ -5318,6 +5340,7 @@ BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs,
>   {
>       int64_t bitmap_size;
>       BdrvDirtyBitmap *bitmap;
> +    int sector_granularity;
>   
>       assert((granularity & (granularity - 1)) == 0);
>   
> @@ -5325,8 +5348,8 @@ BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs,
>           error_setg(errp, "Bitmap already exists: %s", name);
>           return NULL;
>       }
> -    granularity >>= BDRV_SECTOR_BITS;
> -    assert(granularity);
> +    sector_granularity = granularity >> BDRV_SECTOR_BITS;
> +    assert(sector_granularity);
>       bitmap_size = bdrv_nb_sectors(bs);
>       if (bitmap_size < 0) {
>           error_setg_errno(errp, -bitmap_size, "could not get length of device");
> @@ -5334,7 +5357,9 @@ BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs,
>           return NULL;
>       }
>       bitmap = g_new0(BdrvDirtyBitmap, 1);
> -    bitmap->bitmap = hbitmap_alloc(bitmap_size, ffs(granularity) - 1);
> +    bitmap->size = bitmap_size;
> +    bitmap->granularity = granularity;
> +    bitmap->bitmap = hbitmap_alloc(bitmap->size, ffs(sector_granularity) - 1);
>       bitmap->name = g_strdup(name);
>       QLIST_INSERT_HEAD(&bs->dirty_bitmaps, bitmap, list);
>       return bitmap;
> @@ -5406,7 +5431,9 @@ int64_t bdrv_dbm_calc_def_granularity(BlockDriverState *bs)
>   int64_t bdrv_dirty_bitmap_granularity(BlockDriverState *bs,
>                                     BdrvDirtyBitmap *bitmap)
>   {
> -    return BDRV_SECTOR_SIZE << hbitmap_granularity(bitmap->bitmap);
> +    g_assert(BDRV_SECTOR_SIZE << hbitmap_granularity(bitmap->bitmap) == \
> +             bitmap->granularity);

Do you really need that backslash?

If you don't and remove it, or if you do and keep it, and with either 
"name ? g_strdup(name) : NULL" left as-is or replace by "g_strdup(name)":

Reviewed-by: Max Reitz <mreitz@redhat.com>