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

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

[Markus Armbruster]

= Introduction =

The block layer and its monitor commands have evolved, resulting in a
bit of a mess when it comes to referring to block layer objects in
commands.

Semantically, we refer to two different kinds of objects: backends and
nodes within backends.

Example: eject parameter @device names a backend.

Example: change-backing-file parameter @image-node-name names a node.

Backend and node names share a name space.  Names are unique.
Corollary: a name unambiguously identifies either a backend, or a node,
or nothing.

Example: blockdev-add parameter @options is a union with discriminator
"driver".  With its value is "raw", member "file" is an anonymous union.
Its string value can name either a backend or a node.

Node names are a fairly recent feature.  Before, we referenced nodes by
their file name.  Pretty schlocky.  Should be replaced by node name.

Example: block-commit parameter @base is the "file name of the backing
image to write data into".  In other words, it identifies a node.  We
should add a node name parameter, and deprecate @base.

Some commands predating the node name feature can reference only
backends even though nodes could make sense, too.  Such restrictions
should be lifted.  Others reference backends where only nodes make
sense.  Should be cleaned up.

Example: drive-mirror parameter @device names a backend.  This restricts
mirroring to backends.  If we want to support mirroring nodes, we need
to extend the command to permit referencing nodes.

Example: block_passwd parameter @device names a backend.  However,
backends aren't encryped, nodes are.  In 2.0, we cleaned this up: we
added parameter @node-name.  We kept @device for backward compatibility.
Either @device or @node-name must be given.

Note: in block_passwd, we have separate parameters for backend and node
name.  In the blockdev-add example above, we have only one, and use its
value to figure out what it is.

I find this inconsistency rather ugly.  We discussed cleaning it up in
the "BB name vs. BDS name in the QAPI schema" thread.

A backend has a tree of nodes.  We call the tree's root the backend's
root node.

For convenience, we sometimes accept a backend name when a node name is
wanted, and resolve it to the backend's root node.

Example: block_passwd again; it's how its @device parameter works.

Recent development (blockdev-add) permits nodes to be shared by multiple
backends.  Op blockers should ensure the sharing is safe.  I wouldn't
bet a nickel on this to work today.


= Block layer data structures =

A backend is represented by a BlockBackend object (short: BB).

A node is represented by a BlockDriverState object (short: BDS).

A backend (BB) has a tree of nodes (BDSes).

Two of a nodes children carry special meaning: the one stored in BDS
member file (sometimes called "parent"), and the one stored in BDS
member backing_hd (sometimes called "backing").  These used to be the
only children a node could have.

A BB always has a name.  We sometimes call it device name.  Stored in BB
member name.

A BDS may have a name.  We call it node name.  Stored in BDS member
node_name[], empty when the node has no name.

A BDS has a file name.  The actual matching of a command's file name
argument against a BDS's file name is complex, but we don't have to
worry about that here.

BBs are a fairly recent invention.  Before, a backend was represented by
a BDS with a non-empty member device_name[].


= Commands =

I'm going to discuss all QMP commands whose handlers are defined in
block*.  To make sense of it, you'll probably have to peruse the command
documentation in the schema and/or qmp-commands.hx.

When I think it's fairly obvious what needs to be done for a command, I
write it down as TODO.  Please challenge it if you think I'm wrong.

When it's not so obvious, I write it down as questions.  Answers
appreciated.

== 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-job-cancel
* block-job-complete
* block-job-pause
* block-job-resume
* block-job-set-speed

  @device names a backend.

  The question whether we need to support a node name here is moot,
  because jobs shouldn't be tied to a single backend (or node) in the
  first place.  They should have their own, independent job name.

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

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

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

* query-block

  Returns information on all backends as array of BlockInfo.  BlockInfo
  member @device is the backend name.

* query-named-block-nodes

  Returns information on all named nodes as an array of BlockDeviceInfo
  (we suck at naming).  BlockDeviceInfo member @node-name is the node
  name.

  You can't get information on nodes that don't have a node name.

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

== block.json ==

* blockdev-snapshot-delete-internal-sync
* blockdev-snapshot-internal-sync

  @device names a backend.

  Do we want to be able to snapshot any node, or only a backend?

* eject

  @device names a backend.  Appropriate, because this is really a
  backend operation.

* nbd-server-add

  @device names a backend.

  I think backend is appropriate here.  The NBD server sits on top of
  the block layer just like device models do.  It should therefore use
  the BB API.  See Max's [PATCH v2 0/6] nbd: Use BlockBackend.

* nbd-server-start
* nbd-server-stop

  No backend or node name parameters.

== qapi-schema.json ==

* transaction

  This is a wrapper around a list of transaction-capable commands.
  Thus, nothing new here.

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

[Fam Zheng]

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.

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

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

Fam

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

[Paolo Bonzini]

On 03/12/2014 06:52, Fam Zheng wrote:
>> > * drive-backup
>> > 
>> >   @device names a backend.
>> > 
>> >   Do we want to be able to back up any node, or only a backend?

If non-backend nodes are read-only, we can just copy them outside QEMU.

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

I think block-backup should be added that takes a backend for target.
It could also take a node name for the source, letting you copy any node
if you really want to.

>> > * 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?

Same as above.

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

Again, block-mirror could be added that takes a backend for @target (and
if you want to, a node name for the source).

Paolo

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

[Markus Armbruster]

Fam Zheng <famz@redhat.com> 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.

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

[Kevin Wolf]

