From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:36194) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YCA73-0006Ef-1i for qemu-devel@nongnu.org; Fri, 16 Jan 2015 11:51:10 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YCA6z-0001AI-Q9 for qemu-devel@nongnu.org; Fri, 16 Jan 2015 11:51:08 -0500 Received: from mx1.redhat.com ([209.132.183.28]:59540) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YCA6z-00019v-Ig for qemu-devel@nongnu.org; Fri, 16 Jan 2015 11:51:05 -0500 Message-ID: <54B94176.20201@redhat.com> Date: Fri, 16 Jan 2015 11:51:02 -0500 From: Max Reitz MIME-Version: 1.0 References: <1421080265-2228-1-git-send-email-jsnow@redhat.com> <1421080265-2228-4-git-send-email-jsnow@redhat.com> <54B93014.8010203@redhat.com> <54B940F7.7000904@redhat.com> In-Reply-To: <54B940F7.7000904@redhat.com> Content-Type: text/plain; charset=iso-8859-15; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v11 03/13] qmp: Add block-dirty-bitmap-add and block-dirty-bitmap-remove List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: John Snow , qemu-devel@nongnu.org Cc: kwolf@redhat.com, vsementsov@parallels.com, famz@redhat.com, armbru@redhat.com, stefanha@redhat.com On 2015-01-16 at 11:48, John Snow wrote: > > > On 01/16/2015 10:36 AM, Max Reitz wrote: >> On 2015-01-12 at 11:30, John Snow wrote: >>> From: Fam Zheng >>> >>> The new command pair is added to manage user created dirty bitmap. The >>> dirty bitmap's name is mandatory and must be unique for the same >>> device, >>> but different devices can have bitmaps with the same names. >>> >>> The granularity is an optional field. If it is not specified, we will >>> choose a default granularity based on the cluster size if available, >>> clamped to between 4K and 64K to mirror how the 'mirror' code was >>> already choosing granularity. If we do not have cluster size info >>> available, we choose 64K. This code has been factored out into a helper >>> shared with block/mirror. >>> >>> This patch also introduces the 'block_dirty_bitmap_lookup' helper, >>> which takes a device name and a dirty bitmap name and validates the >>> lookup, returning NULL and setting errp if there is a problem with >>> either field. This helper will be re-used in future patches in this >>> series. >>> >>> The types added to block-core.json will be re-used in future patches >>> in this series, see: >>> 'qapi: Add transaction support to block-dirty-bitmap-{add, enable, >>> disable}' >>> >>> Signed-off-by: Fam Zheng >>> Signed-off-by: John Snow >>> --- >>> block.c | 20 ++++++++++ >>> block/mirror.c | 10 +---- >>> blockdev.c | 100 >>> ++++++++++++++++++++++++++++++++++++++++++++++++++ >>> include/block/block.h | 1 + >>> qapi/block-core.json | 55 +++++++++++++++++++++++++++ >>> qmp-commands.hx | 51 +++++++++++++++++++++++++ >>> 6 files changed, 228 insertions(+), 9 deletions(-) >>> >>> diff --git a/block.c b/block.c >>> index bfeae6b..3eb77ee 100644 >>> --- a/block.c >>> +++ b/block.c >>> @@ -5417,6 +5417,26 @@ int bdrv_get_dirty(BlockDriverState *bs, >>> BdrvDirtyBitmap *bitmap, int64_t sector >>> } >>> } >>> +/** >>> + * Chooses a default granularity based on the existing cluster size, >>> + * but clamped between [4K, 64K]. Defaults to 64K in the case that >>> there >>> + * is no cluster size information available. >>> + */ >>> +uint64_t bdrv_get_default_bitmap_granularity(BlockDriverState *bs) >>> +{ >>> + BlockDriverInfo bdi; >>> + uint64_t granularity; >>> + >>> + if (bdrv_get_info(bs, &bdi) >= 0 && bdi.cluster_size != 0) { >>> + granularity = MAX(4096, bdi.cluster_size); >>> + granularity = MIN(65536, granularity); >>> + } else { >>> + granularity = 65536; >>> + } >>> + >>> + return granularity; >>> +} >>> + >>> void bdrv_dirty_iter_init(BlockDriverState *bs, >>> BdrvDirtyBitmap *bitmap, HBitmapIter *hbi) >>> { >>> diff --git a/block/mirror.c b/block/mirror.c >>> index d819952..fc545f1 100644 >>> --- a/block/mirror.c >>> +++ b/block/mirror.c >>> @@ -667,15 +667,7 @@ static void mirror_start_job(BlockDriverState >>> *bs, BlockDriverState *target, >>> MirrorBlockJob *s; >>> if (granularity == 0) { >>> - /* Choose the default granularity based on the target file's >>> cluster >>> - * size, clamped between 4k and 64k. */ >>> - BlockDriverInfo bdi; >>> - if (bdrv_get_info(target, &bdi) >= 0 && bdi.cluster_size != >>> 0) { >>> - granularity = MAX(4096, bdi.cluster_size); >>> - granularity = MIN(65536, granularity); >>> - } else { >>> - granularity = 65536; >>> - } >>> + granularity = bdrv_get_default_bitmap_granularity(target); >>> } >>> assert ((granularity & (granularity - 1)) == 0); >>> diff --git a/blockdev.c b/blockdev.c >>> index 5651a8e..95251c7 100644 >>> --- a/blockdev.c >>> +++ b/blockdev.c >>> @@ -1173,6 +1173,48 @@ out_aio_context: >>> return NULL; >>> } >>> +/** >>> + * Return a dirty bitmap (if present), after validating >>> + * the node reference and bitmap names. Returns NULL on error, >>> + * including when the BDS and/or bitmap is not found. >>> + */ >>> +static BdrvDirtyBitmap *block_dirty_bitmap_lookup(const char >>> *node_ref, >>> + const char *name, >>> + BlockDriverState >>> **pbs, >>> + Error **errp) >>> +{ >>> + BlockDriverState *bs; >>> + BdrvDirtyBitmap *bitmap; >>> + >>> + if (!node_ref) { >>> + error_setg(errp, "Node reference cannot be NULL"); >>> + return NULL; >>> + } >>> + if (!name) { >>> + error_setg(errp, "Bitmap name cannot be NULL"); >>> + return NULL; >>> + } >>> + >>> + bs = bdrv_lookup_bs(node_ref, node_ref, errp); >>> + if (!bs) { >>> + error_setg(errp, "Node reference '%s' not found", node_ref); >> >> No need to throw the (hopefully) perfectly fine Error code returned by >> bdrv_lookup_bs() away. >> > > I just wanted an error message consistent with the parameter name, in > this case. i.e., We couldn't find the "Node reference" instead of > "device" or "node name." Just trying to distinguish the fact that this > is an arbitrary reference in the error message. > > I can still remove it, but I am curious to see what Markus thinks of > the names I have chosen before I monkey with the errors too much more. Very well then, but you should clean up the error returned by bdrv_lookup_bs() (call error_free()). Feel free to keep my R-b whichever decision you'll make (as long as the error returned by bdrv_lookup_bs() is not leaked). Max > >>> + return NULL; >>> + } >>> + >>> + /* If caller provided a BDS*, provide the result of that lookup, >>> too. */ >>> + if (pbs) { >>> + *pbs = bs; >>> + } >>> + >>> + bitmap = bdrv_find_dirty_bitmap(bs, name); >>> + if (!bitmap) { >>> + error_setg(errp, "Dirty bitmap not found: %s", name); >> >> I'd use "Dirty bitmap '%s' not found", because "foo: bar" in an error >> message looks like "foo because bar" to me. But it's up to you, dirty >> bitmap names most likely don't look like error reasons so nobody should >> be able to confuse it. >> > > No, I agree. > >> So with the error_setg() in the fail path for bdrv_lookup_bs() removed: >> >> Reviewed-by: Max Reitz >> >