On 17.02.26 16:10, Markus Armbruster wrote:
Vladimir Sementsov-Ogievskiy <[email protected]> writes:
On 14.02.26 11:04, Markus Armbruster wrote:
Vladimir Sementsov-Ogievskiy <[email protected]> writes:
On 04.02.26 15:26, Markus Armbruster wrote:
Vladimir Sementsov-Ogievskiy <[email protected]> writes:
Add a command that can replace bs in following BdrvChild structures:
- qdev blk root child
- block-export blk root child
- any child of BlockDriverState selected by child-name
Signed-off-by: Vladimir Sementsov-Ogievskiy <[email protected]>
[...]
Let's see whether I understand...
@parent determines which of the three cases mentioned in the commit
message it ids:
* If @parent is a QOM path, case 1.
* If @parent is a block export name (@id in BlockExportOptions and
BlockExportInfo), case 2.
* If @parent is a block node name (@node-name in BlockdevOptions and
BlockDeviceInfo), case 3.
Correct?
Problem: this is ambiguous. A @parent "foo" could in fact be any of the
three cases.
Yes. And we return an error in case of any ambiguity.
So, in case of ambiguity, the command does not work.
On the one hand, this is the user's doing: reusing the same ID for
multiple things has always been a bad idea.
On the other hand, we're now turning a bad idea that may cause confusion
into an bad idea that may break things. Feels iffy.
For QOM paths, we have a workaround: avoid partial QOM paths. Feels
okay. Partial QOM paths are rather obscure, and best avoided entirely.
The only workaround for block export vs. block node ambiguity is to
avoid ID reuse. By the time a user sees the command fail, the IDs are
what they are, and there's is no workaround.
Do you need blockdev-replace to work for both *now*?
At the very least, we need to document the trap and point to the
workaround.
Alternatively, come up with an interface that avoids the issue. See
below.
3. If a block node named "foo" exists, it's case 3.
2. If a block export named "foo" exists, it's also case 2.
1. If exactly one QOM object named "foo" exists, it's also case 1. Why?
"foo" is a syntactically valid partial QOM path. Partial QOM paths
are a convenience feature that is virtually unknown (and possibly
ill-advised): you can omit leading path components as long as there's
no ambiguity.
Peeking ahead in the series... PATCH 8 appears to deprecate the
ambiguity between case 2. and 3.
I think we need to do better.
See review of PATCH 8 for doing better on deprecation.
More questions...
In case 1 and 2, @child "should be root". What happens when it's
something else?
Error returned. s/should/must/ ?
Yes, please.
In case 3, @child "should be any if its children names". I figure the
command fails when it isn't.
And here.
Likewise.
+{ 'command': 'blockdev-replace',
+ 'data': { 'parent': 'str', 'child': 'str', 'new-child': 'str' } }
[...]
This interface sort of overloads @parent and @child, and overloading the
former creates ambiguity that can make the command unusable.
Is it a real problem if we do deprecate the thing, that leads to this
ambiguity?
If it is, we may mark the command "unstable" during the deprecation period.
By itself, the proposed command is a trap for the unwary.
Deprecating the usage that enables the trap makes the trap temporary.
Warning on (deprecated) usage that enables the trap helps users avoid
it. Human users, mostly.
Marking the command "unstable" should ensure management application
avoid the command, and thus the trap.
All of the above together feels acceptable to me. Can we do better?
Maybe, and that's discussed below.
Or even postpone its addition up to the end of this period.
No trap, no problem :)
@parent's set of valid values is the union of QOM path, block node name,
block export ID with values occuring in more than one of them dropped.
@child's set of valid values depends on @parent: child name when it's a
QOM path, else "root".
The obvious non-overloaded interface is a union of three branches: QOM,
block node, block export. The QOM branch has variant members @qom-path
and @child. The block node branch has variant member @node-name. The
block export branch has variant member @id.
This is a bit more verbose: you have to specify the union tag[*].
If we ever need to replace block nodes associated with additional
things, we simply more branches. These branches can have arbitrary
members. In your interface, we'd have to make do with @child, or maybe
add optional arguments.
Thoughts? Mind, this isn't a demand! I'm exploring the design space.
Union-based solution was v9:
https://lore.kernel.org/qemu-devel/[email protected]/
And there was a discussion, that we may try a way without a union. And that's
this v10.
I see Kevin suggested to explore this approach. I certainly respect his
judgement.
Hmm, but that had different field names for parents: qdev-id, export-id and
node-name.
What you suggest is to keep one filed for parent - "parent", but also add a
"parent-type"
field. This way, we may deprecate and remove "parent-type" in future. Or, we may
add it as deprecated now (together with deprecating non-unique IDs)
We can't deprecate the union tag right away: it is *required*.
[*] We could add a "may omit union tag when the tag value can be derived
from present variant members" convenience feature to QAPI, but I'm not
sure it's worth the complexity.
It also may be not a union, but a simple structure with optional fields:
{
parent: str, required, qom-path, or node-name, or export name
child: str, optional, but required when parent is node-name, and must be
'root'
if present for other parent types
parent-type: str, optional, "qom-path" | "block-node" | "block-export",
helps to
overcome parent ambiguity, deprecated
new-child: str, required node-name
}
PRO union: which members are valid together is syntactically obvious.
With a bunch of optional members, that information is in the member
descriptions.
CON union: need to specify the union tag. The verbosity is a non-issue
for management applications, and mildly bothersome for humans.
How important is keeping things terse for humans here?
It's not very important. Mostly, I just want to make "nice" interface, and
possibility to
reference any supported object by one ID seemed very tempting to me.
But to be honest, introducing such a precedent makes sense, if we have in mind
more
cases where such approach may be reused. Personally, I don't have them. If
blockdev-replace
command remains the only API for the ages, which allows to mix in one field
different kinds
of IDs, it becomes just an extra exclusion from common practices.
Finally, I don't care too much, which interface we chose. I'm not sure now that
unifying
ids is better than union. So I'm OK to go back to union-based solution from v9,
update and
resend it as v11.
--
Best regards,
Vladimir