[ CCed Benoît and Max, this is blockdev work ]
[ CCed Jeff, we're also talking about op blockers ]

Not stripping quoted text for their convenience.

Am 02.12.2014 um 20:06 hat Markus Armbruster geschrieben:
> = Introduction =
> 
> The block layer and its monitor commands have evolved, resulting in a
> bit of a mess when it comes to referring to block layer objects in
> commands.
> 
> Semantically, we refer to two different kinds of objects: backends and
> nodes within backends.
> 
> Example: eject parameter @device names a backend.
> 
> Example: change-backing-file parameter @image-node-name names a node.
> 
> Backend and node names share a name space.  Names are unique.
> Corollary: a name unambiguously identifies either a backend, or a node,
> or nothing.
> 
> Example: blockdev-add parameter @options is a union with discriminator
> "driver".  With its value is "raw", member "file" is an anonymous union.
> Its string value can name either a backend or a node.
> 
> Node names are a fairly recent feature.  Before, we referenced nodes by
> their file name.  Pretty schlocky.  Should be replaced by node name.
> 
> Example: block-commit parameter @base is the "file name of the backing
> image to write data into".  In other words, it identifies a node.  We
> should add a node name parameter, and deprecate @base.
> 
> Some commands predating the node name feature can reference only
> backends even though nodes could make sense, too.  Such restrictions
> should be lifted.  Others reference backends where only nodes make
> sense.  Should be cleaned up.
> 
> Example: drive-mirror parameter @device names a backend.  This restricts
> mirroring to backends.  If we want to support mirroring nodes, we need
> to extend the command to permit referencing nodes.
> 
> Example: block_passwd parameter @device names a backend.  However,
> backends aren't encryped, nodes are.  In 2.0, we cleaned this up: we
> added parameter @node-name.  We kept @device for backward compatibility.
> Either @device or @node-name must be given.
> 
> Note: in block_passwd, we have separate parameters for backend and node
> name.  In the blockdev-add example above, we have only one, and use its
> value to figure out what it is.
> 
> I find this inconsistency rather ugly.  We discussed cleaning it up in
> the "BB name vs. BDS name in the QAPI schema" thread.
> 
> A backend has a tree of nodes.  We call the tree's root the backend's
> root node.
> 
> For convenience, we sometimes accept a backend name when a node name is
> wanted, and resolve it to the backend's root node.
> 
> Example: block_passwd again; it's how its @device parameter works.
> 
> Recent development (blockdev-add) permits nodes to be shared by multiple
> backends.  Op blockers should ensure the sharing is safe.  I wouldn't
> bet a nickel on this to work today.
> 
> 
> = Block layer data structures =
> 
> A backend is represented by a BlockBackend object (short: BB).
> 
> A node is represented by a BlockDriverState object (short: BDS).
> 
> A backend (BB) has a tree of nodes (BDSes).
> 
> Two of a nodes children carry special meaning: the one stored in BDS
> member file (sometimes called "parent"), and the one stored in BDS
> member backing_hd (sometimes called "backing").  These used to be the
> only children a node could have.
> 
> A BB always has a name.  We sometimes call it device name.  Stored in BB
> member name.
> 
> A BDS may have a name.  We call it node name.  Stored in BDS member
> node_name[], empty when the node has no name.
> 
> A BDS has a file name.  The actual matching of a command's file name
> argument against a BDS's file name is complex, but we don't have to
> worry about that here.
> 
> BBs are a fairly recent invention.  Before, a backend was represented by
> a BDS with a non-empty member device_name[].
> 
> 
> = Commands =
> 
> I'm going to discuss all QMP commands whose handlers are defined in
> block*.  To make sense of it, you'll probably have to peruse the command
> documentation in the schema and/or qmp-commands.hx.
> 
> When I think it's fairly obvious what needs to be done for a command, I
> write it down as TODO.  Please challenge it if you think I'm wrong.
> 
> When it's not so obvious, I write it down as questions.  Answers
> appreciated.
> 
> == 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.

It doesn't.

The real requirement for the commit job (for inactive layers) is that
base...top (I'm using git-rev-parse syntax for describing nodes and
subtrees from here on) is read-only for the duration of the commit
operation. For committing the active layer, we use a mirror job
internally, which works for images that are written during the
operation.

In theory, the mirror should work in all cases, it's just less efficient
(and the implementation of the completion code which reconfigures the
tree would have to be changed). Jeff, is this correct?

What commit should be doing:
- Check whether it can block (as in op blockers) writes to top, i.e. no
  other user is currently requiring the ability to write
- If it can, block writes on base...top and start a commit job
- If it can't, block writes on base...top^ and start a mirror job
- If it can't block at least base...top^, fail

This automatically solves the problem of multiple parents, as long as
these parents advertise correctly which op blocker capabilities they are
using (no, they don't, and we don't have the infrastructure for them to
do so yet - but I think it's becoming clearer what it would have to look
like).

> * block-job-cancel
> * block-job-complete
> * block-job-pause
> * block-job-resume
> * block-job-set-speed
> 
>   @device names a backend.
> 
>   The question whether we need to support a node name here is moot,
>   because jobs shouldn't be tied to a single backend (or node) in the
>   first place.  They should have their own, independent job name.

Correct.

TODO: add a @name argument to commands starting block jobs
TODO: add a @job-name argument to these job management commands and
      deprecate @device

How to get rid of bs->job to actually allow multiple jobs on one BDS and
make the job IDs useful is a separate question that is harder to answer.

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

Yes. I think Benoît suggested this before. I don't remember if he already
coded up something.

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

How is this "fairly obvious what needs to be done"? We can't get rid of
any field because of compatibility. Do you mean this:

TODO: Make @device accept node names as well

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

Yes, this makes sense, as we discussed privately a while ago.

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

Once we have the new op blockers, we'll make @device optional then and
completely ignore its value.

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

I can't think of a reasonable use case for backing up non-roots because
the only thing that would write to them are other block jobs. So you
could backup the old snapshot contents while committing to it.
Usefulness questionable.

That said, while there's no urgent need for it, it would be nicer to
allow all operations that can safely be performed, and this is one of
them. Might fall out almost natually once we have op blockers, so that
we really only need to add a @node-name option.

But that's something for later.

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

We probably want to mirror any node (Benoît has a use case for this with
replacing broken quorum children).

> * query-block
> 
>   Returns information on all backends as array of BlockInfo.  BlockInfo
>   member @device is the backend name.
> 
> * query-named-block-nodes
> 
>   Returns information on all named nodes as an array of BlockDeviceInfo
>   (we suck at naming).  BlockDeviceInfo member @node-name is the node
>   name.
> 
>   You can't get information on nodes that don't have a node name.
> 
> * 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.

Do we want the same thing for query-block?

> * 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.
> 
> == block.json ==
> 
> * blockdev-snapshot-delete-internal-sync
> * blockdev-snapshot-internal-sync
> 
>   @device names a backend.
> 
>   Do we want to be able to snapshot any node, or only a backend?

Probably only a backend. At least until someone comes up with a
reasonable use case. Internal snapshots and backing files don't mix very
well anyway.

> * eject
> 
>   @device names a backend.  Appropriate, because this is really a
>   backend operation.
> 
> * nbd-server-add
> 
>   @device names a backend.
> 
>   I think backend is appropriate here.  The NBD server sits on top of
>   the block layer just like device models do.  It should therefore use
>   the BB API.  See Max's [PATCH v2 0/6] nbd: Use BlockBackend.

Agreed.

> * nbd-server-start
> * nbd-server-stop
> 
>   No backend or node name parameters.
> 
> == qapi-schema.json ==
> 
> * transaction
> 
>   This is a wrapper around a list of transaction-capable commands.
>   Thus, nothing new here.

Surprisingly few really hard questions.

Kevin

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 732 bytes --]

