| Message ID | 1423532117-14490-4-git-send-email-jsnow@redhat.com |
|---|---|
| State | New |
| Headers | show |
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>
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-- > is handled consistently as a uint64_t throughout the code. Just asking whether you want to adjust the following, too: "int granularity" in mirror_free_init() (the granularity is stored as an 64 bit integer, but the value is limited by qmp_drive_mirror() to stay between 512 and 64M, and is defined to be a uint32_t by qapi/block-core.json). It's fine from my perspective, so my R-b stands, but it looked like it might conflict with "is handled consistently". :-) Max > Signed-off-by: John Snow <jsnow@redhat.com> > --- > block.c | 17 +++++++++++------ > include/block/block.h | 3 ++- > 2 files changed, 13 insertions(+), 7 deletions(-)
On 02/10/2015 05:13 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-- >> is handled consistently as a uint64_t throughout the code. > > Just asking whether you want to adjust the following, too: "int > granularity" in mirror_free_init() (the granularity is stored as an 64 > bit integer, but the value is limited by qmp_drive_mirror() to stay > between 512 and 64M, and is defined to be a uint32_t by > qapi/block-core.json). > > It's fine from my perspective, so my R-b stands, but it looked like it > might conflict with "is handled consistently". :-) > > Max > >> Signed-off-by: John Snow <jsnow@redhat.com> >> --- >> block.c | 17 +++++++++++------ >> include/block/block.h | 3 ++- >> 2 files changed, 13 insertions(+), 7 deletions(-) I'll take a look! Every time I change a type I have to change ten more. It is a ceaseless game of whackamole. When I inevitably need to change the QMP parameter name again, I'll fix this while I'm at it.
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? 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.
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.
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; + 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"); @@ -5409,7 +5410,7 @@ BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs, return NULL; } bitmap = g_new0(BdrvDirtyBitmap, 1); - bitmap->bitmap = hbitmap_alloc(bitmap_size, ffs(granularity) - 1); + 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; @@ -5439,8 +5440,7 @@ BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs) BlockDirtyInfo *info = g_new0(BlockDirtyInfo, 1); BlockDirtyInfoList *entry = g_new0(BlockDirtyInfoList, 1); info->count = bdrv_get_dirty_count(bs, bm); - info->granularity = - ((int64_t) BDRV_SECTOR_SIZE << hbitmap_granularity(bm->bitmap)); + info->granularity = bdrv_dirty_bitmap_granularity(bm); info->has_name = !!bm->name; info->name = g_strdup(bm->name); entry->value = info; @@ -5480,6 +5480,11 @@ uint64_t bdrv_get_default_bitmap_granularity(BlockDriverState *bs) return granularity; } +uint64_t bdrv_dirty_bitmap_granularity(BdrvDirtyBitmap *bitmap) +{ + return BDRV_SECTOR_SIZE << hbitmap_granularity(bitmap->bitmap); +} + void bdrv_dirty_iter_init(BlockDriverState *bs, BdrvDirtyBitmap *bitmap, HBitmapIter *hbi) { diff --git a/include/block/block.h b/include/block/block.h index eb58002..6366cbf 100644 --- a/include/block/block.h +++ b/include/block/block.h @@ -439,7 +439,7 @@ bool bdrv_qiov_is_aligned(BlockDriverState *bs, QEMUIOVector *qiov); struct HBitmapIter; typedef struct BdrvDirtyBitmap BdrvDirtyBitmap; BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs, - int granularity, + uint64_t granularity, const char *name, Error **errp); BdrvDirtyBitmap *bdrv_find_dirty_bitmap(BlockDriverState *bs, @@ -448,6 +448,7 @@ void bdrv_dirty_bitmap_make_anon(BlockDriverState *bs, BdrvDirtyBitmap *bitmap); void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap); BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs); uint64_t bdrv_get_default_bitmap_granularity(BlockDriverState *bs); +uint64_t bdrv_dirty_bitmap_granularity(BdrvDirtyBitmap *bitmap); int bdrv_get_dirty(BlockDriverState *bs, BdrvDirtyBitmap *bitmap, int64_t sector); void bdrv_set_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap, int64_t cur_sector, int nr_sectors);
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-- 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(-)