John Snow <js...@redhat.com> writes: > On 01/19/2015 05:08 AM, Markus Armbruster wrote: >> John Snow <js...@redhat.com> writes: >> >>> On 01/16/2015 10:36 AM, Max Reitz wrote: >>>> On 2015-01-12 at 11:30, John Snow wrote: >>>>> From: Fam Zheng <f...@redhat.com> >>>>> >>>>> 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 <f...@redhat.com> >>>>> Signed-off-by: John Snow <js...@redhat.com> >>>>> --- >>>>> 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.
Yes, please. > 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. Follow-up patch is fine. Adding one more bad error message along the way before you fix them all doesn't bother me. > 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). Do we want to introduce a @node-ref naming convention? Currently, QMP calls parameters or members naming nodes @node-name or similar, and parameters/members naming backends @device or similar. The one place where we already accept either is called @reference in the schema, but it's a member of an anonymous union, so it's not really visible in QMP. Previously[*], we agreed (I think) to replace and deprecate the four commands that use the "pair of names" convention to identify a node. Their replacement would use the "single name" convention. The name can either be a node name or a backend name, and the latter automatically resolves to its root node. The "backend name resolves to its root node" convenience feature should be available consistently or not at all. I think the consensus it to want it consistently. Therefore, your new @node-ref is really the same as the existing @node-name, isn't it? Why a new naming convention @node-ref? Is it meant to be in addition to @node-name, or is it meant to replace it? > CC'ing Eric Blake, as well, for comments on a "unified parameter" > interface in general. Good move. [*] https://lists.nongnu.org/archive/html/qemu-devel/2014-12/msg02572.html