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

[John Snow <jsnow@redhat.com>]

On 02/02/2015 05:10 AM, Markus Armbruster wrote:
> Kevin Wolf <kwolf@redhat.com> 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 :)
>

I can roll v12 tomorrow using parameters named "node" if that sounds 
agreeable to everyone, fixing up the other issues noted by other 
reviewers in the process.

Sound good?

--js