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]>
[...]
diff --git a/qapi/block-core.json b/qapi/block-core.json
index b82af74256..9cc7c3d140 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -6334,3 +6334,27 @@
##
{ 'struct': 'DummyBlockCoreForceArrays',
'data': { 'unused-block-graph-info': ['BlockGraphInfo'] } }
+
+##
+# @blockdev-replace:
+#
+# Replace a block-node associated with device (@parent should be
+# QOM path, and @child should be "root") or with block-export (@parent
+# should be export name, and @child should be "root") or any child
+# of block-node (@parent should be node-name, and @child should be any
+# if its children names) with @new-child block-node.
of its
+#
+# @parent: QOM path or block-export or node-name, which @child should
+# be repalced. If several matching parents exist, the command
replaced
+# will fail
End the sentence with a period, please.
+#
+# @child: child to replace. Must be "root" when parent is QOM path or
+# block-export
Likewise.
+#
+# @new-child: node-name of the block-node, which should become a
+# replacement for @child's current block-node
Likewise.
Indent one more, please.
+#
+# Since 11.0
+##
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.
Or even postpone its addition up to the end of this period.
@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.
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 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
}
--
Best regards,
Vladimir