On 12/03/2014 03:30 AM, Kevin Wolf wrote:
> [ CCed Benoît and Max, this is blockdev work ]
> [ CCed Jeff, we're also talking about op blockers ]
> 
> Not stripping quoted text for their convenience.

I still intend to go through this mail in more detail, but off of a
quick glance, I see you missed a command:

qapi-schema.json:
* change
  @device (sometimes) names a backend, with the further restriction that
no backend can be named 'vnc'

TODO: add new commands that de-multiplex this stupidity.  'change' is
not extensible, and management should not be using it once the new
commands are in place


-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 539 bytes --]

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

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 12/03/2014 03:30 AM, Kevin Wolf wrote:
>> [ CCed Benoît and Max, this is blockdev work ]
>> [ CCed Jeff, we're also talking about op blockers ]
>> 
>> Not stripping quoted text for their convenience.
>
> I still intend to go through this mail in more detail, but off of a
> quick glance, I see you missed a command:
>
> qapi-schema.json:
> * change
>   @device (sometimes) names a backend, with the further restriction that
> no backend can be named 'vnc'

Missed because its handler isn't in block*.  I'll double-check by
examining callers functions monitor commands use to find BBs and BDSes.

> TODO: add new commands that de-multiplex this stupidity.  'change' is
> not extensible, and management should not be using it once the new
> commands are in place

Done: replacement for "change vnc password PASSWORD", namely
set_password and change-vnc-password.  No idea why we added two
commands.

Still missing: replacements for "change vnc DISPLAY" and "change DEVICE
FILENAME [FORMAT]".

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

[Markus Armbruster]

Markus Armbruster <armbru@redhat.com> writes:

> Eric Blake <eblake@redhat.com> writes:
>
>> On 12/03/2014 03:30 AM, Kevin Wolf wrote:
>>> [ CCed Benoît and Max, this is blockdev work ]
>>> [ CCed Jeff, we're also talking about op blockers ]
>>> 
>>> Not stripping quoted text for their convenience.
>>
>> I still intend to go through this mail in more detail, but off of a
>> quick glance, I see you missed a command:
>>
>> qapi-schema.json:
>> * change
>>   @device (sometimes) names a backend, with the further restriction that
>> no backend can be named 'vnc'
>
> Missed because its handler isn't in block*.  I'll double-check by
> examining callers functions monitor commands use to find BBs and BDSes.

Done, couldn't find anything else.

[...]

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

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> [ CCed Benoît and Max, this is blockdev work ]
> [ CCed Jeff, we're also talking about op blockers ]
>
> Not stripping quoted text for their convenience.
>
> Am 02.12.2014 um 20:06 hat Markus Armbruster geschrieben:
>> = Introduction =
>> 
>> The block layer and its monitor commands have evolved, resulting in a
>> bit of a mess when it comes to referring to block layer objects in
>> commands.
>> 
>> Semantically, we refer to two different kinds of objects: backends and
>> nodes within backends.
>> 
>> Example: eject parameter @device names a backend.
>> 
>> Example: change-backing-file parameter @image-node-name names a node.
>> 
>> Backend and node names share a name space.  Names are unique.
>> Corollary: a name unambiguously identifies either a backend, or a node,
>> or nothing.
>> 
>> Example: blockdev-add parameter @options is a union with discriminator
>> "driver".  With its value is "raw", member "file" is an anonymous union.
>> Its string value can name either a backend or a node.
>> 
>> Node names are a fairly recent feature.  Before, we referenced nodes by
>> their file name.  Pretty schlocky.  Should be replaced by node name.
>> 
>> Example: block-commit parameter @base is the "file name of the backing
>> image to write data into".  In other words, it identifies a node.  We
>> should add a node name parameter, and deprecate @base.
>> 
>> Some commands predating the node name feature can reference only
>> backends even though nodes could make sense, too.  Such restrictions
>> should be lifted.  Others reference backends where only nodes make
>> sense.  Should be cleaned up.
>> 
>> Example: drive-mirror parameter @device names a backend.  This restricts
>> mirroring to backends.  If we want to support mirroring nodes, we need
>> to extend the command to permit referencing nodes.
>> 
>> Example: block_passwd parameter @device names a backend.  However,
>> backends aren't encryped, nodes are.  In 2.0, we cleaned this up: we
>> added parameter @node-name.  We kept @device for backward compatibility.
>> Either @device or @node-name must be given.
>> 
>> Note: in block_passwd, we have separate parameters for backend and node
>> name.  In the blockdev-add example above, we have only one, and use its
>> value to figure out what it is.
>> 
>> I find this inconsistency rather ugly.  We discussed cleaning it up in
>> the "BB name vs. BDS name in the QAPI schema" thread.
>> 
>> A backend has a tree of nodes.  We call the tree's root the backend's
>> root node.
>> 
>> For convenience, we sometimes accept a backend name when a node name is
>> wanted, and resolve it to the backend's root node.
>> 
>> Example: block_passwd again; it's how its @device parameter works.
>> 
>> Recent development (blockdev-add) permits nodes to be shared by multiple
>> backends.  Op blockers should ensure the sharing is safe.  I wouldn't
>> bet a nickel on this to work today.
>> 
>> 
>> = Block layer data structures =
>> 
>> A backend is represented by a BlockBackend object (short: BB).
>> 
>> A node is represented by a BlockDriverState object (short: BDS).
>> 
>> A backend (BB) has a tree of nodes (BDSes).
>> 
>> Two of a nodes children carry special meaning: the one stored in BDS
>> member file (sometimes called "parent"), and the one stored in BDS
>> member backing_hd (sometimes called "backing").  These used to be the
>> only children a node could have.
>> 
>> A BB always has a name.  We sometimes call it device name.  Stored in BB
>> member name.
>> 
>> A BDS may have a name.  We call it node name.  Stored in BDS member
>> node_name[], empty when the node has no name.
>> 
>> A BDS has a file name.  The actual matching of a command's file name
>> argument against a BDS's file name is complex, but we don't have to
>> worry about that here.
>> 
>> BBs are a fairly recent invention.  Before, a backend was represented by
>> a BDS with a non-empty member device_name[].
>> 
>> 
>> = Commands =
>> 
>> I'm going to discuss all QMP commands whose handlers are defined in
>> block*.  To make sense of it, you'll probably have to peruse the command
>> documentation in the schema and/or qmp-commands.hx.
>> 
>> When I think it's fairly obvious what needs to be done for a command, I
>> write it down as TODO.  Please challenge it if you think I'm wrong.
>> 
>> When it's not so obvious, I write it down as questions.  Answers
>> appreciated.
>> 
>> == 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.
>
> It doesn't.
>
> The real requirement for the commit job (for inactive layers) is that
> base...top (I'm using git-rev-parse syntax for describing nodes and
> subtrees from here on) is read-only for the duration of the commit
> operation. For committing the active layer, we use a mirror job
> internally, which works for images that are written during the
> operation.
>
> In theory, the mirror should work in all cases, it's just less efficient
> (and the implementation of the completion code which reconfigures the
> tree would have to be changed). Jeff, is this correct?
>
> What commit should be doing:
> - Check whether it can block (as in op blockers) writes to top, i.e. no
>   other user is currently requiring the ability to write
> - If it can, block writes on base...top and start a commit job
> - If it can't, block writes on base...top^ and start a mirror job
> - If it can't block at least base...top^, fail
>
> This automatically solves the problem of multiple parents, as long as
> these parents advertise correctly which op blocker capabilities they are
> using (no, they don't, and we don't have the infrastructure for them to
> do so yet - but I think it's becoming clearer what it would have to look
> like).

Makes sense.

>> * block-job-cancel
>> * block-job-complete
>> * block-job-pause
>> * block-job-resume
>> * block-job-set-speed
>> 
>>   @device names a backend.
>> 
>>   The question whether we need to support a node name here is moot,
>>   because jobs shouldn't be tied to a single backend (or node) in the
>>   first place.  They should have their own, independent job name.
>
> Correct.
>
> TODO: add a @name argument to commands starting block jobs
> TODO: add a @job-name argument to these job management commands and
>       deprecate @device
>
> How to get rid of bs->job to actually allow multiple jobs on one BDS and
> make the job IDs useful is a separate question that is harder to answer.

Good point: we can fix the interface before we fix the implementation.

>> * 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.
>
> Yes. I think Benoît suggested this before. I don't remember if he already
> coded up something.
>
>> * 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.
>
> How is this "fairly obvious what needs to be done"? We can't get rid of
> any field because of compatibility. Do you mean this:
>
> TODO: Make @device accept node names as well

Obviousness is subjective.

I like to think about a nice, clean interface first, and worry about
compatibility second.  Compatibility may force us to compromise on
cleanliness.  I feel this helps finding the least unclean interface.

Furthermore, I like to deprecate unwanted interfaces.  Advertize the
clean core, not the compatibility gunk.

Let's try this approach here.

I think we can agree that the clean interface is "single mandatory
parameter".  Ironically, that's what we had until we screwed it up in
2.0.

@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.
#
# Since: 0.14.0

>> * 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.
>> 
>> * 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.
>
> Yes, this makes sense, as we discussed privately a while ago.
>
>> * 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.
>
> Once we have the new op blockers, we'll make @device optional then and
> completely ignore its value.
>
>> * 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.
>
> I can't think of a reasonable use case for backing up non-roots because
> the only thing that would write to them are other block jobs. So you
> could backup the old snapshot contents while committing to it.
> Usefulness questionable.
>
> That said, while there's no urgent need for it, it would be nicer to
> allow all operations that can safely be performed, and this is one of
> them. Might fall out almost natually once we have op blockers, so that
> we really only need to add a @node-name option.
>
> But that's something for later.
>
>> * 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.
>
> We probably want to mirror any node (Benoît has a use case for this with
> replacing broken quorum children).
>
>> * query-block
>> 
>>   Returns information on all backends as array of BlockInfo.  BlockInfo
>>   member @device is the backend name.
>> 
>> * query-named-block-nodes
>> 
>>   Returns information on all named nodes as an array of BlockDeviceInfo
>>   (we suck at naming).  BlockDeviceInfo member @node-name is the node
>>   name.
>> 
>>   You can't get information on nodes that don't have a node name.
>> 
>> * 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.
>
> Do we want the same thing for query-block?

There are a couple of ways to skin the query-cat.

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.

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

"Flat array" seems simplest.

>> * 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.
>> 
>> == block.json ==
>> 
>> * blockdev-snapshot-delete-internal-sync
>> * blockdev-snapshot-internal-sync
>> 
>>   @device names a backend.
>> 
>>   Do we want to be able to snapshot any node, or only a backend?
>
> Probably only a backend. At least until someone comes up with a
> reasonable use case. Internal snapshots and backing files don't mix very
> well anyway.
>
>> * eject
>> 
>>   @device names a backend.  Appropriate, because this is really a
>>   backend operation.
>> 
>> * nbd-server-add
>> 
>>   @device names a backend.
>> 
>>   I think backend is appropriate here.  The NBD server sits on top of
>>   the block layer just like device models do.  It should therefore use
>>   the BB API.  See Max's [PATCH v2 0/6] nbd: Use BlockBackend.
>
> Agreed.
>
>> * nbd-server-start
>> * nbd-server-stop
>> 
>>   No backend or node name parameters.
>> 
>> == qapi-schema.json ==
>> 
>> * transaction
>> 
>>   This is a wrapper around a list of transaction-capable commands.
>>   Thus, nothing new here.
>
> Surprisingly few really hard questions.

Good :)

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 3855 bytes --]

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


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

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 539 bytes --]

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

