[PATCH v10 0/8] blockdev-replace
Hi all! This series presents a new command blockdev-replace, which helps to insert/remove filters anywhere in the block graph. It can: - replace qdev block-node by qdev-id - replace export block-node by export-id - replace any child of parent block-node by node-name and child name So insertions is done in two steps: 1. blockdev_add (create filter node, unparented) [some parent] [new filter] | | V V [ some child ] 2. blockdev-replace (replace child by the filter) [some parent] | V [new filter] | V [some child] And removal is done in reverse order: 1. blockdev-replace (go back to picture 1) 2. blockdev_del (remove filter node) Ideally, we should do both operations (add + replace or replace + del) in a transaction, but that should be another series. v10: reabsed on master. 02: change exp-blk check into assert 04: rewrite to simpler API (on unique parent id) 07: switch to new API 08: new Vladimir Sementsov-Ogievskiy (8): block-backend: blk_root(): drop const specifier on return type block/export: add blk_by_export_id() block: make bdrv_find_child() function public qapi: add blockdev-replace command block: bdrv_get_xdbg_block_graph(): report export ids iotests.py: introduce VM.assert_edges_list() method iotests: add filter-insertion deprecate names duplication between qdev, block-node and block-export block.c | 29 +++ block/block-backend.c | 2 +- block/export/export.c | 41 ++++ blockdev.c | 82 +++++-- docs/about/deprecated.rst | 10 + include/block/block-global-state.h | 1 + include/block/block_int-io.h | 2 + include/block/export.h | 1 + include/system/block-backend-global-state.h | 5 +- qapi/block-core.json | 24 ++ stubs/blk-by-qdev-id.c | 18 ++ stubs/blk-exp-find-by-blk.c | 13 + stubs/meson.build | 2 + system/qdev-monitor.c | 16 ++ tests/qemu-iotests/iotests.py | 17 ++ tests/qemu-iotests/tests/filter-insertion | 222 ++++++++++++++++++ tests/qemu-iotests/tests/filter-insertion.out | 5 + 17 files changed, 474 insertions(+), 16 deletions(-) create mode 100644 stubs/blk-by-qdev-id.c create mode 100644 stubs/blk-exp-find-by-blk.c create mode 100755 tests/qemu-iotests/tests/filter-insertion create mode 100644 tests/qemu-iotests/tests/filter-insertion.out -- 2.52.0
We'll need get non-const child pointer for graph modifications in further commits. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> --- block/block-backend.c | 2 +- include/system/block-backend-global-state.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/block/block-backend.c b/block/block-backend.c index 9944657120..d4b48ac3b0 100644 --- a/block/block-backend.c +++ b/block/block-backend.c @@ -2834,7 +2834,7 @@ int coroutine_fn blk_co_copy_range(BlockBackend *blk_in, int64_t off_in, bytes, read_flags, write_flags); } -const BdrvChild *blk_root(BlockBackend *blk) +BdrvChild *blk_root(BlockBackend *blk) { GLOBAL_STATE_CODE(); return blk->root; diff --git a/include/system/block-backend-global-state.h b/include/system/block-backend-global-state.h index c3849640df..a438f21dff 100644 --- a/include/system/block-backend-global-state.h +++ b/include/system/block-backend-global-state.h @@ -117,7 +117,7 @@ void blk_set_force_allow_inactivate(BlockBackend *blk); bool blk_register_buf(BlockBackend *blk, void *host, size_t size, Error **errp); void blk_unregister_buf(BlockBackend *blk, void *host, size_t size); -const BdrvChild *blk_root(BlockBackend *blk); +BdrvChild *blk_root(BlockBackend *blk); int blk_make_empty(BlockBackend *blk, Error **errp); -- 2.52.0
We need it for further blockdev-replace functionality. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> --- block/export/export.c | 15 +++++++++++++++ include/system/block-backend-global-state.h | 1 + 2 files changed, 16 insertions(+) diff --git a/block/export/export.c b/block/export/export.c index f3bbf11070..d11beb900f 100644 --- a/block/export/export.c +++ b/block/export/export.c @@ -370,3 +370,18 @@ BlockExportInfoList *qmp_query_block_exports(Error **errp) return head; } + +BlockBackend *blk_by_export_id(const char *id, Error **errp) +{ + BlockExport *exp; + + exp = blk_exp_find(id); + if (exp == NULL) { + error_setg(errp, "Export '%s' not found", id); + return NULL; + } + + assert(exp->blk); + + return exp->blk; +} diff --git a/include/system/block-backend-global-state.h b/include/system/block-backend-global-state.h index a438f21dff..f23b9f1518 100644 --- a/include/system/block-backend-global-state.h +++ b/include/system/block-backend-global-state.h @@ -72,6 +72,7 @@ void blk_detach_dev(BlockBackend *blk, DeviceState *dev); DeviceState *blk_get_attached_dev(BlockBackend *blk); BlockBackend *blk_by_dev(void *dev); BlockBackend *blk_by_qdev_id(const char *id, Error **errp); +BlockBackend *blk_by_export_id(const char *id, Error **errp); void blk_set_dev_ops(BlockBackend *blk, const BlockDevOps *ops, void *opaque); int blk_make_zero(BlockBackend *blk, BdrvRequestFlags flags); -- 2.52.0
To be reused soon for blockdev-replace functionality. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> --- block.c | 13 +++++++++++++ blockdev.c | 14 -------------- include/block/block_int-io.h | 2 ++ 3 files changed, 15 insertions(+), 14 deletions(-) diff --git a/block.c b/block.c index 48a17f393c..b13d06f709 100644 --- a/block.c +++ b/block.c @@ -8288,6 +8288,19 @@ int bdrv_make_empty(BdrvChild *c, Error **errp) return 0; } +BdrvChild *bdrv_find_child(BlockDriverState *parent_bs, const char *child_name) +{ + BdrvChild *child; + + QLIST_FOREACH(child, &parent_bs->children, next) { + if (strcmp(child->name, child_name) == 0) { + return child; + } + } + + return NULL; +} + /* * Return the child that @bs acts as an overlay for, and from which data may be * copied in COW or COR operations. Usually this is the backing file. diff --git a/blockdev.c b/blockdev.c index 6e86c6262f..2540f991b3 100644 --- a/blockdev.c +++ b/blockdev.c @@ -3569,20 +3569,6 @@ unlock: } } -static BdrvChild * GRAPH_RDLOCK -bdrv_find_child(BlockDriverState *parent_bs, const char *child_name) -{ - BdrvChild *child; - - QLIST_FOREACH(child, &parent_bs->children, next) { - if (strcmp(child->name, child_name) == 0) { - return child; - } - } - - return NULL; -} - void qmp_x_blockdev_change(const char *parent, const char *child, const char *node, Error **errp) { diff --git a/include/block/block_int-io.h b/include/block/block_int-io.h index ed8b5657d6..8ff3bc6d87 100644 --- a/include/block/block_int-io.h +++ b/include/block/block_int-io.h @@ -130,6 +130,8 @@ bdrv_co_refresh_total_sectors(BlockDriverState *bs, int64_t hint); int co_wrapper_mixed_bdrv_rdlock bdrv_refresh_total_sectors(BlockDriverState *bs, int64_t hint); +BdrvChild * GRAPH_RDLOCK +bdrv_find_child(BlockDriverState *parent_bs, const char *child_name); BdrvChild * GRAPH_RDLOCK bdrv_cow_child(BlockDriverState *bs); BdrvChild * GRAPH_RDLOCK bdrv_filter_child(BlockDriverState *bs); BdrvChild * GRAPH_RDLOCK bdrv_filter_or_cow_child(BlockDriverState *bs); -- 2.52.0
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 <vsementsov@yandex-team.ru> --- blockdev.c | 68 ++++++++++++++++++++++++++++++++++++++++++ qapi/block-core.json | 24 +++++++++++++++ stubs/blk-by-qdev-id.c | 13 ++++++++ stubs/meson.build | 1 + 4 files changed, 106 insertions(+) create mode 100644 stubs/blk-by-qdev-id.c diff --git a/blockdev.c b/blockdev.c index 2540f991b3..3082a5763c 100644 --- a/blockdev.c +++ b/blockdev.c @@ -3681,6 +3681,74 @@ out: bdrv_drain_all_end(); } +void qmp_blockdev_replace(const char *parent, const char *child, + const char *new_child, Error **errp) +{ + BdrvChild *child_to_replace = NULL; + BlockDriverState *new_child_bs; + Error *dev_err = NULL, *exp_err = NULL; + BlockDriverState *parent_bs = bdrv_find_node(parent); + BlockBackend *dev_blk = blk_by_qdev_id(parent, &dev_err); + BlockBackend *exp_blk = blk_by_export_id(parent, &exp_err); + unsigned found = !!parent_bs + !!dev_blk + !!exp_blk; + + if (found == 0) { + error_setg(errp, "Neither device, nor export, nor block-node exist" + " with name '%s'. block-node: not found," + " device block-backend: %s, export block-backend: %s", + parent, error_get_pretty(dev_err), + error_get_pretty(exp_err)); + } + error_free(dev_err); + error_free(exp_err); + + if (found == 0) { + return; + } + + if (found > 1) { + error_setg(errp, "Parent name '%s' is ambigous: block-node %s," + " device block-backend %s, export block-backend %s", + parent, parent_bs ? "found" : "not found", + dev_blk ? "found" : "not found", + exp_blk ? "found" : "not found"); + return; + } + + if (parent_bs) { + child_to_replace = bdrv_find_child(parent_bs, child); + if (!child_to_replace) { + error_setg(errp, "Block driver node '%s' doesn't have child " + "named '%s'", parent, child); + return; + } + } else { + BlockBackend *blk = dev_blk ?: exp_blk; + + if (strcmp(child, "root")) { + error_setg(errp, "Devices and exports may have only 'root' child"); + } + + child_to_replace = blk_root(blk); + if (!child_to_replace) { + error_setg(errp, "%s '%s' is empty, nothing to replace", + dev_blk ? "Device" : "Export", parent); + return; + } + } + + assert(child_to_replace); + assert(child_to_replace->bs); + + new_child_bs = bdrv_find_node(new_child); + if (!new_child_bs) { + error_setg(errp, "Node '%s' not found", new_child); + return; + } + + bdrv_replace_child_bs(child_to_replace, new_child_bs, errp); +} + QemuOptsList qemu_common_drive_opts = { .name = "drive", .head = QTAILQ_HEAD_INITIALIZER(qemu_common_drive_opts.head), 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. +# +# @parent: QOM path or block-export or node-name, which @child should +# be repalced. If several matching parents exist, the command +# will fail +# +# @child: child to replace. Must be "root" when parent is QOM path or +# block-export +# +# @new-child: node-name of the block-node, which should become a +# replacement for @child's current block-node +# +# Since 11.0 +## +{ 'command': 'blockdev-replace', + 'data': { 'parent': 'str', 'child': 'str', 'new-child': 'str' } } diff --git a/stubs/blk-by-qdev-id.c b/stubs/blk-by-qdev-id.c new file mode 100644 index 0000000000..66ead77f4d --- /dev/null +++ b/stubs/blk-by-qdev-id.c @@ -0,0 +1,13 @@ +#include "qemu/osdep.h" +#include "qapi/error.h" +#include "system/block-backend.h" + +BlockBackend *blk_by_qdev_id(const char *id, Error **errp) +{ + /* + * We expect this when blockdev-change is called with parent-type=qdev, + * but qdev-monitor is not linked in. So no blk_ API is not available. + */ + error_setg(errp, "Parameter 'parent-type' does not accept value 'qdev'"); + return NULL; +} diff --git a/stubs/meson.build b/stubs/meson.build index d3b551f4de..93a52d273e 100644 --- a/stubs/meson.build +++ b/stubs/meson.build @@ -15,6 +15,7 @@ if have_block stub_ss.add(files('bdrv-next-monitor-owned.c')) stub_ss.add(files('blk-commit-all.c')) stub_ss.add(files('blk-exp-close-all.c')) + stub_ss.add(files('blk-by-qdev-id.c')) stub_ss.add(files('blockdev-close-all-bdrv-states.c')) stub_ss.add(files('change-state-handler.c')) stub_ss.add(files('get-vm-name.c')) -- 2.52.0
Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> 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 <vsementsov@yandex-team.ru>
[...]
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. 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. More questions... In case 1 and 2, @child "should be root". What happens when it's something else? In case 3, @child "should be any if its children names". I figure the command fails when it isn't.
+{ 'command': 'blockdev-replace', + 'data': { 'parent': 'str', 'child': 'str', 'new-child': 'str' } }
[...]
On 04.02.26 15:26, Markus Armbruster wrote:
Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> 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 <vsementsov@yandex-team.ru>
[...]
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.
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.
More questions...
In case 1 and 2, @child "should be root". What happens when it's something else?
Error returned. s/should/must/ ?
In case 3, @child "should be any if its children names". I figure the command fails when it isn't.
And here.
+{ 'command': 'blockdev-replace', + 'data': { 'parent': 'str', 'child': 'str', 'new-child': 'str' } }
[...]
-- Best regards, Vladimir
Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:
On 04.02.26 15:26, Markus Armbruster wrote:
Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> 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 <vsementsov@yandex-team.ru>
[...]
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. @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. [*] 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.
On 14.02.26 11:04, Markus Armbruster wrote:
Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:
On 04.02.26 15:26, Markus Armbruster wrote:
Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> 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 <vsementsov@yandex-team.ru>
[...]
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/20240626115350.405778-5-vsementsov@yandex... 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
Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:
On 14.02.26 11:04, Markus Armbruster wrote:
Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:
On 04.02.26 15:26, Markus Armbruster wrote:
Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> 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 <vsementsov@yandex-team.ru>
[...]
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/20240626115350.405778-5-vsementsov@yandex...
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?
On 17.02.26 16:10, Markus Armbruster wrote:
Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:
On 14.02.26 11:04, Markus Armbruster wrote:
Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:
On 04.02.26 15:26, Markus Armbruster wrote:
Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> 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 <vsementsov@yandex-team.ru>
[...]
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/20240626115350.405778-5-vsementsov@yandex...
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
Block layer maintainers' advice sought: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:
On 17.02.26 16:10, Markus Armbruster wrote:
Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:
On 14.02.26 11:04, Markus Armbruster wrote:
[...]
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/20240626115350.405778-5-vsementsov@yandex...
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.
Hanna, Kevin, got a preference?
Am 18.02.2026 um 07:25 hat Markus Armbruster geschrieben:
Block layer maintainers' advice sought:
Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:
On 17.02.26 16:10, Markus Armbruster wrote:
Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:
On 14.02.26 11:04, Markus Armbruster wrote:
[...]
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/20240626115350.405778-5-vsementsov@yandex...
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.
Hanna, Kevin, got a preference?
My stance is unchanged from v9. When Vladimir asked if we could somehow get rid of the union, I tried to help him find such a solution that might be acceptable, but also recommended to keep the union for now, simply because it raises a lot less questions and avoids the temporary trap, and to move to a unified ID only later. Still, as I said, "I don't think such a state is very pretty, but it would be okay for me". So I wouldn't block the approach taken here in v10 if Vladimir prefers it (at least as far as I can say from the discussion; I didn't actually review it yet). If you say that you really don't want to have the command fail on ambiguous IDs, we probably shouldn't do it. But as far as I am concerned, it seemed tolerable, so if you don't object, it remains Vladimir's decision. Kevin
Kevin Wolf <kwolf@redhat.com> writes:
Am 18.02.2026 um 07:25 hat Markus Armbruster geschrieben:
Block layer maintainers' advice sought:
Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:
On 17.02.26 16:10, Markus Armbruster wrote:
Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:
On 14.02.26 11:04, Markus Armbruster wrote:
[...]
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/20240626115350.405778-5-vsementsov@yandex...
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.
Hanna, Kevin, got a preference?
My stance is unchanged from v9. When Vladimir asked if we could somehow get rid of the union, I tried to help him find such a solution that might be acceptable, but also recommended to keep the union for now, simply because it raises a lot less questions and avoids the temporary trap, and to move to a unified ID only later.
Still, as I said, "I don't think such a state is very pretty, but it would be okay for me". So I wouldn't block the approach taken here in v10 if Vladimir prefers it (at least as far as I can say from the discussion; I didn't actually review it yet).
If you say that you really don't want to have the command fail on ambiguous IDs, we probably shouldn't do it. But as far as I am concerned, it seemed tolerable, so if you don't object, it remains Vladimir's decision.
I don't object, I advise. Overall, the union approach seems cleaner, and more in line with existing interface practice, albeit a bit more verbose. It avoids the ambiguous ID issue. It's also a more flexible base for extensions: add a branch with whatever variant members are needed. Whether we'll ever need the flexibility is uncertain. The non-union approach looks workable to me. As long as ambiguous IDs remain possible, users can get their VMs into states where the command doesn't work. Documentation must spell this out, which is a bit of a bother. Experience has taught us that "don't do that" documentation reduces the likelihood of users doing it, but not nearly as much as we'd like. Warnings are more effective, and I strongly recommend to add them. More bother. We should also consider to mark the interface unstable. This would delay adoption, which may well be a reason not to mark. Independently, I find truly unique IDs desirable.
Currently for block exports we report empty blk names. That's not good. Let's try to find corresponding block export and report its id. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> --- block.c | 4 ++++ block/export/export.c | 13 +++++++++++++ include/block/export.h | 1 + stubs/blk-exp-find-by-blk.c | 9 +++++++++ stubs/meson.build | 1 + 5 files changed, 28 insertions(+) create mode 100644 stubs/blk-exp-find-by-blk.c diff --git a/block.c b/block.c index b13d06f709..8254d57212 100644 --- a/block.c +++ b/block.c @@ -6354,7 +6354,11 @@ XDbgBlockGraph *bdrv_get_xdbg_block_graph(Error **errp) for (blk = blk_all_next(NULL); blk; blk = blk_all_next(blk)) { char *allocated_name = NULL; const char *name = blk_name(blk); + BlockExport *exp = blk_exp_find_by_blk(blk); + if (!*name && exp) { + name = exp->id; + } if (!*name) { name = allocated_name = blk_get_attached_dev_id(blk); } diff --git a/block/export/export.c b/block/export/export.c index d11beb900f..9169b43e13 100644 --- a/block/export/export.c +++ b/block/export/export.c @@ -60,6 +60,19 @@ BlockExport *blk_exp_find(const char *id) return NULL; } +BlockExport *blk_exp_find_by_blk(BlockBackend *blk) +{ + BlockExport *exp; + + QLIST_FOREACH(exp, &block_exports, next) { + if (exp->blk == blk) { + return exp; + } + } + + return NULL; +} + static const BlockExportDriver *blk_exp_find_driver(BlockExportType type) { int i; diff --git a/include/block/export.h b/include/block/export.h index 4bd9531d4d..2d3a0b4a28 100644 --- a/include/block/export.h +++ b/include/block/export.h @@ -85,6 +85,7 @@ struct BlockExport { BlockExport *blk_exp_add(BlockExportOptions *export, Error **errp); BlockExport *blk_exp_find(const char *id); +BlockExport *blk_exp_find_by_blk(BlockBackend *blk); void blk_exp_ref(BlockExport *exp); void blk_exp_unref(BlockExport *exp); void blk_exp_request_shutdown(BlockExport *exp); diff --git a/stubs/blk-exp-find-by-blk.c b/stubs/blk-exp-find-by-blk.c new file mode 100644 index 0000000000..20f7ff1bdd --- /dev/null +++ b/stubs/blk-exp-find-by-blk.c @@ -0,0 +1,9 @@ +#include "qemu/osdep.h" +#include "system/block-backend.h" +#include "block/export.h" + +BlockExport *blk_exp_find_by_blk(BlockBackend *blk) +{ + return NULL; +} + diff --git a/stubs/meson.build b/stubs/meson.build index 93a52d273e..d530a637d9 100644 --- a/stubs/meson.build +++ b/stubs/meson.build @@ -16,6 +16,7 @@ if have_block stub_ss.add(files('blk-commit-all.c')) stub_ss.add(files('blk-exp-close-all.c')) stub_ss.add(files('blk-by-qdev-id.c')) + stub_ss.add(files('blk-exp-find-by-blk.c')) stub_ss.add(files('blockdev-close-all-bdrv-states.c')) stub_ss.add(files('change-state-handler.c')) stub_ss.add(files('get-vm-name.c')) -- 2.52.0
Add an alternative method to check block graph, to be used in further commit. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> --- tests/qemu-iotests/iotests.py | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py index 05274772ce..d6f4e890da 100644 --- a/tests/qemu-iotests/iotests.py +++ b/tests/qemu-iotests/iotests.py @@ -1126,6 +1126,23 @@ def check_bitmap_status(self, node_name, bitmap_name, fields): return fields.items() <= ret.items() + def get_block_graph(self): + """ + Returns block graph in form of edges list, where each edge is a tuple: + (parent_node_name, child_name, child_node_name) + """ + graph = self.qmp('x-debug-query-block-graph')['return'] + + nodes = {n['id']: n['name'] for n in graph['nodes']} + # Check that all names are unique: + assert len(set(nodes.values())) == len(nodes) + + return [(nodes[e['parent']], e['name'], nodes[e['child']]) + for e in graph['edges']] + + def assert_edges_list(self, edges): + assert sorted(edges) == sorted(self.get_block_graph()) + def assert_block_path(self, root, path, expected_node, graph=None): """ Check whether the node under the given path in the block graph -- 2.52.0
Demonstrate new blockdev-replace API for filter insertion and removal. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> --- tests/qemu-iotests/tests/filter-insertion | 222 ++++++++++++++++++ tests/qemu-iotests/tests/filter-insertion.out | 5 + 2 files changed, 227 insertions(+) create mode 100755 tests/qemu-iotests/tests/filter-insertion create mode 100644 tests/qemu-iotests/tests/filter-insertion.out diff --git a/tests/qemu-iotests/tests/filter-insertion b/tests/qemu-iotests/tests/filter-insertion new file mode 100755 index 0000000000..23e114f959 --- /dev/null +++ b/tests/qemu-iotests/tests/filter-insertion @@ -0,0 +1,222 @@ +#!/usr/bin/env python3 +# +# Tests for inserting and removing filters in a block graph. +# +# Copyright (c) 2022 Virtuozzo International GmbH. +# +# SPDX-License-Identifier: GPL-2.0-or-later + +import os + +import iotests +from iotests import qemu_img_create, try_remove + + +disk = os.path.join(iotests.test_dir, 'disk') +sock = os.path.join(iotests.sock_dir, 'sock') +size = 1024 * 1024 + + +class TestFilterInsertion(iotests.QMPTestCase): + def setUp(self): + qemu_img_create('-f', iotests.imgfmt, disk, str(size)) + + self.vm = iotests.VM() + self.vm.launch() + + self.vm.cmd('blockdev-add', { + 'node-name': 'disk0', + 'driver': 'qcow2', + 'file': { + 'node-name': 'file0', + 'driver': 'file', + 'filename': disk + } + }) + + def tearDown(self): + self.vm.shutdown() + os.remove(disk) + try_remove(sock) + + def test_simple_insertion(self): + vm = self.vm + + vm.cmd('blockdev-add', { + 'node-name': 'filter', + 'driver': 'blkdebug', + 'image': 'file0' + }) + + vm.cmd('blockdev-replace', { + 'parent': 'disk0', + 'child': 'file', + 'new-child': 'filter' + }) + + # Filter inserted: + # disk0 -file-> filter -file-> file0 + vm.assert_edges_list([ + ('disk0', 'file', 'filter'), + ('filter', 'image', 'file0') + ]) + + vm.cmd('blockdev-replace', { + 'parent': 'disk0', + 'child': 'file', + 'new-child': 'file0' + }) + + # Filter replaced, but still exists: + # dik0 -file-> file0 <-file- filter + vm.assert_edges_list([ + ('disk0', 'file', 'file0'), + ('filter', 'image', 'file0') + ]) + + vm.cmd('blockdev-del', node_name='filter') + + # Filter removed + # dik0 -file-> file0 + vm.assert_edges_list([ + ('disk0', 'file', 'file0') + ]) + + def test_insert_under_qdev(self): + vm = self.vm + + vm.cmd('device_add', driver='virtio-scsi') + vm.cmd('device_add', id='sda', driver='scsi-hd', + drive='disk0') + + vm.cmd('blockdev-add', { + 'node-name': 'filter', + 'driver': 'compress', + 'file': 'disk0' + }) + + vm.cmd('blockdev-replace', { + 'parent': 'sda', + 'child': 'root', + 'new-child': 'filter' + }) + + # Filter inserted: + # sda -root-> filter -file-> disk0 -file-> file0 + vm.assert_edges_list([ + # parent_node_name, child_name, child_node_name + ('sda', 'root', 'filter'), + ('filter', 'file', 'disk0'), + ('disk0', 'file', 'file0'), + ]) + + vm.cmd('blockdev-replace', { + 'parent': 'sda', + 'child': 'root', + 'new-child': 'disk0' + }) + vm.cmd('blockdev-del', node_name='filter') + + # Filter removed: + # sda -root-> disk0 -file-> file0 + vm.assert_edges_list([ + # parent_node_name, child_name, child_node_name + ('sda', 'root', 'disk0'), + ('disk0', 'file', 'file0'), + ]) + + def test_insert_under_nbd_export(self): + vm = self.vm + + vm.cmd('nbd-server-start', + addr={'type': 'unix', 'data': {'path': sock}}) + vm.cmd('block-export-add', id='exp1', type='nbd', + node_name='disk0', name='exp1') + vm.cmd('block-export-add', id='exp2', type='nbd', + node_name='disk0', name='exp2') + vm.cmd('object-add', qom_type='throttle-group', + id='tg', limits={'iops-read': 1}) + + vm.cmd('blockdev-add', { + 'node-name': 'filter', + 'driver': 'throttle', + 'throttle-group': 'tg', + 'file': 'disk0' + }) + + vm.cmd('blockdev-replace', { + 'parent': 'exp1', + 'child': 'root', + 'new-child': 'filter' + }) + + # Only exp1 is throttled, exp2 is not: + # exp1 -root-> filter + # | + # |file + # v + # exp2 -file-> disk0 -file> file0 + vm.assert_edges_list([ + # parent_node_name, child_name, child_node_name + ('exp1', 'root', 'filter'), + ('filter', 'file', 'disk0'), + ('disk0', 'file', 'file0'), + ('exp2', 'root', 'disk0') + ]) + + vm.cmd('blockdev-replace', { + 'parent': 'exp2', + 'child': 'root', + 'new-child': 'filter' + }) + + # Both throttled: + # exp1 -root-> filter <-file- exp2 + # | + # |file + # v + # disk0 -file> file0 + vm.assert_edges_list([ + # parent_node_name, child_name, child_node_name + ('exp1', 'root', 'filter'), + ('filter', 'file', 'disk0'), + ('disk0', 'file', 'file0'), + ('exp2', 'root', 'filter') + ]) + + # Check, that filter is in use and can't be removed + result = vm.qmp('blockdev-del', node_name='filter') + self.assert_qmp(result, 'error/desc', 'Node filter is in use') + + vm.cmd('blockdev-replace', { + 'parent': 'exp1', + 'child': 'root', + 'new-child': 'disk0' + }) + + vm.cmd('blockdev-replace', { + 'parent': 'exp2', + 'child': 'root', + 'new-child': 'disk0' + }) + vm.cmd('blockdev-del', node_name='filter') + + # Filter removed: + # exp1 -root-> disk0 <-file- exp2 + # | + # |file + # v + # file0 + vm.assert_edges_list([ + # parent_node_name, child_name, child_node_name + ('exp1', 'root', 'disk0'), + ('disk0', 'file', 'file0'), + ('exp2', 'root', 'disk0') + ]) + + +if __name__ == '__main__': + iotests.main( + supported_fmts=['qcow2'], + supported_protocols=['file'] + ) diff --git a/tests/qemu-iotests/tests/filter-insertion.out b/tests/qemu-iotests/tests/filter-insertion.out new file mode 100644 index 0000000000..8d7e996700 --- /dev/null +++ b/tests/qemu-iotests/tests/filter-insertion.out @@ -0,0 +1,5 @@ +... +---------------------------------------------------------------------- +Ran 3 tests + +OK -- 2.52.0
Now we have blockdev-replace QMP command, which depend on a possibility to select any block parent (block node, block export, or qdev) by one unique name. The command fails, if name is ambiguous (i.e., match several parents of different types). In future we want to rid of this ambiguity. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> --- block.c | 12 ++++++++++++ block/export/export.c | 13 +++++++++++++ docs/about/deprecated.rst | 10 ++++++++++ include/block/block-global-state.h | 1 + include/system/block-backend-global-state.h | 2 ++ stubs/blk-by-qdev-id.c | 5 +++++ stubs/blk-exp-find-by-blk.c | 4 ++++ system/qdev-monitor.c | 16 ++++++++++++++++ 8 files changed, 63 insertions(+) diff --git a/block.c b/block.c index 8254d57212..5eae8b8623 100644 --- a/block.c +++ b/block.c @@ -1649,6 +1649,9 @@ static void bdrv_assign_node_name(BlockDriverState *bs, goto out; } + warn_device_exists(node_name); + warn_block_export_exists(node_name); + /* copy node name into the bs and insert it into the graph list */ pstrcpy(bs->node_name, sizeof(bs->node_name), node_name); QTAILQ_INSERT_TAIL(&graph_bdrv_states, bs, node_list); @@ -6233,6 +6236,15 @@ BlockDriverState *bdrv_find_node(const char *node_name) return NULL; } +void warn_block_node_exists(const char *node_name) +{ + if (bdrv_find_node(node_name)) { + warn_report("block node already exist with name '%s'. " + "Ambigous identifiers are deprecated. " + "In future that would be an error.", node_name); + } +} + /* Put this QMP function here so it can access the static graph_bdrv_states. */ BlockDeviceInfoList *bdrv_named_nodes_list(bool flat, Error **errp) diff --git a/block/export/export.c b/block/export/export.c index 9169b43e13..e65d1bec8e 100644 --- a/block/export/export.c +++ b/block/export/export.c @@ -23,6 +23,7 @@ #include "qapi/qapi-commands-block-export.h" #include "qapi/qapi-events-block-export.h" #include "qemu/id.h" +#include "qemu/error-report.h" #ifdef CONFIG_VHOST_USER_BLK_SERVER #include "vhost-user-blk-server.h" #endif @@ -108,6 +109,9 @@ BlockExport *blk_exp_add(BlockExportOptions *export, Error **errp) return NULL; } + warn_device_exists(export->id); + warn_block_node_exists(export->id); + drv = blk_exp_find_driver(export->type); if (!drv) { error_setg(errp, "No driver found for the requested export type"); @@ -384,6 +388,15 @@ BlockExportInfoList *qmp_query_block_exports(Error **errp) return head; } +void warn_block_export_exists(const char *id) +{ + if (blk_exp_find(id)) { + warn_report("block-export already exist with name '%s'. " + "Ambigous identifiers are deprecated. " + "In future that would be an error.", id); + } +} + BlockBackend *blk_by_export_id(const char *id, Error **errp) { BlockExport *exp; diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst index 88efa3aa80..18bb1eeafc 100644 --- a/docs/about/deprecated.rst +++ b/docs/about/deprecated.rst @@ -551,3 +551,13 @@ command documentation for details on the ``fdset`` usage. The ``zero-blocks`` capability was part of the block migration which doesn't exist anymore since it was removed in QEMU v9.1. + +Identifiers +----------- + +Possibility to intersect qdev ids/paths, block node names, and block +export names namespaces is deprecated. In future that would be +abandoned and all block exports, block nodes and devices will have +unique names. Now, reusing the name for another type of object (for +example, creating block-node with node-name equal to existing qdev +id) produce a warning. diff --git a/include/block/block-global-state.h b/include/block/block-global-state.h index ed89999f0f..ea50478fc4 100644 --- a/include/block/block-global-state.h +++ b/include/block/block-global-state.h @@ -207,6 +207,7 @@ void bdrv_aio_cancel(BlockAIOCB *acb); int bdrv_has_zero_init_1(BlockDriverState *bs); int coroutine_mixed_fn GRAPH_RDLOCK bdrv_has_zero_init(BlockDriverState *bs); BlockDriverState *bdrv_find_node(const char *node_name); +void warn_block_node_exists(const char *node_name); BlockDeviceInfoList *bdrv_named_nodes_list(bool flat, Error **errp); XDbgBlockGraph * GRAPH_RDLOCK bdrv_get_xdbg_block_graph(Error **errp); BlockDriverState *bdrv_lookup_bs(const char *device, diff --git a/include/system/block-backend-global-state.h b/include/system/block-backend-global-state.h index f23b9f1518..69e6aee618 100644 --- a/include/system/block-backend-global-state.h +++ b/include/system/block-backend-global-state.h @@ -73,6 +73,8 @@ DeviceState *blk_get_attached_dev(BlockBackend *blk); BlockBackend *blk_by_dev(void *dev); BlockBackend *blk_by_qdev_id(const char *id, Error **errp); BlockBackend *blk_by_export_id(const char *id, Error **errp); +void warn_block_export_exists(const char *id); +void warn_device_exists(const char *id); void blk_set_dev_ops(BlockBackend *blk, const BlockDevOps *ops, void *opaque); int blk_make_zero(BlockBackend *blk, BdrvRequestFlags flags); diff --git a/stubs/blk-by-qdev-id.c b/stubs/blk-by-qdev-id.c index 66ead77f4d..c83a2dde0d 100644 --- a/stubs/blk-by-qdev-id.c +++ b/stubs/blk-by-qdev-id.c @@ -11,3 +11,8 @@ BlockBackend *blk_by_qdev_id(const char *id, Error **errp) error_setg(errp, "Parameter 'parent-type' does not accept value 'qdev'"); return NULL; } + +void warn_device_exists(const char *id) +{ + /* do nothing */ +} diff --git a/stubs/blk-exp-find-by-blk.c b/stubs/blk-exp-find-by-blk.c index 20f7ff1bdd..a98c4572fc 100644 --- a/stubs/blk-exp-find-by-blk.c +++ b/stubs/blk-exp-find-by-blk.c @@ -7,3 +7,7 @@ BlockExport *blk_exp_find_by_blk(BlockBackend *blk) return NULL; } +void warn_block_export_exists(const char *id) +{ + /* do nothing */ +} diff --git a/system/qdev-monitor.c b/system/qdev-monitor.c index be18902bb2..67b9da952d 100644 --- a/system/qdev-monitor.c +++ b/system/qdev-monitor.c @@ -605,6 +605,8 @@ const char *qdev_set_id(DeviceState *dev, char *id, Error **errp) OBJECT(dev), NULL); if (prop) { dev->id = id; + warn_block_export_exists(id); + warn_block_node_exists(id); } else { error_setg(errp, "Duplicate device ID '%s'", id); g_free(id); @@ -903,6 +905,20 @@ static DeviceState *find_device_state(const char *id, bool use_generic_error, return dev; } +void warn_device_exists(const char *id) +{ + Object *obj = object_resolve_path_at(qdev_get_peripheral(), id); + + if (obj) { + DeviceState *dev = (DeviceState *)object_dynamic_cast(obj, TYPE_DEVICE); + + warn_report("%s '%s' already exist. " + "Ambigous identifiers are deprecated. " + "In future that would be an error.", + dev ? "Device" : "Object", id); + } +} + void qdev_unplug(DeviceState *dev, Error **errp) { HotplugHandler *hotplug_ctrl; -- 2.52.0
Now we have blockdev-replace QMP command, which depend on a possibility to select any block parent (block node, block export, or qdev) by one unique name. The command fails, if name is ambiguous (i.e., match several parents of different types). In future we want to rid of this ambiguity. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> --- v11: rework separate warn_s into one common function check_existing_parent_id(). block.c | 6 ++++++ block/export/export.c | 6 ++++++ blockdev.c | 21 +++++++++++++++++++++ docs/about/deprecated.rst | 10 ++++++++++ include/block/block-global-state.h | 2 ++ stubs/check-existing-parent-id.c | 6 ++++++ stubs/meson.build | 1 + system/qdev-monitor.c | 20 ++++++++++++++++++++ 8 files changed, 72 insertions(+) create mode 100644 stubs/check-existing-parent-id.c diff --git a/block.c b/block.c index 8254d57212..69674ad4ed 100644 --- a/block.c +++ b/block.c @@ -1618,6 +1618,7 @@ static void bdrv_assign_node_name(BlockDriverState *bs, { char *gen_node_name = NULL; GLOBAL_STATE_CODE(); + Error *local_err; if (!node_name) { node_name = gen_node_name = id_generate(ID_BLOCK); @@ -1649,6 +1650,11 @@ static void bdrv_assign_node_name(BlockDriverState *bs, goto out; } + if (!check_existing_parent_id(node_name, &local_err)) { + error_report_err(local_err); + local_err = NULL; + } + /* copy node name into the bs and insert it into the graph list */ pstrcpy(bs->node_name, sizeof(bs->node_name), node_name); QTAILQ_INSERT_TAIL(&graph_bdrv_states, bs, node_list); diff --git a/block/export/export.c b/block/export/export.c index 9169b43e13..174c86b4d2 100644 --- a/block/export/export.c +++ b/block/export/export.c @@ -96,6 +96,7 @@ BlockExport *blk_exp_add(BlockExportOptions *export, Error **errp) AioContext *ctx; uint64_t perm; int ret; + Error *local_err = NULL; GLOBAL_STATE_CODE(); @@ -108,6 +109,11 @@ BlockExport *blk_exp_add(BlockExportOptions *export, Error **errp) return NULL; } + if (!check_existing_parent_id(export->id, &local_err)) { + error_report_err(local_err); + local_err = NULL; + } + drv = blk_exp_find_driver(export->type); if (!drv) { error_setg(errp, "No driver found for the requested export type"); diff --git a/blockdev.c b/blockdev.c index 3082a5763c..643b2132c9 100644 --- a/blockdev.c +++ b/blockdev.c @@ -3681,6 +3681,27 @@ out: bdrv_drain_all_end(); } + +bool check_existing_parent_id(const char *id, Error **errp) +{ + if (bdrv_find_node(id)) { + error_setg(errp, "block node with id '%s' already exist", id); + return false; + } + + if (blk_by_qdev_id(id, NULL)) { + error_setg(errp, "block device with id '%s' already exist", id); + return false; + } + + if (blk_by_export_id(id, NULL)) { + error_setg(errp, "block export with id '%s' already exist", id); + return false; + } + + return true; +} + void qmp_blockdev_replace(const char *parent, const char *child, const char *new_child, Error **errp) { diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst index 88efa3aa80..18bb1eeafc 100644 --- a/docs/about/deprecated.rst +++ b/docs/about/deprecated.rst @@ -551,3 +551,13 @@ command documentation for details on the ``fdset`` usage. The ``zero-blocks`` capability was part of the block migration which doesn't exist anymore since it was removed in QEMU v9.1. + +Identifiers +----------- + +Possibility to intersect qdev ids/paths, block node names, and block +export names namespaces is deprecated. In future that would be +abandoned and all block exports, block nodes and devices will have +unique names. Now, reusing the name for another type of object (for +example, creating block-node with node-name equal to existing qdev +id) produce a warning. diff --git a/include/block/block-global-state.h b/include/block/block-global-state.h index ed89999f0f..5014324241 100644 --- a/include/block/block-global-state.h +++ b/include/block/block-global-state.h @@ -318,4 +318,6 @@ void bdrv_unregister_buf(BlockDriverState *bs, void *host, size_t size); void bdrv_cancel_in_flight(BlockDriverState *bs); +bool check_existing_parent_id(const char *id, Error **errp); + #endif /* BLOCK_GLOBAL_STATE_H */ diff --git a/stubs/check-existing-parent-id.c b/stubs/check-existing-parent-id.c new file mode 100644 index 0000000000..ef5ea3f26d --- /dev/null +++ b/stubs/check-existing-parent-id.c @@ -0,0 +1,6 @@ +#include "qemu/osdep.h" +#include "block/block-global-state.h" + +bool check_existing_parent_id(const char *id, Error **errp) { + return true; +} diff --git a/stubs/meson.build b/stubs/meson.build index 30536ec8fa..3bd8649bd1 100644 --- a/stubs/meson.build +++ b/stubs/meson.build @@ -16,6 +16,7 @@ if have_block stub_ss.add(files('blk-commit-all.c')) stub_ss.add(files('blk-exp-close-all.c')) stub_ss.add(files('blk-by-qdev-id.c')) + stub_ss.add(files('check-existing-parent-id.c')) stub_ss.add(files('blk-exp-find-by-blk.c')) stub_ss.add(files('blockdev-close-all-bdrv-states.c')) stub_ss.add(files('change-state-handler.c')) diff --git a/system/qdev-monitor.c b/system/qdev-monitor.c index be18902bb2..ca14135191 100644 --- a/system/qdev-monitor.c +++ b/system/qdev-monitor.c @@ -593,6 +593,7 @@ static BusState *qbus_find(const char *path, Error **errp) const char *qdev_set_id(DeviceState *dev, char *id, Error **errp) { ObjectProperty *prop; + Error *local_err = NULL; assert(!dev->id && !dev->realized); @@ -601,6 +602,18 @@ const char *qdev_set_id(DeviceState *dev, char *id, Error **errp) * has no parent */ if (id) { + g_autofree char *fullpath = g_strdup_printf("/peripheral/%s", id); + + if (!check_existing_parent_id(id, &local_err)) { + error_report_err(local_err); + local_err = NULL; + } + + if (!check_existing_parent_id(fullpath, &local_err)) { + error_report_err(local_err); + local_err = NULL; + } + prop = object_property_try_add_child(qdev_get_peripheral(), id, OBJECT(dev), NULL); if (prop) { @@ -613,6 +626,13 @@ const char *qdev_set_id(DeviceState *dev, char *id, Error **errp) } else { static int anon_count; gchar *name = g_strdup_printf("device[%d]", anon_count++); + g_autofree char *fullpath = g_strdup_printf("/peripheral-anon/%s", id); + + if (!check_existing_parent_id(fullpath, &local_err)) { + error_report_err(local_err); + local_err = NULL; + } + prop = object_property_add_child(qdev_get_peripheral_anon(), name, OBJECT(dev)); g_free(name); -- 2.52.0
Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:
Now we have blockdev-replace QMP command, which depend on a possibility to select any block parent (block node, block export, or qdev) by one unique name. The command fails, if name is ambiguous (i.e., match several parents of different types). In future we want to rid of this ambiguity.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
We have numerous kinds of IDs, i.e. names chosen by the user than need to be unique, but only among the same kind. For instance, you can't name two block nodes "foo", but you can name a block node, a block export, a qdev, and a network backend "foo". Using the same ID for multiple things is of course a bad idea. Permitting it was also a bad idea. Your patch rectifies this design mistake, but only partially. Consider: * IDs need to be unique with their kind. This is what we have now. I don't like it. * IDs need to be unique among their kind and possibly some set of additional kinds. This is where your patch takes us. I like it even less, to be honest. * IDs need to be unique, period. This is what I'd like to have. Thoughts?
On 04.02.26 15:38, Markus Armbruster wrote:
Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:
Now we have blockdev-replace QMP command, which depend on a possibility to select any block parent (block node, block export, or qdev) by one unique name. The command fails, if name is ambiguous (i.e., match several parents of different types). In future we want to rid of this ambiguity.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
We have numerous kinds of IDs, i.e. names chosen by the user than need to be unique, but only among the same kind. For instance, you can't name two block nodes "foo", but you can name a block node, a block export, a qdev, and a network backend "foo". Using the same ID for multiple things is of course a bad idea. Permitting it was also a bad idea.
Your patch rectifies this design mistake, but only partially. Consider:
* IDs need to be unique with their kind. This is what we have now. I don't like it.
* IDs need to be unique among their kind and possibly some set of additional kinds. This is where your patch takes us. I like it even less, to be honest.
* IDs need to be unique, period. This is what I'd like to have.
I like it. Is it enough to write it so simple in deprecation doc? Or should we still list all such ids we have in QEMU? It may be something like: Any kind of IDs you use to reference objects in QEMU must be unique, any used ID must reference exactly one object. This includes, but is not limited to qdev full and relative to "/peripheral/" paths, block-node and block-export names. -- Best regards, Vladimir
Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:
On 04.02.26 15:38, Markus Armbruster wrote:
Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:
Now we have blockdev-replace QMP command, which depend on a possibility to select any block parent (block node, block export, or qdev) by one unique name. The command fails, if name is ambiguous (i.e., match several parents of different types). In future we want to rid of this ambiguity.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
We have numerous kinds of IDs, i.e. names chosen by the user than need to be unique, but only among the same kind. For instance, you can't name two block nodes "foo", but you can name a block node, a block export, a qdev, and a network backend "foo". Using the same ID for multiple things is of course a bad idea. Permitting it was also a bad idea.
Your patch rectifies this design mistake, but only partially. Consider:
* IDs need to be unique with their kind. This is what we have now. I don't like it.
* IDs need to be unique among their kind and possibly some set of additional kinds. This is where your patch takes us. I like it even less, to be honest.
* IDs need to be unique, period. This is what I'd like to have.
I like it. Is it enough to write it so simple in deprecation doc? Or should we still list all such ids we have in QEMU?
It may be something like:
Any kind of IDs you use to reference objects in QEMU must be unique, any used ID must reference exactly one object. This includes, but is not limited to qdev full and relative to "/peripheral/" paths, block-node and block-export names.
This would serve as a declaration of intent. Better than nothing, I guess. To enforce uniqueness, we'll have to create a single table of IDs. If we make it a set, we can reject duplicate IDs, but can't point to the other ID. Meh. If we make it map to the kind of ID, we can report the kind. Something like "block node name 'foo' clashes with block export ID 'foo'". Feels adequate. If we make it map the the object the ID identifies, we can get rid of existing means to map from ID to object. May or may not be worthwhile. If we create the single table of IDs now rather than later, we can warn. Something like "duplicate IDs are deprecated: block node name 'foo' clashes with block export ID 'foo'". Much more likely to get users' attention than a note in docs/about/deprecated.rst. We could even wire it to the "-compat deprecated-input=..." machinery (I'd assist with that). This would let people test their software is ready for enforced unique IDs. Thoughts?
On 14.02.26 10:13, Markus Armbruster wrote:
Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:
On 04.02.26 15:38, Markus Armbruster wrote:
Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:
Now we have blockdev-replace QMP command, which depend on a possibility to select any block parent (block node, block export, or qdev) by one unique name. The command fails, if name is ambiguous (i.e., match several parents of different types). In future we want to rid of this ambiguity.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
We have numerous kinds of IDs, i.e. names chosen by the user than need to be unique, but only among the same kind. For instance, you can't name two block nodes "foo", but you can name a block node, a block export, a qdev, and a network backend "foo". Using the same ID for multiple things is of course a bad idea. Permitting it was also a bad idea.
Your patch rectifies this design mistake, but only partially. Consider:
* IDs need to be unique with their kind. This is what we have now. I don't like it.
* IDs need to be unique among their kind and possibly some set of additional kinds. This is where your patch takes us. I like it even less, to be honest.
* IDs need to be unique, period. This is what I'd like to have.
I like it. Is it enough to write it so simple in deprecation doc? Or should we still list all such ids we have in QEMU?
It may be something like:
Any kind of IDs you use to reference objects in QEMU must be unique, any used ID must reference exactly one object. This includes, but is not limited to qdev full and relative to "/peripheral/" paths, block-node and block-export names.
This would serve as a declaration of intent. Better than nothing, I guess.
To enforce uniqueness, we'll have to create a single table of IDs.
If we make it a set, we can reject duplicate IDs, but can't point to the other ID. Meh.
If we make it map to the kind of ID, we can report the kind. Something like "block node name 'foo' clashes with block export ID 'foo'". Feels adequate.
If we make it map the the object the ID identifies, we can get rid of existing means to map from ID to object. May or may not be worthwhile.
If we create the single table of IDs now rather than later, we can warn. Something like "duplicate IDs are deprecated: block node name 'foo' clashes with block export ID 'foo'". Much more likely to get users' attention than a note in docs/about/deprecated.rst.
We could even wire it to the "-compat deprecated-input=..." machinery (I'd assist with that). This would let people test their software is ready for enforced unique IDs.
Thoughts?
Sounds good. At least, I may try to make a common function, to be used both on object creation and from blockdev-replace to check for ambiguity. Now, another thought come in my mind: Shouldn't we instead of uniting different name-spaces, select qom-path as a primary source of object referencing? So, simple reserve "block/export/EXPORT_NAME" for exports and "block/node/NODE_NAME" for nodes? I imaging, if we simply "deprecate duplicating IDs", this will force libvirt and others to include object type into the id, so they may start use block node names like "block-node-disk1", and block export names like "block-export-disk1". We can be proactive, providing a common path for "foldering" IDs. This way, node-name becomes "relative" QOM path, and may be used in context where only node-name could be used (all existing use cases in the API now), but in interfaces, where may be used objects of different kinds (like new blockdev-replace) the full path is preferred (or even required). -- Best regards, Vladimir
Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:
On 14.02.26 10:13, Markus Armbruster wrote:
Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:
On 04.02.26 15:38, Markus Armbruster wrote:
Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:
Now we have blockdev-replace QMP command, which depend on a possibility to select any block parent (block node, block export, or qdev) by one unique name. The command fails, if name is ambiguous (i.e., match several parents of different types). In future we want to rid of this ambiguity.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
We have numerous kinds of IDs, i.e. names chosen by the user than need to be unique, but only among the same kind. For instance, you can't name two block nodes "foo", but you can name a block node, a block export, a qdev, and a network backend "foo". Using the same ID for multiple things is of course a bad idea. Permitting it was also a bad idea.
Your patch rectifies this design mistake, but only partially. Consider:
* IDs need to be unique with their kind. This is what we have now. I don't like it.
* IDs need to be unique among their kind and possibly some set of additional kinds. This is where your patch takes us. I like it even less, to be honest.
* IDs need to be unique, period. This is what I'd like to have.
I like it. Is it enough to write it so simple in deprecation doc? Or should we still list all such ids we have in QEMU?
It may be something like:
Any kind of IDs you use to reference objects in QEMU must be unique, any used ID must reference exactly one object. This includes, but is not limited to qdev full and relative to "/peripheral/" paths, block-node and block-export names.
This would serve as a declaration of intent. Better than nothing, I guess.
To enforce uniqueness, we'll have to create a single table of IDs.
If we make it a set, we can reject duplicate IDs, but can't point to the other ID. Meh.
If we make it map to the kind of ID, we can report the kind. Something like "block node name 'foo' clashes with block export ID 'foo'". Feels adequate.
If we make it map the the object the ID identifies, we can get rid of existing means to map from ID to object. May or may not be worthwhile.
If we create the single table of IDs now rather than later, we can warn. Something like "duplicate IDs are deprecated: block node name 'foo' clashes with block export ID 'foo'". Much more likely to get users' attention than a note in docs/about/deprecated.rst.
We could even wire it to the "-compat deprecated-input=..." machinery (I'd assist with that). This would let people test their software is ready for enforced unique IDs.
Thoughts?
Sounds good. At least, I may try to make a common function, to be used both on object creation and from blockdev-replace to check for ambiguity.
Now, another thought come in my mind:
Shouldn't we instead of uniting different name-spaces, select qom-path as a primary source of object referencing? So, simple reserve "block/export/EXPORT_NAME" for exports and "block/node/NODE_NAME" for nodes?
Making everything a QOM object has some merit. However, we'd have to push QOMification pretty far. Some ID kinds are already in QOM. For instance, -device id=foo,... creates "/machine/peripheral/foo", and -object id=bar,.. creates "/objects/bar". Some ID kinds could and quite possibly should be in QOM, such as device backends: -chardev, -netdev, ... What about monitors? -mon & friends. Having them in QOM wouldn't hurt, I guess, but would it be worth the churn? What about -rtc? -global? -action?
I imaging, if we simply "deprecate duplicating IDs", this will force libvirt and others to include object type into the id, so they may start use block node names like "block-node-disk1", and block export names like "block-export-disk1".
Question for libvirt developers: would any living version of libvirt use the same ID for different kinds of IDs?
We can be proactive, providing a common path for "foldering" IDs.
This way, node-name becomes "relative" QOM path, and may be used in context where only node-name could be used (all existing use cases in the API now), but in interfaces, where may be used objects of different kinds (like new blockdev-replace) the full path is preferred (or even required).
As is, we cannot define IDs as relative QOM paths even for the kinds of IDs that are already in QOM. Say we have both a device and an object with ID "foo". The device's QOM path is "/machine/peripheral/foo". The object's is "/objects/foo". The relative QOM path "foo" is then ambiguous, and won't resolve.
On 22.01.26 17:01, Vladimir Sementsov-Ogievskiy wrote:
Now we have blockdev-replace QMP command, which depend on a possibility to select any block parent (block node, block export, or qdev) by one unique name. The command fails, if name is ambiguous (i.e., match several parents of different types). In future we want to rid of this ambiguity.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> ---
v11: rework separate warn_s into one common function check_existing_parent_id().
block.c | 6 ++++++ block/export/export.c | 6 ++++++ blockdev.c | 21 +++++++++++++++++++++ docs/about/deprecated.rst | 10 ++++++++++ include/block/block-global-state.h | 2 ++ stubs/check-existing-parent-id.c | 6 ++++++ stubs/meson.build | 1 + system/qdev-monitor.c | 20 ++++++++++++++++++++ 8 files changed, 72 insertions(+) create mode 100644 stubs/check-existing-parent-id.c
[...]
diff --git a/system/qdev-monitor.c b/system/qdev-monitor.c index be18902bb2..ca14135191 100644 --- a/system/qdev-monitor.c +++ b/system/qdev-monitor.c @@ -593,6 +593,7 @@ static BusState *qbus_find(const char *path, Error **errp) const char *qdev_set_id(DeviceState *dev, char *id, Error **errp) { ObjectProperty *prop; + Error *local_err = NULL;
assert(!dev->id && !dev->realized);
@@ -601,6 +602,18 @@ const char *qdev_set_id(DeviceState *dev, char *id, Error **errp) * has no parent */ if (id) { + g_autofree char *fullpath = g_strdup_printf("/peripheral/%s", id); + + if (!check_existing_parent_id(id, &local_err)) { + error_report_err(local_err); + local_err = NULL; + } + + if (!check_existing_parent_id(fullpath, &local_err)) { + error_report_err(local_err); + local_err = NULL; + } + prop = object_property_try_add_child(qdev_get_peripheral(), id, OBJECT(dev), NULL); if (prop) { @@ -613,6 +626,13 @@ const char *qdev_set_id(DeviceState *dev, char *id, Error **errp) } else { static int anon_count; gchar *name = g_strdup_printf("device[%d]", anon_count++); + g_autofree char *fullpath = g_strdup_printf("/peripheral-anon/%s", id); + + if (!check_existing_parent_id(fullpath, &local_err)) { + error_report_err(local_err); + local_err = NULL; + } +
Node names and block export IDs need to conform to id_wellformed() (or id_generate()), which does not allow slashes. So the paths should never intersect with node names and block export IDs by design. With these checks removed (or not, if you think we need them, they don’t hurt either), and with the spelling in patch 4 fixed as per Markus’s suggestions, for the series: Reviewed-by: Hanna Czenczek <hreitz@redhat.com> (I know the discussion around how unique IDs should be is ongoing, but FWIW, having unique IDs across the block layer is good enough for me as a first step. Well, I guess, including block job IDs might be nice, too.)
participants (4)
-
Hanna Czenczek -
Kevin Wolf -
Markus Armbruster -
Vladimir Sementsov-Ogievskiy