From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:34765) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YLcUg-000894-F0 for qemu-devel@nongnu.org; Wed, 11 Feb 2015 13:58:39 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YLcUc-0002z3-Bb for qemu-devel@nongnu.org; Wed, 11 Feb 2015 13:58:38 -0500 Received: from mx1.redhat.com ([209.132.183.28]:60796) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YLcUc-0002yz-3t for qemu-devel@nongnu.org; Wed, 11 Feb 2015 13:58:34 -0500 Message-ID: <54DBA656.80205@redhat.com> Date: Wed, 11 Feb 2015 13:58:30 -0500 From: Max Reitz MIME-Version: 1.0 References: <1423532117-14490-1-git-send-email-jsnow@redhat.com> <1423532117-14490-4-git-send-email-jsnow@redhat.com> <54DA802C.2050101@redhat.com> <54DBA623.70507@redhat.com> In-Reply-To: <54DBA623.70507@redhat.com> Content-Type: text/plain; charset=iso-8859-15; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v12 03/17] block: Introduce bdrv_dirty_bitmap_granularity() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: John Snow , qemu-devel@nongnu.org Cc: kwolf@redhat.com, famz@redhat.com, armbru@redhat.com, vsementsov@parallels.com, stefanha@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 >>> --- >>> 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 > > 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.