[Kevin Wolf]

[-- Attachment #1: Type: text/plain, Size: 4475 bytes --]

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.

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

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

Kevin

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

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

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> 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.

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

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> 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)?

Good point.

> Just blindly try the 'device' argument, and if it fails, fall back to an
> attempt with node-name?

Works, but "try interfaces one after the other until you find one that
works" is a rather lame discovery technique.

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

If we deprecate @device instead of @node-name, we make the appropriate
(non-deprecated) use of backend names rather than node names hard to
probe.

Command argument introspection could help only if it carried
"deprecated" flags.  Might be a good idea anyway, but command
introspection is one of those nice-to-haves we can't seem to deliver.

A possible alternative is our common way to cheat at probing: when
probing for A is hard, probe for B, and assume support for B implies
support for A.

My guess that a "better name [than @device for the single parameter]
isn't worth that much trouble" needs to be reevaluated with
discoverability in mind.  Yes, it's troublesome, but it's also easily
discoverable.

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

Tree reconstruction is possible only if all nodes have names, and the
array elements refer to their children by name.

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

[Kevin Wolf]

Am 05.12.2014 um 10:34 hat Markus Armbruster geschrieben:
> Eric Blake <eblake@redhat.com> writes:
> 
> > 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)?
> 
> Good point.
> 
> > Just blindly try the 'device' argument, and if it fails, fall back to an
> > attempt with node-name?
> 
> Works, but "try interfaces one after the other until you find one that
> works" is a rather lame discovery technique.

