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

[Kevin Wolf]

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?

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

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

> 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