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