As long as we don't have introspection, it's the only one we have.

> > 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).
> 
> If we deprecate @device instead of @node-name, we make the appropriate
> (non-deprecated) use of backend names rather than node names hard to
> probe.
> 
> Command argument introspection could help only if it carried
> "deprecated" flags.  Might be a good idea anyway, but command
> introspection is one of those nice-to-haves we can't seem to deliver.
> 
> A possible alternative is our common way to cheat at probing: when
> probing for A is hard, probe for B, and assume support for B implies
> support for A.
> 
> My guess that a "better name [than @device for the single parameter]
> isn't worth that much trouble" needs to be reevaluated with
> discoverability in mind.  Yes, it's troublesome, but it's also easily
> discoverable.

Still requires trying the new argument and then falling back to @device/
@node-name.

But as long as libvirt doesn't support the node-name interface yet
anyway, I think this discussion is mostly moot.

Kevin

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

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 05.12.2014 um 10:34 hat Markus Armbruster geschrieben:
>> Eric Blake <eblake@redhat.com> writes:
>> 
>> > 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)?
>> 
>> Good point.
>> 
>> > Just blindly try the 'device' argument, and if it fails, fall back to an
>> > attempt with node-name?
>> 
>> Works, but "try interfaces one after the other until you find one that
>> works" is a rather lame discovery technique.
>
> As long as we don't have introspection, it's the only one we have.

We have query-commands.  Lets you check for presence of commands, but no
more.

>> > 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).
>> 
>> If we deprecate @device instead of @node-name, we make the appropriate
>> (non-deprecated) use of backend names rather than node names hard to
>> probe.
>> 
>> Command argument introspection could help only if it carried
>> "deprecated" flags.  Might be a good idea anyway, but command
>> introspection is one of those nice-to-haves we can't seem to deliver.
>> 
>> A possible alternative is our common way to cheat at probing: when
>> probing for A is hard, probe for B, and assume support for B implies
>> support for A.
>> 
>> My guess that a "better name [than @device for the single parameter]
>> isn't worth that much trouble" needs to be reevaluated with
>> discoverability in mind.  Yes, it's troublesome, but it's also easily
>> discoverable.
>
> Still requires trying the new argument and then falling back to @device/
> @node-name.

Yes, but you only have to probe for "parameter is accepted", and not for
"parameter is accepted and both backend and node names work".

> But as long as libvirt doesn't support the node-name interface yet
> anyway, I think this discussion is mostly moot.

Tend to agree for this specific instance.

[Qemu-devel] Can we make monitor commands identify BDS / BB by name consistently? (was: Review of monitor commands identifying BDS / BB by name)

[Markus Armbruster]

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.

   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?

Re: [Qemu-devel] Can we make monitor commands identify BDS / BB by name consistently? (was: Review of monitor commands identifying BDS / BB by name)

[Kevin Wolf]

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?

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

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.

Kevin

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

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> 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.

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

[Markus Armbruster]

Markus Armbruster <armbru@redhat.com> writes:

> 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.
[Text on how to solve them snipped...]

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.

Opinions?

[Qemu-devel] Review of ways to create BDSes (was: Review of monitor commands identifying BDS / BB by name)

[Markus Armbruster]

= Introduction =

BDSes can be opened in various ways.  Some of them don't provide for
user configuration.  Some of them should.

Example: -drive format=qcow2,file=foo.qcow2 where foo.qcow2 has a raw
backing file foo.raw.  This creates the the following tree of BDSes:

            (qcow2,foo.qcow2)
              /            \
    (file,foo.qcow2)    (raw,foo.raw)
                            |
                       (file,foo.raw)

Before Kevin added driver-specific options, -drive let you configure
basically just the root.  Configuration for the others was inferred from
the root's configuration and the images.  Driver-specific options let
you configure all the nodes.  Defaults are still inferred as before.

Example: blockdev-snapshot-sync provides only a small subset of the full
configuration options for the BDS it creates.  Could be fixed by
duplicating the full options, i.e. blockdev-add's.  But a command that
just snapshots and leaves BDS creation to blockdev-add would be cleaner.

This is a systematic review of all the ways you can open BDSes in qemu
proper, i.e. not in qemu-{img,io,nbd}.  I tracked them down by following
the call chains leading to bdrv_open().


