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

[Markus Armbruster]

Fam Zheng <address@hidden> writes:

> On Tue, 12/02 20:06, Markus Armbruster wrote:
>> == block-core.json ==
>> 
>> * block-commit
>> 
>>   @device names a backend, @top and @base each name one of its nodes via
>>   file name matching.
>> 
>>   TODO: support specifying the two nodes via node name.
>> 
>>   Why do we need @device when a top node is specified?
>> 
>>   * We currently need the backend to set op blockers, and finding a
>>     node's backend isn't easy.  Implementation detail shows through the
>>     interface, blech.
>> 
>>   * We need to know whether the top node is the active layer.
>> 
>>     Complication: if it's shared by multiple backends, it may be the
>>     active layer in one but not the other.  Specifying the backend
>>     resolves the ambiguity.  Whether that makes sense I don't know.
>> 
>> * block-stream
>> 
>>   @device names a backend, @base names a node via file name matching.
>> 
>>   TODO: support specifying the node via node name.
>> 
>>   I think backend is inappropriate here, we should support streaming up
>>   to a node, just like block-commit supports committing down from a
>>   node.
>
> Agreed.
>
>> 
>> * block_passwd
>> * block_resize
>> 
>>   @node-name names a node.
>> 
>>   @device names a backend, and references its root node, for
>>   compatibility.
>> 
>>   Either @device or @node-name must be given.
>> 
>>   TODO: should have single mandatory parameter instead of two optionals.
>> 
>> * blockdev-snapshot-sync
>> 
>>   @node-name and @device as for block_passwd, including TODO.
>> 
>>   @snapshot-node-name defines the new node's node name.
>> 
>> * block_set_io_throttle
>> 
>>   @device names a backend.
>> 
>>   TODO: support nodes.
>> 
>>   Note: we'd like to redo throttling as a block filter node, so maybe
>>   we'll have a completely different command instead.
>
> Whether it's implemented in core block layer or as a block filter node is
> implementation detail from user's PoV, so it shouldn't matter unless we use a
> "general" block filter configuration command.

Yes.  Nevertheless, we can generalize the existing command to filters,
or create a new one.  Wait and see.

>> 
>> * blockdev-add
>> 
>>   This command defines a backend and its node tree, where sub-trees may
>>   be references to existing nodes.
>> 
>>   @id defines the backend's name.
>> 
>>   @node-name defines its root node's node name.
>> 
>>   Note: blockdev-add always defines a backend.  When you build up a
>>   backend bottom-up with several commands, you get a bunch of unwanted
>>   backends "in the middle".  I'd like to make @id optional, and omit the
>>   backend when it's missing.
>> 
>> * change-backing-file
>> 
>>   @device names a backend, @image-node-name names a node.
>> 
>>   As far as I can tell, we need the backend only to set op blockers.
>>   Implementation detail shows through the interface.
>> 
>> * drive-backup
>> 
>>   @device names a backend.
>> 
>>   Do we want to be able to back up any node, or only a backend?
>> 
>>   Note: documentation of @target sounds like it could somehow name a
>>   backend, but as far as I can tell it's always interpreted as file
>>   name.
>> 
>> * drive-mirror
>> 
>>   @device names a backend, @replaces names a node, and @node-name
>>   defines the name of the new node.
>> 
>>   Do we want to be able to mirror any node, or only a backend?
>> 
>>   Note: documentation of @target sounds like it could somehow name a
>>   backend, but as far as I can tell it's always interpreted as file
>>   name.
>> 
>>   Note: drive-mirror supports @replaces, but drive-backup doesn't.  Odd.
>
> It's only asymmetric, not odd: @target has the same data in drive-mirror, so
> "replaces" doesn't surprise @device's user. That's not true for drive-backup.

Okay, now I see.

>> * query-blockstats
>> 
>>   Returns some stats for all backends as array of BlockStats.
>>   BlockStats member @device is the backend name.  Member @stats contains
>>   the stats for the backend's root node.  Member @parent is BlockStats
>>   for the child node that is stored in BDS member file.  Member @backing
>>   is BlockStats for the chold node that is stored in BDS member
>>   backing_hd.  Stats for other children aren't returned.
>> 
>>   TODO: generalize this to the full tree, include node names.
>> 
>> * query-block-jobs
>> 
>>   Returns information on block jobs as array of BlockJobInfo.  A block
>>   job is always tied to a backend, and BlockJobInfo member @device is
>>   its name.
>> 
>>   The question whether we need a node name here is moot; see
>>   block-job-cancel above.
>> 
>
> We are not internally ready to untie block job from backend yet, once we get
> there, we should start giving names to block jobs, add support some kind of
> default naming/querying to be backward compatible.

Kevin pointed out that we can fix the interface before the
implementation.