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

[Markus Armbruster]

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 :)