= QMP Commands =

* block-commit

  I figure this clones the @base BDS initially, and replaces it by the
  clone finally.  Is support for user configuration for this clone
  wanted?

* blockdev-add

  Boils down to a bdrv_open(), which is discussed in the next section.

* blockdev-snapshot-sync

  Creates a new image and a new BDS with only a few configuration
  options: @snapshot-file (the filename), @snapshot-node-name,
  @format=qcow2, @mode.  Conflates three operations: create image, open
  image, snapshot.  I guess we want to replace it by a basic snapshot
  operation that can be used with with blockdev-add and some command to
  create images.

* change

  Closes, then opens a BDS with just two configuration options: @target
  (the filename) and @arg (the format).  Needs replacing.

* drive-backup

  Similar to blockdev-snapshot-sync, except the filename is called
  @target, and the node name can't be configured.  I guess we want to
  replace it by a basic backup operation.

* drive-mirror

  Similar to blockdev-snapshot-sync, except the filename is called
  @target, and the node name @node-name.  I guess we want to replace it
  by a basic mirror operation.

* transaction

  This is a wrapper around a list of transaction-capable commands.
  Thus, nothing new here.


= Generic block layer =

bdrv_open() opens a BDS and possibly children "file" and "backing"
according to its configuration.

Subtypes of BlockdevOptionsGenericFormat have configuration for "file".

Subtypes of BlockdevOptionsGenericCOWFormat additionally have
configuration for "backing" (defaults to "infer from COW image").

bdrv_open() can additionally splice in a QCOW2 BDS to implement
snapshot=on.  No way to configure, but that's okay, because it's a
convenience feature, and to configure you simply do the splicing
explicitly.


= Block driver methods =

== bdrv_create() ==

The BDSes created here are all internal temporaries of the method, hence
user configuration isn't needed.  Correct?

== bdrv_file_open() ==

* quorum_open()

  Creates / connects to its children according to configuration in
  BlockdevOptionsQuorum.

* blkdebug_open()

  Creates / connects to its children according to configuration in
  BlockdevOptionsBlkdebug.

* blkverify_open()

  Creates / connects to its children according to configuration in
  BlockdevOptionsBlkverify.

* vvfat's enable_write_target()

  You don't want to know.

== bdrv_open() ==

* vmdk_open()

  Creates BDSes for its extents, configuration inferred from images.
  Looks like this needs work.


= Xen =

blk_connect() calls bdrv_open() under a /* setup via xenbus */ heading.
Looks like backward compatibility crap to me.

Re: [Qemu-devel] Review of ways to create BDSes (was: Review of monitor commands identifying BDS / BB by name)

[Kevin Wolf]

Am 18.12.2014 um 16:25 hat Markus Armbruster geschrieben:
> = Introduction =
> 
> BDSes can be opened in various ways.  Some of them don't provide for
> user configuration.  Some of them should.
> 
> Example: -drive format=qcow2,file=foo.qcow2 where foo.qcow2 has a raw
> backing file foo.raw.  This creates the the following tree of BDSes:
> 
>             (qcow2,foo.qcow2)
>               /            \
>     (file,foo.qcow2)    (raw,foo.raw)
>                             |
>                        (file,foo.raw)
> 
> Before Kevin added driver-specific options, -drive let you configure
> basically just the root.  Configuration for the others was inferred from
> the root's configuration and the images.  Driver-specific options let
> you configure all the nodes.  Defaults are still inferred as before.
> 
> Example: blockdev-snapshot-sync provides only a small subset of the full
> configuration options for the BDS it creates.  Could be fixed by
> duplicating the full options, i.e. blockdev-add's.  But a command that
> just snapshots and leaves BDS creation to blockdev-add would be cleaner.
> 
> This is a systematic review of all the ways you can open BDSes in qemu
> proper, i.e. not in qemu-{img,io,nbd}.  I tracked them down by following
> the call chains leading to bdrv_open().

Possibly also interesting for a followup: All the ways you can reopen
BDSes with different options/flags.

> = QMP Commands =
> 
> * block-commit
> 
>   I figure this clones the @base BDS initially, and replaces it by the
>   clone finally.  Is support for user configuration for this clone
>   wanted?

I don't think such a clone exists. Data is directly committed to @base.
The command looks to me as if it could be okay.

> * blockdev-add
> 
>   Boils down to a bdrv_open(), which is discussed in the next section.
> 
> * blockdev-snapshot-sync
> 
>   Creates a new image and a new BDS with only a few configuration
>   options: @snapshot-file (the filename), @snapshot-node-name,
>   @format=qcow2, @mode.  Conflates three operations: create image, open
>   image, snapshot.  I guess we want to replace it by a basic snapshot
>   operation that can be used with with blockdev-add and some command to
>   create images.

Yes. We should have called this one drive-snapshot, it fits better in
the drive-* family of commands. We can call the real blockdev-style
command blockdev-snapshot - it is still synchronous technically, but it
doesn't do anything like bdrv_open() that could be blocking.

> * change
> 
>   Closes, then opens a BDS with just two configuration options: @target
>   (the filename) and @arg (the format).  Needs replacing.

Max (added to CC) is working on it.

> * drive-backup
> 
>   Similar to blockdev-snapshot-sync, except the filename is called
>   @target, and the node name can't be configured.  I guess we want to
>   replace it by a basic backup operation.
> 
> * drive-mirror
> 
>   Similar to blockdev-snapshot-sync, except the filename is called
>   @target, and the node name @node-name.  I guess we want to replace it
>   by a basic mirror operation.

Yes. We called these drive-* instead of blockdev-* intentionally, so
that the latter names would be free for operations working on existing
BDSes.

> * transaction
> 
>   This is a wrapper around a list of transaction-capable commands.
>   Thus, nothing new here.
> 
> 
> = Generic block layer =
> 
> bdrv_open() opens a BDS and possibly children "file" and "backing"
> according to its configuration.
> 
> Subtypes of BlockdevOptionsGenericFormat have configuration for "file".
> 
> Subtypes of BlockdevOptionsGenericCOWFormat additionally have
> configuration for "backing" (defaults to "infer from COW image").
> 
> bdrv_open() can additionally splice in a QCOW2 BDS to implement
> snapshot=on.  No way to configure, but that's okay, because it's a
> convenience feature, and to configure you simply do the splicing
> explicitly.
> 
> 
> = Block driver methods =
> 
> == bdrv_create() ==
> 
> The BDSes created here are all internal temporaries of the method, hence
> user configuration isn't needed.  Correct?

