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

[Manos Pitsidianakis <el13635@mail.ntua.gr>]

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

On Tue, Aug 15, 2017 at 05:12:42PM -0500, Eric Blake wrote:
>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:

Thanks for the tip, I will use it from now on!
>
>
>> +++ 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)?

If this is used for filter nodes, I suppose only between would make 
sense (for now). Is there a use case for the latter?
>
>
>> +#    }
>> +# <- { '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: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]