Re: [Qemu-devel] Can we make monitor commands identify BDS / BB by name consistently?

[Markus Armbruster]

Kevin Wolf <address@hidden> writes:

> Am 16.12.2014 um 19:12 hat Markus Armbruster geschrieben:
>> Conscious design decision: Backend (BB) and node (BDS) names share a
>> common name space.
>> 
>> Enables a convenience feature: when a command needs a node, we accept
>> either kind of name, and a backend name is resolved to its root node.
>> 
>> Should not be confused with a command that can work either on a backend
>> or on a node.  There, a backend name resolves to the backend, not its
>> root node.  Can't point to an example offhand.
>> 
>> Let's concentrate on the "command needs a node" case.
>> 
>> As we saw in my review of monitor commands, we have two different
>> conventions there.
>> 
>> * Single name
>> 
>>   Within BlockdevOptions objects (used by blockdev-add), we use a single
>>   string member, with a name that explains its role.  Actually, the
>>   member is an anonymous union of string and BlockdevOptions.
>> 
>>   Example: a BlockdevOptionsGenericFormat object (used for format "raw"
>>   and others) has a member @file that may name a backend or a node.
>> 
>>   Example: a BlockdevOptionsQcow2 object (used for "qcow2"), has a
>>   member @file as above, and a member @backing that may again name a
>>   backend or a node.
>> 
>> * Pair of names
>> 
>>   Elsewhere, command argument objects have a pair of optional members,
>>   of which exactly one must be present.  One of them must name a
>>   backend, the other must name a node.  The former is commonly called
>>   @device, the latter @node-name.
>> 
>>   Example: block_passwd parameters @device and @node-name.
>> 
>> I'd very much like some consistency here.
>> 
>> As Kevin pointed out, you can't easily change BlockdevOptions to the
>> "pair of names" convention, because an anonymous union can have only one
>> object member, and that's taken by BlockdevOptions.  If you want us to
>> adopt the "pair of names" convention, you need to come up with a way to
>> use it with BlockdevOptions.
>> 
>> I want us to adopt the "single name" convention instead.  Therefore, I
>> need to come up with a way to use it with the command argument objects
>> that currently use "pair of names".  The problems there are
>> compatibility and discoverability.
>> 
>> Four ways come to mind:
>> 
>> 1. Extend @node-name to accept backend names, deprecate @device
>> 
>>    @node-name becomes mandatory except in deprecated usage.
>>    Nevertheless, it remains optional in the schema, which is confusing.
>> 
>>    For discovery, you first have to try whether the command accepts
>>    parameter @node-name.  If no, you have a QEMU predating node names,
>>    and you need to use @device.  If yes, you need to try whether the
>>    command accepts a backend name as argument for @node-name.  Involves
>>    defining a backend.  Awkward.
>> 
>> 2. Extend @device to accept node names, deprecate @node-name
>> 
>>    @device becomes mandatory except in deprecated usage.  Nevertheless,
>>    it remains optional in the schema, which is confusing.
>
> Actually, I think we may consider that device used to be mandatory until
> recently. Management tools should be able to cope with it. If we make it
> mandatory again, we may just look like an older qemu version - would
> that be that bad?

Good point.  "Recently" is key, of course, because it permits the
argument that management tools serious about QEMU compatibility surely
cope with versions where it was still mandatory.

>>    We're stuck with a bad parameter name: @device.
>> 
>>    For discovery, you need to try whether the command accepts a node
>>    name as argument for @device.  Involves defining a node.  Almost as
>>    awkward.
>> 
>> 3. Add a new parameter, deprecate both old ones
>> 
>>    The new parameter is mandatory except in deprecated usage.
>>    Nevertheless, it's optional in the schema, which is confusing.
>> 
>>    Discovery needs to check which of the parameters the command accepts.
>>    Less awkward.
>> 
>> 4. Add a new command, deprecate the old one
>> 
>>    Quick search for commands to deprecate: block_passwd, block_resize,
>>    blockdev-snapshot-sync.  Not really bad.
>> 
>>    Discovery needs to check query-commands for the new command.  Easy.
>> 
>> Any objections to #4?
>
> The first two don't match the wanted pattern for block layer
> commands anyway, so blockdev-passwd and blockdev-resize seems like an
> improvement.

Precisely.

Dropping block_passwd without a replacement would be an even bigger
improvement in my book ;)

> blockdev-snapshot-sync should probably be replaced by a "real blockdev"
> command that adds a snapshot into the tree without actually creating the
> BDS, i.e. you need to use blockdev-add first. This solves the problem
> that currently you can't specify any bdrv_open() options when taking a
> snapshot.

Good point again.  I think we could use a systematic search for this
design flaw: review all commands that create BDSes.  I'll see what I can
do.