Re: [PATCH 01/10] hbitmap: introduce HBITMAP_MAX_ORIG_SIZE

[Vladimir Sementsov-Ogievskiy]

09.10.2019 18:34, Eric Blake wrote:
> On 9/30/19 10:14 AM, Vladimir Sementsov-Ogievskiy wrote:
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> 
> A bit light on the commit message for explaining why.

If use just INT64_MAX, the comment below may be move here, accompanied by
your note about disk size limit.

> 
>> ---
>>   include/qemu/hbitmap.h | 7 +++++++
>>   util/hbitmap.c         | 2 ++
>>   2 files changed, 9 insertions(+)
>>
>> diff --git a/include/qemu/hbitmap.h b/include/qemu/hbitmap.h
>> index 1bf944ca3d..82317c5364 100644
>> --- a/include/qemu/hbitmap.h
>> +++ b/include/qemu/hbitmap.h
>> @@ -33,6 +33,13 @@ typedef struct HBitmapIter HBitmapIter;
>>    */
>>   #define HBITMAP_LEVELS         ((HBITMAP_LOG_MAX_SIZE / BITS_PER_LEVEL) + 
>> 1)
>> +/*
>> + * We have APIs which returns signed int64_t, to be able to return error.
>> + * Therefore we can't handle bitmaps with absolute size larger than
>> + * (INT64_MAX+1). Still, keep it INT64_MAX to be a bit safer.
>> + */
>> +#define HBITMAP_MAX_ORIG_SIZE INT64_MAX
> 
> That, and bitmaps represent disk images, but disk images can't exceed 
> INT64_MAX bytes (thanks to off_t being signed).  But does introducing a new 
> constant really help?
> 
>> +
>>   struct HBitmapIter {
>>       const HBitmap *hb;
>> diff --git a/util/hbitmap.c b/util/hbitmap.c
>> index 757d39e360..df192234e3 100644
>> --- a/util/hbitmap.c
>> +++ b/util/hbitmap.c
>> @@ -708,6 +708,7 @@ HBitmap *hbitmap_alloc(uint64_t size, int granularity)
>>       HBitmap *hb = g_new0(struct HBitmap, 1);
>>       unsigned i;
>> +    assert(size <= HBITMAP_MAX_ORIG_SIZE);
> 
> or can we just inline INT64_MAX here?

OK for me too.

> 
>>       hb->orig_size = size;
>>       assert(granularity >= 0 && granularity < 64);
>> @@ -738,6 +739,7 @@ void hbitmap_truncate(HBitmap *hb, uint64_t size)
>>       uint64_t num_elements = size;
>>       uint64_t old;
>> +    assert(size <= HBITMAP_MAX_ORIG_SIZE);
>>       hb->orig_size = size;
>>       /* Size comes in as logical elements, adjust for granularity. */
>>
> 


-- 
Best regards,
Vladimir