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

[Manos Pitsidianakis]

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 <address@hidden>
> > ---
> >  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

signature.asc
Description: PGP signature