[PATCH v9 0/7] blockdev-replace

[PATCH v9 0/7] 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 to do both operations (add + replace or replace + del) in a
transaction, but that would be another series.

v9: rebase
    drop x- prefix and use unstable feature
    bump version to 9.1 in qapi spec
    update error message in blk_by_qdev_id stub

v8: rebase. Also don't use "preallocate" filter in a test, as we don't
support removal of this filter for now. Preallocate filter is really
unusual, see discussion here:
https://www.mail-archive.com/qemu-devel@nongnu.org/msg994945.html

Vladimir Sementsov-Ogievskiy (7):
  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

 block.c                                       |  17 ++
 block/block-backend.c                         |   2 +-
 block/export/export.c                         |  31 +++
 blockdev.c                                    |  70 ++++--
 include/block/block_int-io.h                  |   2 +
 include/block/export.h                        |   1 +
 include/sysemu/block-backend-global-state.h   |   3 +-
 qapi/block-core.json                          |  88 +++++++
 stubs/blk-by-qdev-id.c                        |  13 +
 stubs/blk-exp-find-by-blk.c                   |   9 +
 stubs/meson.build                             |   2 +
 tests/qemu-iotests/iotests.py                 |  17 ++
 tests/qemu-iotests/tests/filter-insertion     | 236 ++++++++++++++++++
 tests/qemu-iotests/tests/filter-insertion.out |   5 +
 14 files changed, 480 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.34.1

[PATCH v9 1/7] 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/sysemu/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 db6f9b92a3..71d5002198 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -2879,7 +2879,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/sysemu/block-backend-global-state.h b/include/sysemu/block-backend-global-state.h
index 49c12b0fa9..ccb35546a1 100644
--- a/include/sysemu/block-backend-global-state.h
+++ b/include/sysemu/block-backend-global-state.h
@@ -126,7 +126,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.34.1

[PATCH v9 2/7] 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                       | 18 ++++++++++++++++++
 include/sysemu/block-backend-global-state.h |  1 +
 2 files changed, 19 insertions(+)

diff --git a/block/export/export.c b/block/export/export.c
index 6d51ae8ed7..57beae7982 100644
--- a/block/export/export.c
+++ b/block/export/export.c
@@ -355,3 +355,21 @@ 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;
+    }
+
+    if (!exp->blk) {
+        error_setg(errp, "Export '%s' is empty", id);
+        return NULL;
+    }
+
+    return exp->blk;
+}
diff --git a/include/sysemu/block-backend-global-state.h b/include/sysemu/block-backend-global-state.h
index ccb35546a1..410d0cc5c7 100644
--- a/include/sysemu/block-backend-global-state.h
+++ b/include/sysemu/block-backend-global-state.h
@@ -74,6 +74,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);
 
 void blk_activate(BlockBackend *blk, Error **errp);
-- 
2.34.1

Re: [PATCH v9 2/7] block/export: add blk_by_export_id()

[Markus Armbruster]

Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:

> We need it for further blockdev-replace functionality.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> ---
>  block/export/export.c                       | 18 ++++++++++++++++++
>  include/sysemu/block-backend-global-state.h |  1 +
>  2 files changed, 19 insertions(+)
>
> diff --git a/block/export/export.c b/block/export/export.c
> index 6d51ae8ed7..57beae7982 100644
> --- a/block/export/export.c
> +++ b/block/export/export.c
> @@ -355,3 +355,21 @@ 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;
> +    }
> +
> +    if (!exp->blk) {
> +        error_setg(errp, "Export '%s' is empty", id);

Can this happen?

> +        return NULL;
> +    }
> +
> +    return exp->blk;
> +}
> diff --git a/include/sysemu/block-backend-global-state.h b/include/sysemu/block-backend-global-state.h
> index ccb35546a1..410d0cc5c7 100644
> --- a/include/sysemu/block-backend-global-state.h
> +++ b/include/sysemu/block-backend-global-state.h
> @@ -74,6 +74,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);
>  
>  void blk_activate(BlockBackend *blk, Error **errp);

Re: [PATCH v9 2/7] block/export: add blk_by_export_id()

[Vladimir Sementsov-Ogievskiy]

On 18.07.24 14:48, Markus Armbruster wrote:
> Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:
> 
>> We need it for further blockdev-replace functionality.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>> ---
>>   block/export/export.c                       | 18 ++++++++++++++++++
>>   include/sysemu/block-backend-global-state.h |  1 +
>>   2 files changed, 19 insertions(+)
>>
>> diff --git a/block/export/export.c b/block/export/export.c
>> index 6d51ae8ed7..57beae7982 100644
>> --- a/block/export/export.c
>> +++ b/block/export/export.c
>> @@ -355,3 +355,21 @@ 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;
>> +    }
>> +
>> +    if (!exp->blk) {
>> +        error_setg(errp, "Export '%s' is empty", id);
> 
> Can this happen?
> 

Hmm, looking at the code in blk_exp_add:

     assert(exp->blk != NULL);
                                                                                  
     QLIST_INSERT_HEAD(&block_exports, exp, next);
     return exp;


seems not. And I can't find anything changing exp->blk except for blk_exp_add().

Will switch to assertion.

>> +        return NULL;
>> +    }
>> +
>> +    return exp->blk;
>> +}
>> diff --git a/include/sysemu/block-backend-global-state.h b/include/sysemu/block-backend-global-state.h
>> index ccb35546a1..410d0cc5c7 100644
>> --- a/include/sysemu/block-backend-global-state.h
>> +++ b/include/sysemu/block-backend-global-state.h
>> @@ -74,6 +74,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);
>>   
>>   void blk_activate(BlockBackend *blk, Error **errp);
> 

-- 
Best regards,
Vladimir

[PATCH v9 3/7] 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 468cf5e67d..f6292f459a 100644
--- a/block.c
+++ b/block.c
@@ -8174,6 +8174,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 835064ed03..ba7e90b06e 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3452,20 +3452,6 @@ void qmp_blockdev_del(const char *node_name, Error **errp)
     bdrv_unref(bs);
 }
 