Filenames ought to be enough for everyone. Not.

But at the moment all the callers can't deal with non-filename
specifications of the image location, so that's a larger problem.

> == bdrv_file_open() ==
> 
> * quorum_open()
> 
>   Creates / connects to its children according to configuration in
>   BlockdevOptionsQuorum.
> 
> * blkdebug_open()
> 
>   Creates / connects to its children according to configuration in
>   BlockdevOptionsBlkdebug.
> 
> * blkverify_open()
> 
>   Creates / connects to its children according to configuration in
>   BlockdevOptionsBlkverify.
> 
> * vvfat's enable_write_target()
> 
>   You don't want to know.

This would have been an interesting one for a change. ;-)

> == bdrv_open() ==
> 
> * vmdk_open()
> 
>   Creates BDSes for its extents, configuration inferred from images.
>   Looks like this needs work.

Very much so.

> = Xen =
> 
> blk_connect() calls bdrv_open() under a /* setup via xenbus */ heading.
> Looks like backward compatibility crap to me.

Are you sure? But Xen is another "you don't want to know" for me.

Kevin

Re: [Qemu-devel] Review of ways to create BDSes

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 18.12.2014 um 16:25 hat Markus Armbruster geschrieben:
>> = Introduction =
>> 
>> BDSes can be opened in various ways.  Some of them don't provide for
>> user configuration.  Some of them should.
>> 
>> Example: -drive format=qcow2,file=foo.qcow2 where foo.qcow2 has a raw
>> backing file foo.raw.  This creates the the following tree of BDSes:
>> 
>>             (qcow2,foo.qcow2)
>>               /            \
>>     (file,foo.qcow2)    (raw,foo.raw)
>>                             |
>>                        (file,foo.raw)
>> 
>> Before Kevin added driver-specific options, -drive let you configure
>> basically just the root.  Configuration for the others was inferred from
>> the root's configuration and the images.  Driver-specific options let
>> you configure all the nodes.  Defaults are still inferred as before.
>> 
>> Example: blockdev-snapshot-sync provides only a small subset of the full
>> configuration options for the BDS it creates.  Could be fixed by
>> duplicating the full options, i.e. blockdev-add's.  But a command that
>> just snapshots and leaves BDS creation to blockdev-add would be cleaner.
>> 
>> This is a systematic review of all the ways you can open BDSes in qemu
>> proper, i.e. not in qemu-{img,io,nbd}.  I tracked them down by following
>> the call chains leading to bdrv_open().
>
> Possibly also interesting for a followup: All the ways you can reopen
> BDSes with different options/flags.

Can do "all the ways you can reopen BDSes".  Finding out whether and how
options or flags can be changed in each case would be too much for one
go, I'm afraid.

>> = QMP Commands =
>> 
>> * block-commit
>> 
>>   I figure this clones the @base BDS initially, and replaces it by the
>>   clone finally.  Is support for user configuration for this clone
>>   wanted?
>
> I don't think such a clone exists. Data is directly committed to @base.
> The command looks to me as if it could be okay.

Here's what made me speculate about clones.  Call chain:

    qmp_block_commit()
        commit_active_start()
            mirror_start_job(), passing commit_active_job_driver

On completion:

    block_job_complete()
        mirror_complete(), through commit_active_job_driver.complete
            bdrv_open_backing_file()
                bdrv_open()

I now see that my reading wasn't correct.  But I can't see offhand what
exactly happens here.  Can you enlighten me?

>> * blockdev-add
>> 
>>   Boils down to a bdrv_open(), which is discussed in the next section.
>> 
>> * blockdev-snapshot-sync
>> 
>>   Creates a new image and a new BDS with only a few configuration
>>   options: @snapshot-file (the filename), @snapshot-node-name,
>>   @format=qcow2, @mode.  Conflates three operations: create image, open
>>   image, snapshot.  I guess we want to replace it by a basic snapshot
>>   operation that can be used with with blockdev-add and some command to
>>   create images.
>
> Yes. We should have called this one drive-snapshot, it fits better in
> the drive-* family of commands. We can call the real blockdev-style
> command blockdev-snapshot - it is still synchronous technically, but it
> doesn't do anything like bdrv_open() that could be blocking.

Yes.

>> * change
>> 
>>   Closes, then opens a BDS with just two configuration options: @target
>>   (the filename) and @arg (the format).  Needs replacing.
>
> Max (added to CC) is working on it.
>
>> * drive-backup
>> 
>>   Similar to blockdev-snapshot-sync, except the filename is called
>>   @target, and the node name can't be configured.  I guess we want to
>>   replace it by a basic backup operation.
>> 
>> * drive-mirror
>> 
>>   Similar to blockdev-snapshot-sync, except the filename is called
>>   @target, and the node name @node-name.  I guess we want to replace it
>>   by a basic mirror operation.
>
> Yes. We called these drive-* instead of blockdev-* intentionally, so
> that the latter names would be free for operations working on existing
> BDSes.
>
>> * transaction
>> 
>>   This is a wrapper around a list of transaction-capable commands.
>>   Thus, nothing new here.
>> 
>> 
>> = Generic block layer =
>> 
>> bdrv_open() opens a BDS and possibly children "file" and "backing"
>> according to its configuration.
>> 
>> Subtypes of BlockdevOptionsGenericFormat have configuration for "file".
>> 
>> Subtypes of BlockdevOptionsGenericCOWFormat additionally have
>> configuration for "backing" (defaults to "infer from COW image").
>> 
>> bdrv_open() can additionally splice in a QCOW2 BDS to implement
>> snapshot=on.  No way to configure, but that's okay, because it's a
>> convenience feature, and to configure you simply do the splicing
>> explicitly.
>> 
>> 
>> = Block driver methods =
>> 
>> == bdrv_create() ==
>> 
>> The BDSes created here are all internal temporaries of the method, hence
>> user configuration isn't needed.  Correct?
>
> Filenames ought to be enough for everyone. Not.
>
> But at the moment all the callers can't deal with non-filename
> specifications of the image location, so that's a larger problem.

