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

[John Snow <jsnow@redhat.com>]

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.'

>> 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.

>
>> 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.

>> 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 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.

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