| Message ID | 20190930151502.7829-2-vsementsov@virtuozzo.com |
|---|---|
| State | New |
| Headers | show |
| Series | Further bitmaps improvements | expand |
On 9/30/19 10:14 AM, Vladimir Sementsov-Ogievskiy wrote: > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> A bit light on the commit message for explaining why. > --- > 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? > 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. */ >
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 <vsementsov@virtuozzo.com> > > 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. */ >> >
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 + 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); 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. */
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> --- include/qemu/hbitmap.h | 7 +++++++ util/hbitmap.c | 2 ++ 2 files changed, 9 insertions(+)