Moreover, the pattern "create, then open" is silly, because create does
open; close internally.  Badly chosen abstraction.

>> == bdrv_file_open() ==
>> 
>> * quorum_open()
>> 
>>   Creates / connects to its children according to configuration in
>>   BlockdevOptionsQuorum.
>> 
>> * blkdebug_open()
>> 
>>   Creates / connects to its children according to configuration in
>>   BlockdevOptionsBlkdebug.
>> 
>> * blkverify_open()
>> 
>>   Creates / connects to its children according to configuration in
>>   BlockdevOptionsBlkverify.
>> 
>> * vvfat's enable_write_target()
>> 
>>   You don't want to know.
>
> This would have been an interesting one for a change. ;-)

Fishing for a cringeworthy Friday afternoon story?

>> == bdrv_open() ==
>> 
>> * vmdk_open()
>> 
>>   Creates BDSes for its extents, configuration inferred from images.
>>   Looks like this needs work.
>
> Very much so.
>
>> = Xen =
>> 
>> blk_connect() calls bdrv_open() under a /* setup via xenbus */ heading.
>> Looks like backward compatibility crap to me.
>
> Are you sure? But Xen is another "you don't want to know" for me.

It's Xen, of course I'm not sure!

Re: [Qemu-devel] Review of ways to create BDSes

[Markus Armbruster]

Markus Armbruster <armbru@redhat.com> writes:

> = Introduction =
>
> BDSes can be opened in various ways.  Some of them don't provide for
> user configuration.  Some of them should.
>
> Example: -drive format=qcow2,file=foo.qcow2 where foo.qcow2 has a raw
> backing file foo.raw.  This creates the the following tree of BDSes:
>
>             (qcow2,foo.qcow2)
>               /            \
>     (file,foo.qcow2)    (raw,foo.raw)
>                             |
>                        (file,foo.raw)
>
> Before Kevin added driver-specific options, -drive let you configure
> basically just the root.  Configuration for the others was inferred from
> the root's configuration and the images.  Driver-specific options let
> you configure all the nodes.  Defaults are still inferred as before.
>
> Example: blockdev-snapshot-sync provides only a small subset of the full
> configuration options for the BDS it creates.  Could be fixed by
> duplicating the full options, i.e. blockdev-add's.  But a command that
> just snapshots and leaves BDS creation to blockdev-add would be cleaner.
>
> This is a systematic review of all the ways you can open BDSes in qemu
> proper, i.e. not in qemu-{img,io,nbd}.  I tracked them down by following
> the call chains leading to bdrv_open().

Forgot to mention: I ignored HMP commands, too.

[...]

[Qemu-devel] Review of ways to reopen BDSes (was: Review of monitor commands identifying BDS / BB by name)

[Markus Armbruster]

= Introduction =

Kevin asked for all the ways you can reopen BDSes with different options
or flags.  This memo only covers all the ways you can reopen, but it
doesn't attempt to fully analyze how options or flags change in each
case.

This is a systematic review of all the ways you can reopen BDSes in qemu
proper, i.e. not in qemu-{img,io,nbd}.  I tracked them down by following
the call chains leading to bdrv_reopen_multiple().


= QMP commands =

* block-commit

  Two separate code paths, depending on whether we're it's an active
  layer commit or not.

  - If active, we reopen the base with the root's flags.  If we fail
    later during start, we try to restore the flags by reopening again.
    In theory, restoring can fail, and then we mess up the flags.
    Avoiding that would be nice.

    The actual commit is done by code shared with drive-mirror, covered
    below.

  - Else, we reopen both the base and the overlay read/write.  The
    overlay is the BDS that has the top as backing file.  If we fail
    later during start, we don't attempt to undo a change to read/write,
    as far as I can tell.  Looks like a bug.

    On completion, we do attempt to undo, by reopening them again with
    their original flags.

* blockdev-snapshot-sync

  Creating an external snapshot of a node means splicing in a new
  snapshot BDS with the node as backing file.  We then try to reopen the
  node readonly, because it's now a backing file.  Failure is ignored,
  i.e. the node simply remains as writable as before.

* change-backing-file

  This updates the backing file in the image.  If the BDS is read-only,
  we reopen read/write before, and reopen read-only after.  If the first
  reopen fails, the command fails without doing anything.  If the update
  fails, we try the second reopen before failing the command with the
  failed update's error.  If the second reopen fails (unlikely), the
  command fails in an ugly way: the BDS is left writable.  Avoiding that
  would be nice.

* drive-mirror

  On successful completion, we do some BDS rewiring I don't fully
  understand.  The mirror target BDS gets swapped into another BDS.  If
  the two have different flags, we reopen the target to make them the
  same before we swap.  An explanation of what happens here is welcome.

* transaction

  This is a wrapper around a list of transaction-capable commands.
  Thus, nothing new here.


= Other user interfaces =

* HMP command commit

  Less capable cousin of QMP command block-commit.  Should be
  implemented on top of QMP, but isn't.  Instead, we a separate,
  specialized duplicate of the general commit code: bdrv_commit().

* Console escape key 's'

  Like HMP command "commit all".  See mux_proc_byte().

  Public service announcement: don't name your block backends "all".

* HMP command drive_mirror

  Implemented on top of QMP, so nothing new here.


= Generic block layer =

bdrv_reopen_queue() collects BDSes for atomic reopen,
bdrv_reopen_multiple() actually does it, in two stages,
bdrv_reopen_prepare() and bdrv_reopen_commit() / bdrv_reopen_abort().
All three call block driver methods with the same name.

bdrv_reopen() is a convenience wrapper for reopening a single BDS.


= Block driver methods =

Only one of them recurses:

* vmdk_reopen_prepare()

  Queues the extents.  Necessary, because the block driver hides them
  from the generic block layer.  Needs work, see also "Review of ways to
  create BDSes".