John Snow <js...@redhat.com> writes: > On 01/20/2015 03:26 AM, Markus Armbruster wrote: >> 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? > > bdrv_lookup_bs() as used in the patch, as that function exists today, > will resolve backends to root nodes, so these QMP commands will > operate with device/backend names or node-names. > >> Why a new naming convention @node-ref? Is it meant to be in addition to >> @node-name, or is it meant to replace it? > > Is it the same? It was my understanding that we didn't have a QMP > command currently that accepted /only/ nodes; from your previous mail > characterizing the existing patterns: > > "3. Node name only > > No known example." > > So this patch is /intending/ to add a command wherein you can identify > either a "node-name" or a "device," where the real goal is to obtain > any arbitrary node -- so I used a new name. > > If we want a new unified parameter in the future, we should probably > figure out what it is and start using it. I propose "node-ref." > > I *did* see that "reference" was already used for this purpose, but as > you say, the semantics are somewhat different there, so I opted for a > new name to not confuse the usages. Maybe this is what we want, maybe > it isn't: A case could be made for either case. > > I'm making my case for node-ref: > > Short for Node Reference, it's different from "Node Name" in that it > does not describe a single node's name, it's simply a reference to > one. > To me, this means that it could either be a node-name OR a > backend-name, because a backend name could be considered a reference > to the root node of that tree. > > So it seems generic-y enough to be a unified parameter.
I'm afraid I forgot much of the discussion we had before the break, and only now it's coming back, slowly. Quoting myself on naming parameters identifying nodes[*]: John Snow pointed out to me that we still haven't spelled out how this single parameter should be named. On obvious option is calling it node-name, or FOO-node-name when we have several. However, we'd then have two subtly different kinds of parameters called like that: the old ones accept *only* node names, the new ones also accept backend names, which automatically resolve to the backend's root node. Three ways to cope with that: * Find a better name. * Make the old ones accept backend names, too. Only a few, not that much work. However, there are exceptions: - blockdev-add's node-name *defines* the node name. - query-named-block-nodes's node-name *is* the node's name. * Stop worrying and embrace the inconsistency. The affected commands are headed for deprecation anyway. I think I'd go with "node" or "FOO-node" for parameters that reference nodes and accept both node names and backend names, and refrain from touching the existing node-name parameters. Let's go through existing uses of @node-name again: 1. Define a node name QMP commands blockdev-add (type BlockdevOptionsBase), drive-mirror 2. Report a node name QMP command query-named-block-nodes (type BlockDeviceInfo) 3. Node reference with backend names permitted for convenience New QMP command block-dirty-bitmap-add (type BlockDirtyBitmapAdd) and others 4. Node reference with backend names not permitted QMP commands drive-mirror @replaces, change-backing-file @image-node-name We may want to support the "backend name resolves to root node" convenience feature here, for consistency. Then this moves under 3. Note interface wart: change-backing-file additionally requires the backend owning the node. We need the backend to set op-blockers, we can't easily find it from the node, so we make the user point it out to us. 5. "Pair of names" node reference, specify exactly one QMP commands block_passwd, block_resize, blockdev-snapshot-sync We can ignore this one, because we intend to replace the commands and deprecate the old ones. If I understand you correctly, you're proposing to use @node-name or @FOO-node-name when the value must be a node name (items 1+2 and 4), and @node-ref or @FOO-node-ref where we additionally support the "backend name resolves to root node" convenience feature (item 3). Is that a fair description of your proposal? PRO: the name makes it clear when the convenience feature is supported. CON: if we eliminate 4 by supporting the convenience feature, we either create ugly exceptions to the naming convention, or rename the parameters. Opinions? >>> 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 >> > > Adding Eric back in, where'd he go? Looks like you announced the cc:, but didn't actually do it, twice %-) Let me have a try. [1] https://lists.nongnu.org/archive/html/qemu-devel/2014-12/msg03151.html