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

[Max Reitz <mreitz@redhat.com>]

On 2015-02-11 at 13:57, John Snow wrote:
>
>
> On 02/10/2015 05:03 PM, Max Reitz wrote:
>> 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>
>
> Coverity would whine about right-shifting a value?

Not about right-shifting, but about right-shifting a uint64_t by less 
than 32 and storing it in an int.

Max

>
> In this case, right-shifting an unsigned value should be fine for all 
> cases from 0 through UINT64_MAX. It won't underflow and loop around to 
> something too big; this is safe as an integral division operation.