| Message ID | 20240626115350.405778-5-vsementsov@yandex-team.ru |
|---|---|
| State | New |
| Headers | show |
| Series | blockdev-replace | expand |
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')) >
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' } }
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' } } >
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
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'))
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