Re: [Qemu-devel] [PATCH v11 03/13] qmp: Add block-dirty-bitmap-add and block-dirty-bitmap-remove

[Kevin Wolf]

Am 30.01.2015 um 18:04 hat John Snow geschrieben:
> 
> 
> On 01/30/2015 09:32 AM, Kevin Wolf wrote:
> >Am 21.01.2015 um 10:34 hat Markus Armbruster geschrieben:
> >>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.
> >
> >Wasn't the conclusion last time that we would try to find a better name
> >for new commands and leave old commands alone because they are going to
> >become deprecated? That is, a combination of your first and last option?
> >
> 
> That was my impression, too: Use a new name for new commands and
> then slowly phase out the old things. This makes the new name clear
> as to what it supports (BOTH backends and nodes through a common
> namespace) to external management utilities like libvirt.
> 
> That's why I just rolled 'node-ref.'

Yes, I agree with that in principle. I called it 'node' below because
that's shorter and doesn't include type information in the name, but if
everyone preferred 'node-ref', I wouldn't have a problem with it either.

> >>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)
> >
> >Whatever name we end up using, 1. and 2. should probably use the same.
> 
> Should they? If these commands accept directly *node* names and have
> no chance of referencing a backend, maybe they should use different
> parameter names.

Note that these cases don't refer to a node, but they define/return the
name of a node. No way that could ever be a backend name, unless we
broke the code.

> >>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.
> >
> >These shouldn't be existing. As you say, we should move them to 3.
> >
> 
> Technically #3 here isn't a usage of "node-name," because... I
> didn't use node-name for these commands. Unless I am reading this
> list wrong, but it's definitely not an "existing use."
> 
> I don't have any opinions about #4; presumably that's something
> we're aiming to phase out.

Yes. Where "phasing out" simply means to extend it so that it accepts
backend names. That should in theory be an easy change, except that
@image-node-name has a name that isn't quite compatible with it...

> >>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.
> >
> >Agreed, these shouldn't be existing either.
> >
> >>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?
> >
> >If we don't have any cases where node names are allowed, but backend
> >names are not, then there is no reason to have two different names. I've
> >yet to see a reason for having commands that can accept node names, but
> >not backend names.
> >
> >It's a bit different when the command can already accept both, but uses
> >two separate arguments for it. But I think most of them will be
> >deprecated, so we can ignore them here.
> >
> >As for the naming, I'm not that sure that it's even useful to add
> >something to the field name. After all, this is really the _type_ of the
> >object, not the name. We don't have fields like 'read-only-bool' either.
> >
> >If we're more specifically looking at things that actually refer to
> >block devices, you already mentioned drive-mirrors @replaces, which is a
> >great name in my opinion. @replaces-node-ref wouldn't improve anything.
> >Likewise, blockdev-add already refers to 'file' and 'backing' instead of
> >'file-node' or 'backing-node-ref'.
> >
> >This probably means that FOO-node-{ref,name} shouldn't exist, because
> >just FOO is as good or better. The question is a bit harder where there
> >is only one node involved and we don't have a nice word to describe its
> >role for the command. This is where we used to use 'device' in the past,
> >when node-level addressing didn't exist yet. I think just 'node' would
> >be fine there.
> >
> >Kevin
> >
> 
> I'd be happy with naming things "node" (or node-ref, either is fine)
> going forward; and leaving the old commands (node-name) alone. I
> seem to recall there was a reason we didn't want to just keep using
> node-name for the new unified parameters.

It's probably a bad name when we accept backend names as well. And I'm
not completely sure, but I think we considered removing the relative
recent node-name again, which has to be probed by management tools
anyway. We can't remove 'device', which has always been there, and
keeping both as optional fields isn't really nice.

> It makes sense that if we don't keep a "this means node name ONLY"
> parameter for any command then there is no reason to make some
> distinction between that and "this parameter accepts both," but I
> think for purposes of libvirt, it is helpful to have a concrete
> distinction between versions that it can rely on.

Well, at least for new commands that doesn't matter.

> Could be mis-remembering, this whole discussion is spread out over
> months now.

Yes, remembering all the details is hard...

Kevin