-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 4a7cf2b4fd..11ed4aa927 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.34.1

[PATCH v9 4/7] 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             | 56 +++++++++++++++++++++++++++
 qapi/block-core.json   | 88 ++++++++++++++++++++++++++++++++++++++++++
 stubs/blk-by-qdev-id.c | 13 +++++++
 stubs/meson.build      |  1 +
 4 files changed, 158 insertions(+)
 create mode 100644 stubs/blk-by-qdev-id.c

diff --git a/blockdev.c b/blockdev.c
index ba7e90b06e..2190467022 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3559,6 +3559,62 @@ void qmp_x_blockdev_set_iothread(const char *node_name, StrOrNull *iothread,
     bdrv_try_change_aio_context(bs, new_context, NULL, errp);
 }
 
+void qmp_blockdev_replace(BlockdevReplace *repl, Error **errp)
+{
+    BdrvChild *child = NULL;
+    BlockDriverState *new_child_bs;
+
+    if (repl->parent_type == BLOCK_PARENT_TYPE_DRIVER) {
+        BlockDriverState *parent_bs;
+
+        parent_bs = bdrv_find_node(repl->u.driver.node_name);
+        if (!parent_bs) {
+            error_setg(errp, "Block driver node with node-name '%s' not "
+                       "found", repl->u.driver.node_name);
+            return;
+        }
+
+        child = bdrv_find_child(parent_bs, repl->u.driver.child);
+        if (!child) {
+            error_setg(errp, "Block driver node '%s' doesn't have child "
+                       "named '%s'", repl->u.driver.node_name,
+                       repl->u.driver.child);
+            return;
+        }
+    } else {
+        /* Other types are similar, they work through blk */
+        BlockBackend *blk;
+        bool is_qdev = repl->parent_type == BLOCK_PARENT_TYPE_QDEV;
+        const char *id =
+            is_qdev ? repl->u.qdev.qdev_id : repl->u.export.export_id;
+
+        assert(is_qdev || repl->parent_type == BLOCK_PARENT_TYPE_EXPORT);
+
+        blk = is_qdev ? blk_by_qdev_id(id, errp) : blk_by_export_id(id, errp);
+        if (!blk) {
+            return;
+        }
+
+        child = blk_root(blk);
+        if (!child) {
+            error_setg(errp, "%s '%s' is empty, nothing to replace",
+                       is_qdev ? "Device" : "Export", id);
+            return;
+        }
+    }
+
+    assert(child);
+    assert(child->bs);
+
+    new_child_bs = bdrv_find_node(repl->new_child);
+    if (!new_child_bs) {
+        error_setg(errp, "Node '%s' not found", repl->new_child);
+        return;
+    }
+
+    bdrv_replace_child_bs(child, 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 df5e07debd..0a6f08a6e0 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -6148,3 +6148,91 @@
 ##
 { 'struct': 'DummyBlockCoreForceArrays',
   'data': { 'unused-block-graph-info': ['BlockGraphInfo'] } }
+
+##
+# @BlockParentType:
+#
+# @qdev: block device, such as created by device_add, and denoted by
+#     qdev-id
+#
+# @driver: block driver node, such as created by blockdev-add, and
+#     denoted by node-name
+#
+# @export: block export, such created by block-export-add, and
+#     denoted by export-id
+#
+# Since 9.1
+##
+{ 'enum': 'BlockParentType',
+  'data': ['qdev', 'driver', 'export'] }
+
+##
+# @BdrvChildRefQdev:
+#
+# @qdev-id: the device's ID or QOM path
+#
+# Since 9.1
+##
+{ 'struct': 'BdrvChildRefQdev',
+  'data': { 'qdev-id': 'str' } }
+
+##
+# @BdrvChildRefExport:
+#
+# @export-id: block export identifier
+#
+# Since 9.1
+##
+{ 'struct': 'BdrvChildRefExport',
+  'data': { 'export-id': 'str' } }
+
+##
+# @BdrvChildRefDriver:
+#
+# @node-name: the node name of the parent block node
+#
+# @child: name of the child to be replaced, like "file" or "backing"
+#
+# Since 9.1
+##
+{ 'struct': 'BdrvChildRefDriver',
+  'data': { 'node-name': 'str', 'child': 'str' } }
+
+##
+# @BlockdevReplace:
+#
+# @parent-type: type of the parent, which child is to be replaced
+#
+# @new-child: new child for replacement
+#
+# Since 9.1
+##
+{ 'union': 'BlockdevReplace',
+  'base': {
+      'parent-type': 'BlockParentType',
+      'new-child': 'str'
+  },
+  'discriminator': 'parent-type',
+  'data': {
+      'qdev': 'BdrvChildRefQdev',
+      'export': 'BdrvChildRefExport',
+      'driver': 'BdrvChildRefDriver'
+  } }
+
+##
+# @blockdev-replace:
+#
+# Replace a block-node associated with device (selected by
+# @qdev-id) or with block-export (selected by @export-id) or
+# any child of block-node (selected by @node-name and @child)
+# with @new-child block-node.
+#
+# Features:
+#
+# @unstable: This command is experimental.
+#
+# Since 9.1
+##
+{ 'command': 'blockdev-replace', 'boxed': true,
+  'features': [ 'unstable' ],
+  'data': 'BlockdevReplace' }
diff --git a/stubs/blk-by-qdev-id.c b/stubs/blk-by-qdev-id.c
new file mode 100644
index 0000000000..5ec9f755ee
--- /dev/null
+++ b/stubs/blk-by-qdev-id.c
@@ -0,0 +1,13 @@
+#include "qemu/osdep.h"
+#include "qapi/error.h"
+#include "sysemu/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 772a3e817d..068998c1a5 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.34.1

Re: [PATCH v9 4/7] 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>
> ---
>  blockdev.c             | 56 +++++++++++++++++++++++++++
>  qapi/block-core.json   | 88 ++++++++++++++++++++++++++++++++++++++++++
>  stubs/blk-by-qdev-id.c | 13 +++++++
>  stubs/meson.build      |  1 +
>  4 files changed, 158 insertions(+)
>  create mode 100644 stubs/blk-by-qdev-id.c
>
> diff --git a/blockdev.c b/blockdev.c
> index ba7e90b06e..2190467022 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -3559,6 +3559,62 @@ void qmp_x_blockdev_set_iothread(const char *node_name, StrOrNull *iothread,
>      bdrv_try_change_aio_context(bs, new_context, NULL, errp);
>  }
>  
> +void qmp_blockdev_replace(BlockdevReplace *repl, Error **errp)
> +{
> +    BdrvChild *child = NULL;
> +    BlockDriverState *new_child_bs;
> +
> +    if (repl->parent_type == BLOCK_PARENT_TYPE_DRIVER) {
> +        BlockDriverState *parent_bs;
> +
> +        parent_bs = bdrv_find_node(repl->u.driver.node_name);
> +        if (!parent_bs) {
> +            error_setg(errp, "Block driver node with node-name '%s' not "
> +                       "found", repl->u.driver.node_name);
> +            return;
> +        }
> +
> +        child = bdrv_find_child(parent_bs, repl->u.driver.child);
> +        if (!child) {
> +            error_setg(errp, "Block driver node '%s' doesn't have child "
> +                       "named '%s'", repl->u.driver.node_name,
> +                       repl->u.driver.child);
> +            return;
> +        }
> +    } else {
> +        /* Other types are similar, they work through blk */
> +        BlockBackend *blk;
> +        bool is_qdev = repl->parent_type == BLOCK_PARENT_TYPE_QDEV;
> +        const char *id =
> +            is_qdev ? repl->u.qdev.qdev_id : repl->u.export.export_id;
> +
> +        assert(is_qdev || repl->parent_type == BLOCK_PARENT_TYPE_EXPORT);
> +
> +        blk = is_qdev ? blk_by_qdev_id(id, errp) : blk_by_export_id(id, errp);

blk_by_export_id() finds export @exp, and returns the associated block
backend exp->blk.  Fine.

blk_by_qdev_id() finds the device, and then searches @block_backends for
a blk with blk->dev == blk.  If a device has more than one block
backend, you get the one first in @block_backends.  I figure that's the
one created first.

Interface issue: when a device has multiple block backends, only one of
them can be replaced, and which one is kind of random.

Do such devices exist?

If no, could they exist?

If yes, what should we do about it now?

> +        if (!blk) {
> +            return;
> +        }
> +
> +        child = blk_root(blk);
> +        if (!child) {
> +            error_setg(errp, "%s '%s' is empty, nothing to replace",
> +                       is_qdev ? "Device" : "Export", id);
> +            return;
> +        }
> +    }
> +
> +    assert(child);
> +    assert(child->bs);
> +
> +    new_child_bs = bdrv_find_node(repl->new_child);
> +    if (!new_child_bs) {
> +        error_setg(errp, "Node '%s' not found", repl->new_child);
> +        return;
> +    }
> +
> +    bdrv_replace_child_bs(child, 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 df5e07debd..0a6f08a6e0 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -6148,3 +6148,91 @@
>  ##
>  { 'struct': 'DummyBlockCoreForceArrays',
>    'data': { 'unused-block-graph-info': ['BlockGraphInfo'] } }
> +
> +##
> +# @BlockParentType:
> +#
> +# @qdev: block device, such as created by device_add, and denoted by
> +#     qdev-id
> +#
> +# @driver: block driver node, such as created by blockdev-add, and
> +#     denoted by node-name

node-name and child?

> +#
> +# @export: block export, such created by block-export-add, and
> +#     denoted by export-id
> +#
> +# Since 9.1
> +##

I'm kind of unhappy with this doc comment.  I feel makes sense only in
the context of its use.  Let me try to avoid that:

   # @driver: the parent is a block node, the child is one of its
   #     children.
   #
   # @export: the parent is a block export, the child is its block
   #     backend.
   #
   # @qdev: the parent is a device, the child is its block backend.

> +{ 'enum': 'BlockParentType',
> +  'data': ['qdev', 'driver', 'export'] }

If you take my comment, change the order here as well.

> +
> +##
> +# @BdrvChildRefQdev:
> +#
> +# @qdev-id: the device's ID or QOM path
> +#
> +# Since 9.1
> +##
> +{ 'struct': 'BdrvChildRefQdev',
> +  'data': { 'qdev-id': 'str' } }
> +
> +##
> +# @BdrvChildRefExport:
> +#
> +# @export-id: block export identifier

block-export.json calls this "block export id" in some places, and
"block export identifier" in others.  *Sigh*

Nothing to see here, move on!

> +#
> +# Since 9.1
> +##
> +{ 'struct': 'BdrvChildRefExport',
> +  'data': { 'export-id': 'str' } }
> +
> +##
> +# @BdrvChildRefDriver:
> +#
> +# @node-name: the node name of the parent block node
> +#
> +# @child: name of the child to be replaced, like "file" or "backing"
> +#
> +# Since 9.1
> +##
> +{ 'struct': 'BdrvChildRefDriver',
> +  'data': { 'node-name': 'str', 'child': 'str' } }
> +
> +##
> +# @BlockdevReplace:
> +#
> +# @parent-type: type of the parent, which child is to be replaced

Suggest to scratch ", which child ..."

> +#
> +# @new-child: new child for replacement

Suggest "the new child".

> +#
> +# Since 9.1
> +##
> +{ 'union': 'BlockdevReplace',
> +  'base': {
> +      'parent-type': 'BlockParentType',
> +      'new-child': 'str'
> +  },
> +  'discriminator': 'parent-type',
> +  'data': {
> +      'qdev': 'BdrvChildRefQdev',
> +      'export': 'BdrvChildRefExport',
> +      'driver': 'BdrvChildRefDriver'
> +  } }
> +
> +##
> +# @blockdev-replace:
> +#
> +# Replace a block-node associated with device (selected by
> +# @qdev-id) or with block-export (selected by @export-id) or
> +# any child of block-node (selected by @node-name and @child)
> +# with @new-child block-node.

s/block-node/block node/ for consistency with existing usage.

Likewise, s/block-export/block export/.

> +#
> +# Features:
> +#
> +# @unstable: This command is experimental.
> +#
> +# Since 9.1
> +##
> +{ 'command': 'blockdev-replace', 'boxed': true,
> +  'features': [ 'unstable' ],
> +  'data': 'BlockdevReplace' }
> diff --git a/stubs/blk-by-qdev-id.c b/stubs/blk-by-qdev-id.c
> new file mode 100644
> index 0000000000..5ec9f755ee
> --- /dev/null
> +++ b/stubs/blk-by-qdev-id.c
> @@ -0,0 +1,13 @@
> +#include "qemu/osdep.h"
> +#include "qapi/error.h"
> +#include "sysemu/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.
> +     */

The last sentence is confusing.

> +    error_setg(errp, "Parameter 'parent-type' does not accept value 'qdev'");

I suggested this message.  I must have suffered from tunnel vision then.

The error message is good when the caller is qmp_blockdev_replace().
Then parameter @parent-type exists, and parameter value "qdev" cannot
work for any value of parameter "qdev-id" (which is @id here).

There are several more callers.  They don't use the stub now (or else
they wouldn't link before this patch).  But future callers may well use
it, and then the error message will likely be misleading.

v8 had

       error_setg(errp, "blk '%s' not found", id);

instead.  No good, because @id is not a "blk" (whatever that may be),
it's a qdev ID.  You offered "devices are not supported".  Less than
ideal, since it doesn't point to the argument that's causing the error,
but I figure it's the best we can do without refactoring.  Maybe
"can't select block backend by device ID".  Up to you.

> +    return NULL;
> +}
> diff --git a/stubs/meson.build b/stubs/meson.build
> index 772a3e817d..068998c1a5 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'))

Re: [PATCH v9 4/7] qapi: add blockdev-replace command

[Vladimir Sementsov-Ogievskiy]

On 18.07.24 15:00, 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>
>> ---
>>   blockdev.c             | 56 +++++++++++++++++++++++++++
>>   qapi/block-core.json   | 88 ++++++++++++++++++++++++++++++++++++++++++
>>   stubs/blk-by-qdev-id.c | 13 +++++++
>>   stubs/meson.build      |  1 +
>>   4 files changed, 158 insertions(+)
>>   create mode 100644 stubs/blk-by-qdev-id.c
>>
>> diff --git a/blockdev.c b/blockdev.c
>> index ba7e90b06e..2190467022 100644
>> --- a/blockdev.c
>> +++ b/blockdev.c
>> @@ -3559,6 +3559,62 @@ void qmp_x_blockdev_set_iothread(const char *node_name, StrOrNull *iothread,
>>       bdrv_try_change_aio_context(bs, new_context, NULL, errp);
>>   }
>>   
>> +void qmp_blockdev_replace(BlockdevReplace *repl, Error **errp)
>> +{
>> +    BdrvChild *child = NULL;
>> +    BlockDriverState *new_child_bs;
>> +
>> +    if (repl->parent_type == BLOCK_PARENT_TYPE_DRIVER) {
>> +        BlockDriverState *parent_bs;
>> +
>> +        parent_bs = bdrv_find_node(repl->u.driver.node_name);
>> +        if (!parent_bs) {
>> +            error_setg(errp, "Block driver node with node-name '%s' not "
>> +                       "found", repl->u.driver.node_name);
>> +            return;
>> +        }
>> +
>> +        child = bdrv_find_child(parent_bs, repl->u.driver.child);
>> +        if (!child) {
>> +            error_setg(errp, "Block driver node '%s' doesn't have child "
>> +                       "named '%s'", repl->u.driver.node_name,
>> +                       repl->u.driver.child);
>> +            return;
>> +        }
>> +    } else {
>> +        /* Other types are similar, they work through blk */
>> +        BlockBackend *blk;
>> +        bool is_qdev = repl->parent_type == BLOCK_PARENT_TYPE_QDEV;
>> +        const char *id =
>> +            is_qdev ? repl->u.qdev.qdev_id : repl->u.export.export_id;
>> +
>> +        assert(is_qdev || repl->parent_type == BLOCK_PARENT_TYPE_EXPORT);
>> +
>> +        blk = is_qdev ? blk_by_qdev_id(id, errp) : blk_by_export_id(id, errp);
> 
> blk_by_export_id() finds export @exp, and returns the associated block
> backend exp->blk.  Fine.
> 
> blk_by_qdev_id() finds the device, and then searches @block_backends for
> a blk with blk->dev == blk.  If a device has more than one block
> backend, you get the one first in @block_backends.  I figure that's the
> one created first.
> 
> Interface issue: when a device has multiple block backends, only one of
> them can be replaced, and which one is kind of random.
> 
> Do such devices exist?

Hmm.. I expect, they don't. If there is a problem, it's pre-existing, all callers of qmp_get_blk are affected. But honestly, I don't know.

> 
> If no, could they exist?
> 
> If yes, what should we do about it now?

Maybe, continue the loop in blk_by_dev() up the the end, and if two blk found for the device, return an error? Or even abort?

And add check to blk_attach_dev(), that we don't attach same device to several blks.

> 
>> +        if (!blk) {
>> +            return;
>> +        }
>> +
>> +        child = blk_root(blk);
>> +        if (!child) {
>> +            error_setg(errp, "%s '%s' is empty, nothing to replace",
>> +                       is_qdev ? "Device" : "Export", id);
>> +            return;
>> +        }
>> +    }
>> +
>> +    assert(child);
>> +    assert(child->bs);
>> +
>> +    new_child_bs = bdrv_find_node(repl->new_child);
>> +    if (!new_child_bs) {
>> +        error_setg(errp, "Node '%s' not found", repl->new_child);
>> +        return;
>> +    }
>> +
>> +    bdrv_replace_child_bs(child, 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 df5e07debd..0a6f08a6e0 100644
>> --- a/qapi/block-core.json
>> +++ b/qapi/block-core.json
>> @@ -6148,3 +6148,91 @@
>>   ##
>>   { 'struct': 'DummyBlockCoreForceArrays',
>>     'data': { 'unused-block-graph-info': ['BlockGraphInfo'] } }
>> +
>> +##
>> +# @BlockParentType:
>> +#
>> +# @qdev: block device, such as created by device_add, and denoted by
>> +#     qdev-id
>> +#
>> +# @driver: block driver node, such as created by blockdev-add, and
>> +#     denoted by node-name
> 
> node-name and child?

Hmm. I'd say that parent is denoted only by node-name, as parent is node itself (the fact that we have separate BdrvChild structure as a parent I'd keep as internal realization). But parent may have several children, and concrete child is denoted by @child.

> 
>> +#
>> +# @export: block export, such created by block-export-add, and
>> +#     denoted by export-id
>> +#
>> +# Since 9.1
>> +##
> 
> I'm kind of unhappy with this doc comment.  I feel makes sense only in
> the context of its use.  Let me try to avoid that:
> 
>     # @driver: the parent is a block node, the child is one of its
>     #     children.
>     #
>     # @export: the parent is a block export, the child is its block
>     #     backend.
>     #
>     # @qdev: the parent is a device, the child is its block backend.

Sounds good to me and leaves no questions)

> 
>> +{ 'enum': 'BlockParentType',
>> +  'data': ['qdev', 'driver', 'export'] }
> 
> If you take my comment, change the order here as well.
> 
>> +
>> +##
>> +# @BdrvChildRefQdev:
>> +#
>> +# @qdev-id: the device's ID or QOM path
>> +#
>> +# Since 9.1
>> +##
>> +{ 'struct': 'BdrvChildRefQdev',
>> +  'data': { 'qdev-id': 'str' } }
>> +
>> +##
>> +# @BdrvChildRefExport:
>> +#
>> +# @export-id: block export identifier
> 
> block-export.json calls this "block export id" in some places, and
> "block export identifier" in others.  *Sigh*
> 
> Nothing to see here, move on!
> 
>> +#
>> +# Since 9.1
>> +##
>> +{ 'struct': 'BdrvChildRefExport',
>> +  'data': { 'export-id': 'str' } }
>> +
>> +##
>> +# @BdrvChildRefDriver:
>> +#
>> +# @node-name: the node name of the parent block node
>> +#
>> +# @child: name of the child to be replaced, like "file" or "backing"
>> +#
>> +# Since 9.1
>> +##
>> +{ 'struct': 'BdrvChildRefDriver',
>> +  'data': { 'node-name': 'str', 'child': 'str' } }
>> +
>> +##
>> +# @BlockdevReplace:
>> +#
>> +# @parent-type: type of the parent, which child is to be replaced
> 
> Suggest to scratch ", which child ..."
> 
>> +#
>> +# @new-child: new child for replacement
> 
> Suggest "the new child".
> 
>> +#
>> +# Since 9.1
>> +##
>> +{ 'union': 'BlockdevReplace',
>> +  'base': {
>> +      'parent-type': 'BlockParentType',
>> +      'new-child': 'str'
>> +  },
>> +  'discriminator': 'parent-type',
>> +  'data': {
>> +      'qdev': 'BdrvChildRefQdev',
>> +      'export': 'BdrvChildRefExport',
>> +      'driver': 'BdrvChildRefDriver'
>> +  } }
>> +
>> +##
>> +# @blockdev-replace:
>> +#
>> +# Replace a block-node associated with device (selected by
>> +# @qdev-id) or with block-export (selected by @export-id) or
>> +# any child of block-node (selected by @node-name and @child)
>> +# with @new-child block-node.
> 
> s/block-node/block node/ for consistency with existing usage.
> 
> Likewise, s/block-export/block export/.
> 
>> +#
>> +# Features:
>> +#
>> +# @unstable: This command is experimental.
>> +#
>> +# Since 9.1
>> +##
>> +{ 'command': 'blockdev-replace', 'boxed': true,
>> +  'features': [ 'unstable' ],
>> +  'data': 'BlockdevReplace' }
>> diff --git a/stubs/blk-by-qdev-id.c b/stubs/blk-by-qdev-id.c
>> new file mode 100644
>> index 0000000000..5ec9f755ee
>> --- /dev/null
>> +++ b/stubs/blk-by-qdev-id.c
>> @@ -0,0 +1,13 @@
>> +#include "qemu/osdep.h"
>> +#include "qapi/error.h"
>> +#include "sysemu/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.
>> +     */
> 
> The last sentence is confusing.
> 
>> +    error_setg(errp, "Parameter 'parent-type' does not accept value 'qdev'");
> 
> I suggested this message.  I must have suffered from tunnel vision then.
> 
> The error message is good when the caller is qmp_blockdev_replace().
> Then parameter @parent-type exists, and parameter value "qdev" cannot
> work for any value of parameter "qdev-id" (which is @id here).
> 
> There are several more callers.  They don't use the stub now (or else
> they wouldn't link before this patch).  But future callers may well use
> it, and then the error message will likely be misleading.
> 
> v8 had
> 
>         error_setg(errp, "blk '%s' not found", id);
> 
> instead.  No good, because @id is not a "blk" (whatever that may be),
> it's a qdev ID.  You offered "devices are not supported".  Less than
> ideal, since it doesn't point to the argument that's causing the error,
> but I figure it's the best we can do without refactoring.  Maybe
> "can't select block backend by device ID".  Up to you.

"selecting block backend by device ID is not supported" may be?


> 
>> +    return NULL;
>> +}
>> diff --git a/stubs/meson.build b/stubs/meson.build
>> index 772a3e817d..068998c1a5 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'))
> 

-- 
Best regards,
Vladimir

Re: [PATCH v9 4/7] qapi: add blockdev-replace command

[Vladimir Sementsov-Ogievskiy]

On 26.06.24 14:53, Vladimir Sementsov-Ogievskiy wrote:
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index df5e07debd..0a6f08a6e0 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -6148,3 +6148,91 @@
>   ##
>   { 'struct': 'DummyBlockCoreForceArrays',
>     'data': { 'unused-block-graph-info': ['BlockGraphInfo'] } }
> +
> +##
> +# @BlockParentType:
> +#
> +# @qdev: block device, such as created by device_add, and denoted by
> +#     qdev-id
> +#
> +# @driver: block driver node, such as created by blockdev-add, and
> +#     denoted by node-name
> +#
> +# @export: block export, such created by block-export-add, and
> +#     denoted by export-id
> +#
> +# Since 9.1
> +##
> +{ 'enum': 'BlockParentType',
> +  'data': ['qdev', 'driver', 'export'] }
> +
> +##
> +# @BdrvChildRefQdev:
> +#
> +# @qdev-id: the device's ID or QOM path
> +#
> +# Since 9.1
> +##
> +{ 'struct': 'BdrvChildRefQdev',
> +  'data': { 'qdev-id': 'str' } }
> +
> +##
> +# @BdrvChildRefExport:
> +#
> +# @export-id: block export identifier
> +#
> +# Since 9.1
> +##
> +{ 'struct': 'BdrvChildRefExport',
> +  'data': { 'export-id': 'str' } }
> +
> +##
> +# @BdrvChildRefDriver:
> +#
> +# @node-name: the node name of the parent block node
> +#
> +# @child: name of the child to be replaced, like "file" or "backing"
> +#
> +# Since 9.1
> +##
> +{ 'struct': 'BdrvChildRefDriver',
> +  'data': { 'node-name': 'str', 'child': 'str' } }
> +
> +##
> +# @BlockdevReplace:
> +#
> +# @parent-type: type of the parent, which child is to be replaced
> +#
> +# @new-child: new child for replacement
> +#
> +# Since 9.1
> +##
> +{ 'union': 'BlockdevReplace',
> +  'base': {
> +      'parent-type': 'BlockParentType',
> +      'new-child': 'str'
> +  },
> +  'discriminator': 'parent-type',
> +  'data': {
> +      'qdev': 'BdrvChildRefQdev',
> +      'export': 'BdrvChildRefExport',
> +      'driver': 'BdrvChildRefDriver'
> +  } }
> +
> +##
> +# @blockdev-replace:
> +#
> +# Replace a block-node associated with device (selected by
> +# @qdev-id) or with block-export (selected by @export-id) or
> +# any child of block-node (selected by @node-name and @child)
> +# with @new-child block-node.
> +#
> +# Features:
> +#
> +# @unstable: This command is experimental.
> +#
> +# Since 9.1
> +##
> +{ 'command': 'blockdev-replace', 'boxed': true,
> +  'features': [ 'unstable' ],
> +  'data': 'BlockdevReplace' }


Looking back at this all, I now have another idea: instead of trying to unite different types of parents, maybe just publish concept of BdrvChild to QAPI? So that it will have unique id. Like for block-nodes, it could be auto-generated or specified by user.

Then we'll add parameters for commands:

device-add
    root-child-slot-id: ID

block-export-add
    block-child-slot-id: ID

and for block-drivers we already have BlockdevRef structure, it only lacks an id.

{ 'alternate': 'BlockdevRef',
   'data': { 'definition': 'BlockdevOptions',
             'reference': 'str' } }

hmm.. Could it be as simple as


{ 'alternate': 'BlockdevRef',
   'base': { '*child-slot-id': 'str' },
   'data': { 'definition': 'BlockdevOptions',
             'reference': 'str' } }

?

Unfortunately, no: "../qapi/block-core.json:4781: alternate has unknown key 'base'"

Anyway, I believe, some solution should exist, probably by improving QAPI generator. Or, add "child-slot-id" to base of BlockdevOptions, and add virtual "reference" BlockdevDriver, to handle case with reference.


----

And finally, the new command becomes as simple as:

{ 'command': 'blockdev-replace',
   'data': {'child-slot': 'str', 'new-child': 'str' } }

-- 
Best regards,
Vladimir

Re: [PATCH v9 4/7] qapi: add blockdev-replace command

[Vladimir Sementsov-Ogievskiy]

On 02.10.24 17:41, Vladimir Sementsov-Ogievskiy wrote:
> On 26.06.24 14:53, Vladimir Sementsov-Ogievskiy wrote:
>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>> index df5e07debd..0a6f08a6e0 100644
>> --- a/qapi/block-core.json
>> +++ b/qapi/block-core.json
>> @@ -6148,3 +6148,91 @@
>>   ##
>>   { 'struct': 'DummyBlockCoreForceArrays',
>>     'data': { 'unused-block-graph-info': ['BlockGraphInfo'] } }
>> +
>> +##
>> +# @BlockParentType:
>> +#
>> +# @qdev: block device, such as created by device_add, and denoted by
>> +#     qdev-id
>> +#
>> +# @driver: block driver node, such as created by blockdev-add, and
>> +#     denoted by node-name
>> +#
>> +# @export: block export, such created by block-export-add, and
>> +#     denoted by export-id
>> +#
>> +# Since 9.1
>> +##
>> +{ 'enum': 'BlockParentType',
>> +  'data': ['qdev', 'driver', 'export'] }
>> +
>> +##
>> +# @BdrvChildRefQdev:
>> +#
>> +# @qdev-id: the device's ID or QOM path
>> +#
>> +# Since 9.1
>> +##
>> +{ 'struct': 'BdrvChildRefQdev',
>> +  'data': { 'qdev-id': 'str' } }
>> +
>> +##
>> +# @BdrvChildRefExport:
>> +#
>> +# @export-id: block export identifier
>> +#
>> +# Since 9.1
>> +##
>> +{ 'struct': 'BdrvChildRefExport',
>> +  'data': { 'export-id': 'str' } }
>> +
>> +##
>> +# @BdrvChildRefDriver:
>> +#
>> +# @node-name: the node name of the parent block node
>> +#
>> +# @child: name of the child to be replaced, like "file" or "backing"
>> +#
>> +# Since 9.1
>> +##
>> +{ 'struct': 'BdrvChildRefDriver',
>> +  'data': { 'node-name': 'str', 'child': 'str' } }
>> +
>> +##
>> +# @BlockdevReplace:
>> +#
>> +# @parent-type: type of the parent, which child is to be replaced
>> +#
>> +# @new-child: new child for replacement
>> +#
>> +# Since 9.1
>> +##
>> +{ 'union': 'BlockdevReplace',
>> +  'base': {
>> +      'parent-type': 'BlockParentType',
>> +      'new-child': 'str'
>> +  },
>> +  'discriminator': 'parent-type',
>> +  'data': {
>> +      'qdev': 'BdrvChildRefQdev',
>> +      'export': 'BdrvChildRefExport',
>> +      'driver': 'BdrvChildRefDriver'
>> +  } }
>> +
>> +##
>> +# @blockdev-replace:
>> +#
>> +# Replace a block-node associated with device (selected by
>> +# @qdev-id) or with block-export (selected by @export-id) or
>> +# any child of block-node (selected by @node-name and @child)
>> +# with @new-child block-node.
>> +#
>> +# Features:
>> +#
>> +# @unstable: This command is experimental.
>> +#
>> +# Since 9.1
>> +##
>> +{ 'command': 'blockdev-replace', 'boxed': true,
>> +  'features': [ 'unstable' ],
>> +  'data': 'BlockdevReplace' }
> 
> 
> Looking back at this all, I now have another idea: instead of trying to unite different types of parents, maybe just publish concept of BdrvChild to QAPI? So that it will have unique id. Like for block-nodes, it could be auto-generated or specified by user.
> 
> Then we'll add parameters for commands:
> 
> device-add
>     root-child-slot-id: ID
> 
> block-export-add
>     block-child-slot-id: ID
> 
> and for block-drivers we already have BlockdevRef structure, it only lacks an id.
> 
> { 'alternate': 'BlockdevRef',
>    'data': { 'definition': 'BlockdevOptions',
>              'reference': 'str' } }
> 
> hmm.. Could it be as simple as
> 
> 
> { 'alternate': 'BlockdevRef',
>    'base': { '*child-slot-id': 'str' },
>    'data': { 'definition': 'BlockdevOptions',
>              'reference': 'str' } }
> 
> ?

Oops that was obviously impossible idea :) Then, I think the only way is to introduce virtual "reference" BlockdevDriver, with only one parameter { 'reference': 'str' }, this way user will be able to specify

file: {driver: reference, reference: NODE_NAME, child-slot-id: NEW_ID}

> 
> Unfortunately, no: "../qapi/block-core.json:4781: alternate has unknown key 'base'"
> 
> Anyway, I believe, some solution should exist, probably by improving QAPI generator. Or, add "child-slot-id" to base of BlockdevOptions, and add virtual "reference" BlockdevDriver, to handle case with reference.
> 
> 
> ----
> 
> And finally, the new command becomes as simple as:
> 
> { 'command': 'blockdev-replace',
>    'data': {'child-slot': 'str', 'new-child': 'str' } }
> 

-- 
Best regards,
Vladimir

Re: [PATCH v9 4/7] qapi: add blockdev-replace command

[Kevin Wolf]

Am 04.10.2024 um 19:01 hat Vladimir Sementsov-Ogievskiy geschrieben:
> On 02.10.24 17:41, Vladimir Sementsov-Ogievskiy wrote:
> > On 26.06.24 14:53, Vladimir Sementsov-Ogievskiy wrote:
> > > diff --git a/qapi/block-core.json b/qapi/block-core.json
> > > index df5e07debd..0a6f08a6e0 100644
> > > --- a/qapi/block-core.json
> > > +++ b/qapi/block-core.json
> > > @@ -6148,3 +6148,91 @@
> > >   ##
> > >   { 'struct': 'DummyBlockCoreForceArrays',
> > >     'data': { 'unused-block-graph-info': ['BlockGraphInfo'] } }
> > > +
> > > +##
> > > +# @BlockParentType:
> > > +#
> > > +# @qdev: block device, such as created by device_add, and denoted by
> > > +#     qdev-id
> > > +#
> > > +# @driver: block driver node, such as created by blockdev-add, and
> > > +#     denoted by node-name
> > > +#
> > > +# @export: block export, such created by block-export-add, and
> > > +#     denoted by export-id
> > > +#
> > > +# Since 9.1
> > > +##
> > > +{ 'enum': 'BlockParentType',
> > > +  'data': ['qdev', 'driver', 'export'] }
> > > +
> > > +##
> > > +# @BdrvChildRefQdev:
> > > +#
> > > +# @qdev-id: the device's ID or QOM path
> > > +#
> > > +# Since 9.1
> > > +##
> > > +{ 'struct': 'BdrvChildRefQdev',
> > > +  'data': { 'qdev-id': 'str' } }
> > > +
> > > +##
> > > +# @BdrvChildRefExport:
> > > +#
> > > +# @export-id: block export identifier
> > > +#
> > > +# Since 9.1
> > > +##
> > > +{ 'struct': 'BdrvChildRefExport',
> > > +  'data': { 'export-id': 'str' } }
> > > +
> > > +##
> > > +# @BdrvChildRefDriver:
> > > +#
> > > +# @node-name: the node name of the parent block node
> > > +#
> > > +# @child: name of the child to be replaced, like "file" or "backing"
> > > +#
> > > +# Since 9.1
> > > +##
> > > +{ 'struct': 'BdrvChildRefDriver',
> > > +  'data': { 'node-name': 'str', 'child': 'str' } }
> > > +
> > > +##
> > > +# @BlockdevReplace:
> > > +#
> > > +# @parent-type: type of the parent, which child is to be replaced
> > > +#
> > > +# @new-child: new child for replacement
> > > +#
> > > +# Since 9.1
> > > +##
> > > +{ 'union': 'BlockdevReplace',
> > > +  'base': {
> > > +      'parent-type': 'BlockParentType',
> > > +      'new-child': 'str'
> > > +  },
> > > +  'discriminator': 'parent-type',
> > > +  'data': {
> > > +      'qdev': 'BdrvChildRefQdev',
> > > +      'export': 'BdrvChildRefExport',
> > > +      'driver': 'BdrvChildRefDriver'
> > > +  } }
> > > +
> > > +##
> > > +# @blockdev-replace:
> > > +#
> > > +# Replace a block-node associated with device (selected by
> > > +# @qdev-id) or with block-export (selected by @export-id) or
> > > +# any child of block-node (selected by @node-name and @child)
> > > +# with @new-child block-node.
> > > +#
> > > +# Features:
> > > +#
> > > +# @unstable: This command is experimental.
> > > +#
> > > +# Since 9.1
> > > +##
> > > +{ 'command': 'blockdev-replace', 'boxed': true,
> > > +  'features': [ 'unstable' ],
> > > +  'data': 'BlockdevReplace' }
> > 
> > 
> > Looking back at this all, I now have another idea: instead of trying
> > to unite different types of parents, maybe just publish concept of
> > BdrvChild to QAPI? So that it will have unique id. Like for
> > block-nodes, it could be auto-generated or specified by user.
> > 
> > Then we'll add parameters for commands:
> > 
> > device-add
> >     root-child-slot-id: ID
> > 
> > block-export-add
> >     block-child-slot-id: ID
> > 
> > and for block-drivers we already have BlockdevRef structure, it only
> > lacks an id.
> > 
> > { 'alternate': 'BlockdevRef',
> >    'data': { 'definition': 'BlockdevOptions',
> >              'reference': 'str' } }
> > 
> > hmm.. Could it be as simple as
> > 
> > 
> > { 'alternate': 'BlockdevRef',
> >    'base': { '*child-slot-id': 'str' },
> >    'data': { 'definition': 'BlockdevOptions',
> >              'reference': 'str' } }
> > 
> > ?
> 
> Oops that was obviously impossible idea :) Then, I think the only way
> is to introduce virtual "reference" BlockdevDriver, with only one
> parameter { 'reference': 'str' }, this way user will be able to
> specify
> 
> file: {driver: reference, reference: NODE_NAME, child-slot-id: NEW_ID}

