From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:51849) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bXpbe-0001FL-5F for qemu-devel@nongnu.org; Thu, 11 Aug 2016 09:01:07 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bXpbb-0004hG-Sc for qemu-devel@nongnu.org; Thu, 11 Aug 2016 09:01:05 -0400 Date: Thu, 11 Aug 2016 15:00:53 +0200 From: Kevin Wolf Message-ID: <20160811130053.GE5035@noname.redhat.com> References: <1470668720-211300-1-git-send-email-vsementsov@virtuozzo.com> <1470668720-211300-8-git-send-email-vsementsov@virtuozzo.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1470668720-211300-8-git-send-email-vsementsov@virtuozzo.com> Subject: Re: [Qemu-devel] [PATCH 07/29] qcow2-bitmap: add qcow2_bitmap_load() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Vladimir Sementsov-Ogievskiy Cc: qemu-block@nongnu.org, qemu-devel@nongnu.org, mreitz@redhat.com, armbru@redhat.com, eblake@redhat.com, jsnow@redhat.com, famz@redhat.com, den@openvz.org, stefanha@redhat.com, pbonzini@redhat.com Am 08.08.2016 um 17:04 hat Vladimir Sementsov-Ogievskiy geschrieben: > This function loads block dirty bitmap from qcow2. > > Signed-off-by: Vladimir Sementsov-Ogievskiy As you said this is really unused at the moment, I'll mostly skip this patch. Just one thing: > diff --git a/include/block/block_int.h b/include/block/block_int.h > index 1fe0fd9..2c11ad7 100644 > --- a/include/block/block_int.h > +++ b/include/block/block_int.h > @@ -224,6 +224,10 @@ struct BlockDriver { > int (*bdrv_get_info)(BlockDriverState *bs, BlockDriverInfo *bdi); > ImageInfoSpecific *(*bdrv_get_specific_info)(BlockDriverState *bs); > > + BdrvDirtyBitmap *(*bdrv_dirty_bitmap_load)(BlockDriverState *bs, > + const char *name, > + Error **errp); > + I would prefer adding BlockDriver callbacks in the same patch as the wrapper function, and keeping them separate from the first format driver implementation. The reason for this is that sometimes people want to backport the infrastructure, but not the driver change. Similar to my comment on an earlier series, this means doing patch 16 (which this hunk added) first and only then implementing the function in qcow2. This specific instance probably disappears anyway as you remove the dead code for now, but just to clarify what the preferred order is generally (and I think you're going to re-submit it later, right?) Kevin