| Message ID | 1423865338-8576-3-git-send-email-jsnow@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 2015-02-13 at 17:08, John Snow wrote: > We treat this field with a variety of different types everywhere > in the code. Now it's just uint32_t. > > Signed-off-by: John Snow <jsnow@redhat.com> > --- > block.c | 11 ++++++----- > block/mirror.c | 4 ++-- > include/block/block.h | 2 +- > include/block/block_int.h | 2 +- > qapi/block-core.json | 2 +- > 5 files changed, 11 insertions(+), 10 deletions(-) Considering you called this series the "$MY_NAME Fixup issue", I'm assuming this patch is the response to something I said, maybe that Coverity might complain about one right-shift of a 64 bit value with the result stored in a 32 bit integer. If so, that would have been preventable by assert((x >> 9) <= INT_MAX), so it's not that you would have to force everything to be 32 bits wide. Anyway, I'm fine with either 32 or 64 bits (2 GB maximum granularity should be good for now): Reviewed-by: Max Reitz <mreitz@redhat.com>
On 02/16/2015 02:49 PM, Max Reitz wrote: > On 2015-02-13 at 17:08, John Snow wrote: >> We treat this field with a variety of different types everywhere >> in the code. Now it's just uint32_t. >> >> Signed-off-by: John Snow <jsnow@redhat.com> >> --- >> block.c | 11 ++++++----- >> block/mirror.c | 4 ++-- >> include/block/block.h | 2 +- >> include/block/block_int.h | 2 +- >> qapi/block-core.json | 2 +- >> 5 files changed, 11 insertions(+), 10 deletions(-) > > Considering you called this series the "$MY_NAME Fixup issue", I'm > assuming this patch is the response to something I said, maybe that > Coverity might complain about one right-shift of a 64 bit value with the > result stored in a 32 bit integer. If so, that would have been > preventable by assert((x >> 9) <= INT_MAX), so it's not that you would > have to force everything to be 32 bits wide. > > Anyway, I'm fine with either 32 or 64 bits (2 GB maximum granularity > should be good for now): > > Reviewed-by: Max Reitz <mreitz@redhat.com> You mentioned the types weren't /actually/ consistent, and that did actually bother me, so I did try to go out of my way to correct it. I chose uint32_t to match the existing interface for granularity via the drive mirror command, and just whackamole'd the types until they flattened out. Doing it any other way would involve changing other interface options elsewhere, and a 2GiB granularity should seriously be sufficient, considering we currently generally limit it to under 64KiB :)
On 02/13/2015 03:08 PM, John Snow wrote: > We treat this field with a variety of different types everywhere > in the code. Now it's just uint32_t. > > Signed-off-by: John Snow <jsnow@redhat.com> > --- > block.c | 11 ++++++----- > block/mirror.c | 4 ++-- > include/block/block.h | 2 +- > include/block/block_int.h | 2 +- > qapi/block-core.json | 2 +- > 5 files changed, 11 insertions(+), 10 deletions(-) > > +++ b/qapi/block-core.json > @@ -335,7 +335,7 @@ > # Since: 1.3 > ## > { 'type': 'BlockDirtyInfo', > - 'data': {'*name': 'str', 'count': 'int', 'granularity': 'int'} } > + 'data': {'*name': 'str', 'count': 'int', 'granularity': 'uint32'} } The wire format of QMP is unchanged. Prior to this patch, a user could pass a value larger than 4G and get past initial validation, but as other code then capped the value (for example, drive-mirror caps at a maximum of 64M), we aren't changing working semantics. Narrowing a type is not always backwards-compatible, but in this case it looks fine. So: Reviewed-by: Eric Blake <eblake@redhat.com>
diff --git a/block.c b/block.c index c9c2359..6e08b26 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, + uint32_t granularity, const char *name, Error **errp) { int64_t bitmap_size; BdrvDirtyBitmap *bitmap; + uint32_t 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; @@ -5440,7 +5441,7 @@ BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs) 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)); + ((uint32_t) BDRV_SECTOR_SIZE << hbitmap_granularity(bm->bitmap)); info->has_name = !!bm->name; info->name = g_strdup(bm->name); entry->value = info; diff --git a/block/mirror.c b/block/mirror.c index f073ad7..f8aac33 100644 --- a/block/mirror.c +++ b/block/mirror.c @@ -656,7 +656,7 @@ static const BlockJobDriver commit_active_job_driver = { static void mirror_start_job(BlockDriverState *bs, BlockDriverState *target, const char *replaces, - int64_t speed, int64_t granularity, + int64_t speed, uint32_t granularity, int64_t buf_size, BlockdevOnError on_source_error, BlockdevOnError on_target_error, @@ -717,7 +717,7 @@ static void mirror_start_job(BlockDriverState *bs, BlockDriverState *target, void mirror_start(BlockDriverState *bs, BlockDriverState *target, const char *replaces, - int64_t speed, int64_t granularity, int64_t buf_size, + int64_t speed, uint32_t granularity, int64_t buf_size, MirrorSyncMode mode, BlockdevOnError on_source_error, BlockdevOnError on_target_error, BlockCompletionFunc *cb, diff --git a/include/block/block.h b/include/block/block.h index 0988b77..abdf8d6 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, + uint32_t granularity, const char *name, Error **errp); BdrvDirtyBitmap *bdrv_find_dirty_bitmap(BlockDriverState *bs, diff --git a/include/block/block_int.h b/include/block/block_int.h index 7ad1950..be99ec0 100644 --- a/include/block/block_int.h +++ b/include/block/block_int.h @@ -576,7 +576,7 @@ void commit_active_start(BlockDriverState *bs, BlockDriverState *base, */ void mirror_start(BlockDriverState *bs, BlockDriverState *target, const char *replaces, - int64_t speed, int64_t granularity, int64_t buf_size, + int64_t speed, uint32_t granularity, int64_t buf_size, MirrorSyncMode mode, BlockdevOnError on_source_error, BlockdevOnError on_target_error, BlockCompletionFunc *cb, diff --git a/qapi/block-core.json b/qapi/block-core.json index 829ba1c..39291e7 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -335,7 +335,7 @@ # Since: 1.3 ## { 'type': 'BlockDirtyInfo', - 'data': {'*name': 'str', 'count': 'int', 'granularity': 'int'} } + 'data': {'*name': 'str', 'count': 'int', 'granularity': 'uint32'} } ## # @BlockInfo:
We treat this field with a variety of different types everywhere in the code. Now it's just uint32_t. Signed-off-by: John Snow <jsnow@redhat.com> --- block.c | 11 ++++++----- block/mirror.c | 4 ++-- include/block/block.h | 2 +- include/block/block_int.h | 2 +- qapi/block-core.json | 2 +- 5 files changed, 11 insertions(+), 10 deletions(-)