I don't think adding such a hack would make the interface any nicer
compared to what you have now with the union.

If we want to get rid of the union, I think the best course of action
would unifying the namespaces (so that nodes, exports and devices can't
share the same ID) and then we could just accept a universal 'id' along
with 'child'.

This would mean having 'child' even for devices, which feels
unnecessary at least in the common case, but it would at the same time
resolve Markus' concern if there could be any devices with multiple
BlockBackends.

I can only think of isa-fdc that used to be such a device, but that was
just incorrect modelling on the qdev level. Not sure if there are actual
legitimate use cases for having multiple BlockBackends.

Anyway, for the moment, I would suggest going ahead with the union.

Kevin

Re: [PATCH v9 4/7] qapi: add blockdev-replace command

[Vladimir Sementsov-Ogievskiy]

On 18.10.24 16:59, Kevin Wolf wrote:
> If we want to get rid of the union, I think the best course of action
> would unifying the namespaces (so that nodes, exports and devices can't
> share the same ID) and then we could just accept a universal 'id' along
> with 'child'.

Maybe we can go this way even without explicit restriction (which should
some how go through deprecation period, etc), but simply look for the id
among nodes, devices and exports and if found more than one parent - fail.

And we document, that id should not be ambiguous, should not match more
than one parent object. So, those who want to use new command will care
to make unique ids.

