Kevin Wolf <address@hidden> writes:
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.
Maybe. But even if we use different names for things that can only be
node names, never backend names, 1. and 2. should use the same name,
because they're both things that can only be node names. That's what
Kevin said.
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...
Our choice for 3. affects the phasing out of 4.
Our choice for 3 is a naming convention for parameters referencing nodes
that accept both node and backend names.
If, after extending the code to accept backend names, the old names for
4. follow the naming convention for 3., we're done.
Else, we still have an interface wart. We can live with it, or we can
rename the parameter to follow the convention.
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.
Cases 1. and 2. But I'm not sure using different names there to
emphasize "backend names not possible here" would be useful.
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.
Yes, case 5.
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.
Yes, but there the type is actually bool, which makes the bool-ness
perfectly obvious.
Here, the type is string. That's why I feel a FOO-node convention could
be useful.
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.
I'm fine with just 'node'. I like it better than 'node-ref' or
'node-name'.
What sets the 'node-name' convention apart is that the existing
(FOO-)node-name already fit this convention. A quick grep finds nine
occurences, in block-core.json and event.json. Advantage if we want to
keep them, disadvantage if we want to get rid of them.
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.
Sounds like we want to get rid of them.
It makes sense that if we don't keep a "this means node name ONLY"
parameter for any command
Cases 1. and 2.
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...
Quite a hairball :)