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.1On 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
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
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
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'))
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
© 2016 - 2024 Red Hat, Inc.