-- 
Best regards,
Vladimir

[PATCH v9 5/7] 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 f6292f459a..601475e835 100644
--- a/block.c
+++ b/block.c
@@ -6326,7 +6326,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 57beae7982..8744a1171e 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 f2fe0f8078..16863d37cf 100644
--- a/include/block/export.h
+++ b/include/block/export.h
@@ -82,6 +82,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..2fc1da953b
--- /dev/null
+++ b/stubs/blk-exp-find-by-blk.c
@@ -0,0 +1,9 @@
+#include "qemu/osdep.h"
+#include "sysemu/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 068998c1a5..cbe30f94e8 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.34.1

[PATCH v9 6/7] 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 ea48af4a7b..8a5fd01eac 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -1108,6 +1108,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.34.1

[PATCH v9 7/7] 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     | 236 ++++++++++++++++++
 tests/qemu-iotests/tests/filter-insertion.out |   5 +
 2 files changed, 241 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..02a0978f96
--- /dev/null
+++ b/tests/qemu-iotests/tests/filter-insertion
@@ -0,0 +1,236 @@
+#!/usr/bin/env python3
+#
+# Tests for inserting and removing filters in a block graph.
+#
+# Copyright (c) 2022 Virtuozzo International GmbH.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+
+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-type': 'driver',
+            'node-name': '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-type': 'driver',
+            'node-name': '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-type': 'qdev',
+            'qdev-id': 'sda',
+            '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-type': 'qdev',
+            'qdev-id': 'sda',
+            '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-type': 'export',
+            'export-id': 'exp1',
+            '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-type': 'export',
+            'export-id': 'exp2',
+            '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-type': 'export',
+            'export-id': 'exp1',
+            'new-child': 'disk0'
+        })
+
+        vm.cmd('blockdev-replace', {
+            'parent-type': 'export',
+            'export-id': 'exp2',
+            '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.34.1