Re: [Qemu-devel] How to introduce bs->node_name ?

[Markus Armbruster <armbru@redhat.com>]

[Note cc: Eric & Luiz, because this also relates to QMP]

Benoît Canet <benoit.canet@irqsave.net> writes:

> Hi list,
>
> After a discussion on irc we have two potential solution in order to introduce
> a new bs->node_name member in order to be able to manipulate the graph from the
> monitors.
>
> The first one is to make the QMP device parameter of the block commands optional
> and add the node-name parameter as a second optional parameter.
> This is Markus prefered solution and Eric is ok with making mandatory parameters
> optional in QMP.
>
> The second one suggested by Kevin Would be to add some magic to the new
> node_name member by making it equal to device_name for backends and then making
> the qmp commands operate only on node-names.

Needs elaboration.  Let me try.

Currently, a BlockDriverState (short: BDS) has a device_name only when
it's a root of a BDS tree.  These roots are the drives defined with
-drive & friends.  There's code relying on "device_name implies root,
and vice versa".

We're working on giving the user much more control over how block
drivers are combined into backends.  One aspect of that is we need a way
for users to refer to a BDS.  Elsewhere in QEMU, we let users tack IDs
to stuff they create.  The new node_name suggested by Benoît is exactly
that.

Another aspect is that we're going to create a BlockBackend (short: BB)
object that captures the stuff now in BDS that is really about the whole
backend (and thus is used only in root BDSes now).

Semantically, device_name belongs to the whole backend, so it should
move into BB.

So far, QMP commands identify the object to be worked on by its
device_name, typically as parameter "device".  Consequently, they can
only work on roots now.

This is just fine for QMP commands that want to work on a backend.

It's not fine for QMP commands that want to work on an arbitrary,
possibly non-root BDS.

The first proposal is to add another parameter, say "id".  Users can
then refer either to an arbitrary BDS by "id", or (for backward
compatibility) to the root BDS by "device".  When the code sees
"device", it'll look up the BB, then fetch its root BDS.

CON: Existing parameter "device" becomes compatibility cruft.

PRO: Clean and obvious semantics (in my opinion).

The second proposal is to press the existing parameter "device" into
service for referring to BDS node_name.

To keep backward compatibility, we obviously need to ensure that
whenever the old code accepts a value of "device", the new code accepts
it as well, and both resolve it to the same BDS.

What about situations where the old code rejects a value of "device"?
The new code may resolve it to a non-root BDS that happens to have that
ID...

What about dynamic reconfiguration changing the root?  Example: a
synchronous snapshot splices in a QCOW2, which becomes the new root.  In
the current code, device_name refers to the new root.  Wouldn't that
require the BDS ID of the old root moves to the new root?  But that
would mean BDS IDs change!

To be honest, this scares me.  I've been burned by convenience magic and
compatibility magic often enough already.

> My personnal suggestion would be that non specified node-name would be set to
> "undefined" meaning that no operation could occur on this bs.

Yes, that's how IDs work elsewhere.

> For QMP access the device_name is accessed via bdrv_find() in a few place in
> blockdev.
>
> Here are the occurences of it:

[QMP and HMP code using bdrv_find() snipped]

I think we should review with the QMP schema first, code second.

> Very few of them operate on what is destined to become block backend and most of
> them should be able to operate on nodes of the graph;
>
> What solution do you prefer ?

Should be obvious by now :)