[PATCH v10 0/8] blockdev-replace

[PATCH v10 0/8] blockdev-replace

[Vladimir Sementsov-Ogievskiy]

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

[PATCH v10 1/8] block-backend: blk_root(): drop const specifier on return type

[Vladimir Sementsov-Ogievskiy]

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

[PATCH v10 2/8] block/export: add blk_by_export_id()

[Vladimir Sementsov-Ogievskiy]

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

[PATCH v10 3/8] block: make bdrv_find_child() function public

[Vladimir Sementsov-Ogievskiy]

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

[PATCH v10 4/8] qapi: add blockdev-replace command

[Vladimir Sementsov-Ogievskiy]

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

[PATCH v10 5/8] block: bdrv_get_xdbg_block_graph(): report export ids

[Vladimir Sementsov-Ogievskiy]

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

[PATCH v10 6/8] iotests.py: introduce VM.assert_edges_list() method

[Vladimir Sementsov-Ogievskiy]

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

[PATCH v10 7/8] iotests: add filter-insertion

[Vladimir Sementsov-Ogievskiy]

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

[PATCH v10 8/8] deprecate names duplication between qdev, block-node and block-export

[Vladimir Sementsov-Ogievskiy]

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

[PATCH v11 8/8] deprecate names duplication between qdev, block-node and block-export

[Vladimir Sementsov-Ogievskiy]

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

Re: [PATCH v10 4/8] qapi: add blockdev-replace command

[Markus Armbruster]

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' } }

[...]

Re: [PATCH v11 8/8] deprecate names duplication between qdev, block-node and block-export

[Markus Armbruster]

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?

Re: [PATCH v10 4/8] qapi: add blockdev-replace command

[Vladimir Sementsov-Ogievskiy]

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

Re: [PATCH v11 8/8] deprecate names duplication between qdev, block-node and block-export

[Vladimir Sementsov-Ogievskiy]

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

Re: [PATCH v11 8/8] deprecate names duplication between qdev, block-node and block-export

[Markus Armbruster]

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?

Re: [PATCH v10 4/8] qapi: add blockdev-replace command

[Markus Armbruster]

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.

Re: [PATCH v10 4/8] qapi: add blockdev-replace command

[Vladimir Sementsov-Ogievskiy]

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-team.ru/

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

Re: [PATCH v11 8/8] deprecate names duplication between qdev, block-node and block-export

[Vladimir Sementsov-Ogievskiy]

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

Re: [PATCH v11 8/8] deprecate names duplication between qdev, block-node and block-export

[Markus Armbruster]

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.

Re: [PATCH v10 4/8] qapi: add blockdev-replace command

[Markus Armbruster]

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-team.ru/
>
> 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?

Re: [PATCH v10 4/8] qapi: add blockdev-replace command

[Vladimir Sementsov-Ogievskiy]

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-team.ru/
>>
>> 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

Re: [PATCH v10 4/8] qapi: add blockdev-replace command

[Markus Armbruster]

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-team.ru/
>>>
>>> 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?

Re: [PATCH v10 4/8] qapi: add blockdev-replace command

[Kevin Wolf]

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-team.ru/
> >>>
> >>> 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

Re: [PATCH v10 4/8] qapi: add blockdev-replace command

[Markus Armbruster]

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-team.ru/
>> >>>
>> >>> 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.

Re: [PATCH v11 8/8] deprecate names duplication between qdev, block-node and block-export

[Hanna Czenczek]

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