From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:52053) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XtbwZ-0001j8-LU for qemu-devel@nongnu.org; Wed, 26 Nov 2014 07:43:45 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XtbwT-00056Z-GQ for qemu-devel@nongnu.org; Wed, 26 Nov 2014 07:43:39 -0500 Received: from mx1.redhat.com ([209.132.183.28]:55533) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XtbwT-00056V-8R for qemu-devel@nongnu.org; Wed, 26 Nov 2014 07:43:33 -0500 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id sAQChWsg029062 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL) for ; Wed, 26 Nov 2014 07:43:32 -0500 Message-ID: <5475CAF1.9070603@redhat.com> Date: Wed, 26 Nov 2014 13:43:29 +0100 From: Max Reitz MIME-Version: 1.0 References: <1416944800-17919-1-git-send-email-jsnow@redhat.com> <1416944800-17919-6-git-send-email-jsnow@redhat.com> In-Reply-To: <1416944800-17919-6-git-send-email-jsnow@redhat.com> Content-Type: text/plain; charset=iso-8859-15; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [2.3 PATCH v7 05/10] block: Add bdrv_copy_dirty_bitmap and bdrv_reset_dirty_bitmap List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: John Snow , qemu-devel@nongnu.org Cc: Fam Zheng On 2014-11-25 at 20:46, John Snow wrote: > From: Fam Zheng > > Signed-off-by: Fam Zheng > Signed-off-by: John Snow > --- > block.c | 35 +++++++++++++++++++++++++++++++---- > include/block/block.h | 4 ++++ > 2 files changed, 35 insertions(+), 4 deletions(-) > > diff --git a/block.c b/block.c > index ea4c8d8..9582550 100644 > --- a/block.c > +++ b/block.c > @@ -53,6 +53,8 @@ > > struct BdrvDirtyBitmap { > HBitmap *bitmap; > + int64_t size; > + int64_t granularity; > char *name; > QLIST_ENTRY(BdrvDirtyBitmap) list; > }; > @@ -5311,6 +5313,26 @@ void bdrv_dirty_bitmap_make_anon(BlockDriverState *bs, BdrvDirtyBitmap *bitmap) > bitmap->name = NULL; > } > > +BdrvDirtyBitmap *bdrv_copy_dirty_bitmap(BlockDriverState *bs, > + const BdrvDirtyBitmap *bitmap, > + const char *name) > +{ > + BdrvDirtyBitmap *new_bitmap; > + > + new_bitmap = g_malloc0(sizeof(BdrvDirtyBitmap)); > + new_bitmap->bitmap = hbitmap_copy(bitmap->bitmap); > + new_bitmap->size = bitmap->size; > + new_bitmap->granularity = bitmap->granularity; > + new_bitmap->name = name ? g_strdup(name) : NULL; You changed this in patch 1 so you may want to change it here, too. I'm fine with it either way. > + QLIST_INSERT_HEAD(&bs->dirty_bitmaps, new_bitmap, list); > + return new_bitmap; > +} > + > +void bdrv_reset_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap) > +{ > + hbitmap_reset(bitmap->bitmap, 0, bitmap->size); > +} > + > BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs, > int granularity, > const char *name, > @@ -5318,6 +5340,7 @@ BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs, > { > int64_t bitmap_size; > BdrvDirtyBitmap *bitmap; > + int sector_granularity; > > assert((granularity & (granularity - 1)) == 0); > > @@ -5325,8 +5348,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"); > @@ -5334,7 +5357,9 @@ BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs, > return NULL; > } > bitmap = g_new0(BdrvDirtyBitmap, 1); > - bitmap->bitmap = hbitmap_alloc(bitmap_size, ffs(granularity) - 1); > + bitmap->size = bitmap_size; > + bitmap->granularity = granularity; > + 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; > @@ -5406,7 +5431,9 @@ int64_t bdrv_dbm_calc_def_granularity(BlockDriverState *bs) > int64_t bdrv_dirty_bitmap_granularity(BlockDriverState *bs, > BdrvDirtyBitmap *bitmap) > { > - return BDRV_SECTOR_SIZE << hbitmap_granularity(bitmap->bitmap); > + g_assert(BDRV_SECTOR_SIZE << hbitmap_granularity(bitmap->bitmap) == \ > + bitmap->granularity); Do you really need that backslash? If you don't and remove it, or if you do and keep it, and with either "name ? g_strdup(name) : NULL" left as-is or replace by "g_strdup(name)": Reviewed-by: Max Reitz