Re: [Qemu-devel] [PATCH RFC] block: add block-insert-node QMP command

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3052 bytes --]

On 08/15/2017 02:45 AM, Manos Pitsidianakis wrote:
> block-insert-node and its pair command block-remove-node provide runtime
> insertion and removal of filter nodes.
> 
> block-insert-node takes a (parent, child) and (node, child) pair of
> edges and unrefs the (parent, child) BdrvChild relationship and creates
> a new (parent, node) child with the same BdrvChildRole.
> 
> This is a different approach than x-blockdev-change which uses the driver
> methods bdrv_add_child() and bdrv_del_child(),
> 
> Signed-off-by: Manos Pitsidianakis <el13635@mail.ntua.gr>
> ---
>  block.c                    |  192 ++++++++
>  blockdev.c                 |   44 ++
>  include/block/block.h      |    6 +
>  qapi/block-core.json       |   60 +++
>  tests/qemu-iotests/193     |  241 ++++++++++
>  tests/qemu-iotests/193.out | 1116 ++++++++++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/group   |    1 +
>  7 files changed, 1660 insertions(+)
>  create mode 100755 tests/qemu-iotests/193
>  create mode 100644 tests/qemu-iotests/193.out

You may want to look at using scripts/git.orderfile, to rearrange your
patch so that interface changes (.json, .h) occur before implementation
(.c).  For now, I'm just focusing on the interface:


> +++ b/qapi/block-core.json
> @@ -3947,3 +3947,63 @@
>    'data' : { 'parent': 'str',
>               '*child': 'str',
>               '*node': 'str' } }
> +
> +##
> +# @block-insert-node:
> +#
> +# Insert a filter node between a specific edge in the block driver state graph.
> +# @parent:  the name of the parent node or device
> +# @node:    the name of the node to insert under parent
> +# @child:   the name of the child of both node and parent

Is this always going to be between two existing nodes, or can this
command also be used to insert at the end of the chain (for example, if
parent or child is omitted)?


> +#    }
> +# <- { 'return': {} }
> +#
> +##

Missing 'Since: 2.11'.

> +{ 'command': 'block-insert-node',
> +  'data': { 'parent': 'str',
> +             'child': 'str',
> +             'node': 'str'} }

For now, it looks like you require all arguments, and therefore this is
always insertion in the middle.

> +##
> +# @block-remove-node:
> +#
> +# Remove a filter node between two other nodes in the block driver state graph.
> +# @parent:  the name of the parent node or device
> +# @node:    the name of the node to remove from parent
> +# @child:   the name of the child of node which will go under parent
> +##
> +{ 'command': 'block-remove-node',
> +  'data': { 'parent': 'str',
> +             'child': 'str',
> +             'node': 'str'} }

Likewise missing 2.11.

Overall I'm not seeing problems with the interface from the UI
perspective, but I have not been paying close attention to your larger
efforts on throttling nodes, so I hope other reviewers will chime in.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]