From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:37065) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YCAAF-0008Um-FC for qemu-devel@nongnu.org; Fri, 16 Jan 2015 11:54:28 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YCAAC-0002eJ-86 for qemu-devel@nongnu.org; Fri, 16 Jan 2015 11:54:27 -0500 Received: from mx1.redhat.com ([209.132.183.28]:47070) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YCAAB-0002e7-Vt for qemu-devel@nongnu.org; Fri, 16 Jan 2015 11:54:24 -0500 Message-ID: <54B9423C.7050007@redhat.com> Date: Fri, 16 Jan 2015 11:54:20 -0500 From: John Snow 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> <54B94176.20201@redhat.com> In-Reply-To: <54B94176.20201@redhat.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: quoted-printable 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: Max Reitz , qemu-devel@nongnu.org Cc: kwolf@redhat.com, vsementsov@parallels.com, famz@redhat.com, armbru@redhat.com, stefanha@redhat.com On 01/16/2015 11:51 AM, Max Reitz wrote: > 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. T= he >>>> 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 wil= l >>>> 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 hel= per >>>> 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) >=3D 0 && bdi.cluster_size !=3D 0) = { >>>> + granularity =3D MAX(4096, bdi.cluster_size); >>>> + granularity =3D MIN(65536, granularity); >>>> + } else { >>>> + granularity =3D 65536; >>>> + } >>>> + >>>> + return granularity; >>>> +} >>>> + >>>> void bdrv_dirty_iter_init(BlockDriverState *bs, >>>> BdrvDirtyBitmap *bitmap, HBitmapIter *hb= i) >>>> { >>>> 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 =3D=3D 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) >=3D 0 && bdi.cluster_size = !=3D >>>> 0) { >>>> - granularity =3D MAX(4096, bdi.cluster_size); >>>> - granularity =3D MIN(65536, granularity); >>>> - } else { >>>> - granularity =3D 65536; >>>> - } >>>> + granularity =3D bdrv_get_default_bitmap_granularity(target)= ; >>>> } >>>> assert ((granularity & (granularity - 1)) =3D=3D 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 =3D 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 b= y >>> 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 > Or a new helper designed specifically for single argument "BB -or- BDS"=20 lookups, too, that uses the name of the parameter that we eventually=20 decide upon. I'll clean up the leak for now. Thanks! >> >>>> + return NULL; >>>> + } >>>> + >>>> + /* If caller provided a BDS*, provide the result of that lookup= , >>>> too. */ >>>> + if (pbs) { >>>> + *pbs =3D bs; >>>> + } >>>> + >>>> + bitmap =3D 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 shou= ld >>> be able to confuse it. >>> >> >> No, I agree. >> >>> So with the error_setg() in the fail path for bdrv_lookup_bs() remove= d: >>> >>> Reviewed-by: Max Reitz >>> >> > --=20 =E2=80=94js