From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:39591) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YDJVn-0007cv-Kx for qemu-devel@nongnu.org; Mon, 19 Jan 2015 16:05:29 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YDJVj-0007hi-T8 for qemu-devel@nongnu.org; Mon, 19 Jan 2015 16:05:27 -0500 Received: from mx1.redhat.com ([209.132.183.28]:59005) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YDJVj-0007h9-MM for qemu-devel@nongnu.org; Mon, 19 Jan 2015 16:05:23 -0500 Message-ID: <54BD7191.2060404@redhat.com> Date: Mon, 19 Jan 2015 16:05:21 -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> <87oapvnkv1.fsf@blackfin.pond.sub.org> In-Reply-To: <87oapvnkv1.fsf@blackfin.pond.sub.org> Content-Type: text/plain; charset=windows-1252; 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: Markus Armbruster Cc: kwolf@redhat.com, famz@redhat.com, qemu-devel@nongnu.org, Max Reitz , vsementsov@parallels.com, stefanha@redhat.com On 01/19/2015 05:08 AM, Markus Armbruster wrote: > John Snow writes: > >> 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. > > bdrv_lookup_bs() is an awkward interface. > > If @device is non-null, try to look up a backend (BB) named @device. If > it exists, return the backend's root node (BDS). > > Else if @node_name is non-null, try to look up a node (BDS) named > @node_name. If it exists, return it. > > Else, set this error: > > error_setg(errp, "Cannot find device=%s nor node_name=%s", > device ? device : "", > node_name ? node_name : ""); > > The error message is crap unless both device and node_name are non-null > and different. Which is never the case: we always either pass two > identical non-null arguments, or we pass a null and a non-null > argument[*]. In other words, the error message is always crap. > > In case you wonder why @device takes precedence over node_name when both > are given: makes no sense. But when both are given, they are always > identical, and since backend and node names share a name space, only one > can resolve. > > A couple of cleaner solutions come to mind: > > * Make bdrv_lookup_bs() suck less > > Assert its tacit preconditions: > > assert(device || node_name); > assert(!device || !node_name || device == node_name); > > Then make it produce a decent error: > > if (device && node_name) { > error_setg(errp, "Neither block backend nor node %s found", device); > else if (device) { > error_setg(errp, "Block backend %s not found", device); > } else if (node_name) { > error_setg(errp, "Block node %s not found", node_name); > } > > Note how the three cases mirror the three usage patterns. > > Further note that the proposed error messages deviate from the > existing practice of calling block backends "devices". Calling > everything and its dog a "device" is traditional, but it's also lazy > and confusing. End of digression. > > * Make bdrv_lookup_bs suck less by doing less: leave error_setg() to its > callers > > Drop the Error ** parameter. Callers know whether a failed lookup was > for a device name, a node name or both, and can set an appropriate > error themselves. > > I'd still assert the preconditions. > > * Replace the function by one for each of its usage patterns > > I think that's what I'd do. > > [...] > > > [*] See > https://lists.nongnu.org/archive/html/qemu-devel/2014-12/msg01298.html > I can submit a patch for making bdrv_lookup_bs "nicer," or at least its usage more clear, in a separate patch. If you really want me to fold it into this series, I'd invite you to review explicitly my usage of the parameter "node-ref" before I embark on cleaning up other interface areas. Does this naming scheme look sane to you, and fit with your general expectations? I can also add a "bdrv_lookup_noderef" function that takes only one argument, which will help enforce the "If both arguments are provided, they must be the same" paradigm. This patch (#3) covers my shot at a unified parameter, and you can see further consequences in #7, and #10 (transactions). CC'ing Eric Blake, as well, for comments on a "unified parameter" interface in general. Thanks, --js