Re: [Qemu-devel] Review of monitor commands identifying BDS / BB by name

[Markus Armbruster]

Kevin Wolf <address@hidden> writes:

> Am 04.12.2014 um 20:44 hat Eric Blake geschrieben:
>> On 12/04/2014 08:56 AM, Markus Armbruster wrote:
>> 
>> > 
>> > @device is a sub-optimal name for this single parameter.  Either we
>> > accept that and move on, or we deprecate it in favor of a new parameter
>> > with a better name.  I guess the better name isn't worth that much
>> > trouble, in particular since the command name is already ugly.
>> > 
>> > When @node-name is given, @device must not be given.  So @device is
>> > mandatory *except* in the @node-name usage we retain for compatibility.
>> > Deprecate the usage.
>> > 
>> > Command documentation could then look like this:
>> > 
>> > ##
>> > # @block-resize
>> > #
>> > # Resize a block image while a guest is running.
>> > #
>> > # @device: the name of the block backend or node to resize (node names
>> > # supported since 2.3)
>> > #
>> > # @size: new image size in bytes
>> > #
>> > # Deprecated usage (since 2.3):
>> > #
>> > # @device: #optional the name of the block backend to resize
>> > #
>> > # @node-name: #optional name of the node to resize (since 2.0)
>> > #
>> > # Either @device or @node-name must be set but not both.
>> 
>> But this isn't discoverable.  You aren't changing whether any parameters
>> are optional, only enhancing the semantics of existing parameters.  How
>> is libvirt supposed to know if qemu is old (node names have to go
>> through node-name) or new (node names are preferred through device)?
>> Just blindly try the 'device' argument, and if it fails, fall back to an
>> attempt with node-name?
>> 
>> On the other hand, if 'node-name' becomes the preferred interface, then
>> libvirt has a solution: if node-name is present (it is easy during
>> up-front QMP probing to determine whether 'node-name' is a recognized
>> optional argument or an unknown argument), then always use node-name.
>> As long as libvirt always names the nodes of each device (which it
>> should be doing, but that's a patch series for another day and another
>> list), then a device lookup is never needed.  If 'node-name' is not
>> present, then only the 'device' form is supported, and libvirt can only
>> manage the top image of a backend (but can make that point obvious to
>> the end user that they should upgrade qemu for more functionality).
>
> I thought libvirt didn't use any node names yet? Then it should probably
> never try node-name, but only device.
>
> There's probably little reason to resize a non-root node anyway, so if
> you can't do that with qemu < 2.3, I don't think it would be a big
> problem.

Fair enough for block-resize, but whether similar arguments can be made
for all the the other affected commmands I can't say off hand.

I think there are three node interface generations to consider: "old",
"half-baked", "new".  In a perfect world, "half-baked" wouldn't exist,
but in this one it does.  However, I figure libvirt can treat
"half-baked" like "old" without much loss.  So this boils down to
detecting "new" reliably.  Perhaps we can even detect a command that is
available only in "new".

>> > Let's get the easy case out of the way first: a query that reports only
>> > backend properties and not node properties can return an array where
>> > each element carries a backend name, like query-block does now.
>> > 
>> > For queries that report node properties, we have a couple of options:
>> > 
>> > * Flat array with node names
>> > 
>> >   Like current query-named-block-nodes.
>> > 
>> >   Can't report on nodes without names.  Jeff's project to give all nodes
>> >   names would moot this issue.
>> 
>> I could live with this.
>> 
>> > 
>> > * Array of trees mirroring the actual node forest,
>> > 
>> >   Similar to current query-blockstats.
>> > 
>> >   Tree roots correspond to backends, and backends have names.
>> > 
>> >   Unfortunately, the nodes don't actually form a forest: node trees may
>> >   be shared.  To turn it into make a forest, we'd have to duplicate the
>> >   shared trees.
>> > 
>> > * Hybrid: array of trees, but named sub-trees are represented by name
>> > 
>> >   Like the previous one, except the recursion stops at named nodes.
>> >   Instead of including such a sub-tree, we reference it by name, then
>> >   add it to the array if it's not already there.
>> 
>> This one seems like it might be easier for avoiding the reconstruction
>> of relationships; but if management doesn't already know relationships,
>> I'm not sure it is worth the complexity.
>> 
>> > 
>> > "Flat array" seems simplest.
>> 
>> Simplest to implement.  Management can't easily reconstruct the tree,
>> but for looking up information about a known node, iterating over the
>> simpler structure will be faster than trying to find a known node in a
>> complex structure.
>
> I think what we need to add in the flat array is references (by name) to
> the child nodes.

Exactly.  Requires the nodes to have names.

> The approach is a bit complicated by the fact that we already include
> random subtrees that appeared useful in some places.

If it gets too ugly, we start over with new commands, and deprecate the
old ones.