Re: [Qemu-devel] [PATCH v12 03/17] block: Introduce bdrv_dirty_bitmap_granularity()

[Max Reitz <mreitz@redhat.com>]

On 2015-02-09 at 20:35, John Snow wrote:
> This returns the granularity (in bytes) of dirty bitmap,
> which matches the QMP interface and the existing query
> interface.
>
> Small adjustments are made to ensure that granularity-- in bytes--

I guess these should be ASCII replacements of an em dash? But they kind 
of look like decrement operators to me...

> is handled consistently as a uint64_t throughout the code.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   block.c               | 17 +++++++++++------
>   include/block/block.h |  3 ++-
>   2 files changed, 13 insertions(+), 7 deletions(-)
>
> diff --git a/block.c b/block.c
> index 1661ff9..83f411f 100644
> --- a/block.c
> +++ b/block.c
> @@ -5387,12 +5387,13 @@ void bdrv_dirty_bitmap_make_anon(BlockDriverState *bs, BdrvDirtyBitmap *bitmap)
>   }
>   
>   BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs,
> -                                          int granularity,
> +                                          uint64_t granularity,
>                                             const char *name,
>                                             Error **errp)
>   {
>       int64_t bitmap_size;
>       BdrvDirtyBitmap *bitmap;
> +    int sector_granularity;
>   
>       assert((granularity & (granularity - 1)) == 0);
>   
> @@ -5400,8 +5401,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;

I can see Coverity's screams regarding a possible overflow already... 
(but maybe it doesn't even scream and I'm just overcautious)

Whether you add an assert((granularity >> BDRV_SECTOR_BITS) <= INT_MAX) 
or not here (it does look pretty ugly and it is pretty unnecessary, I 
know) or not, and whether you do something about the decrement operators 
in the commit message or not:

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