Re: [Qemu-devel] [PATCH v6 3/4] qmp: add monitor command to add/remove a child

[Kevin Wolf]

Am 13.11.2015 um 12:19 hat Wen Congyang geschrieben:
> On 11/13/2015 06:53 PM, Kevin Wolf wrote:
> > Am 13.11.2015 um 11:25 hat Wen Congyang geschrieben:
> >> On 11/10/2015 09:40 AM, Wen Congyang wrote:
> >>> On 11/10/2015 12:04 AM, Kevin Wolf wrote:
> >>>> Am 16.10.2015 um 10:57 hat Wen Congyang geschrieben:
> >>>>> +##
> >>>>> +# @ChangeOperation:
> >>>>> +#
> >>>>> +# An enumeration of block device change operation.
> >>>>> +#
> >>>>> +# @add: Add a new block driver state to a existed block driver state.
> >>>>> +#
> >>>>> +# @delete: Delete a block driver state's child.
> >>>>> +#
> >>>>> +# Since: 2.5
> >>>>> +##
> >>>>> +{ 'enum': 'ChangeOperation',
> >>>>> +  'data': [ 'add', 'delete' ] }
> >>>>
> >>>> What's the advantage of this enum compared to separate QMP commands? The
> >>>> way it is specified here, ChangeOperation is already implicit by whether
> >>>> or not child and node are given.
> >>>>
> >>>>> +##
> >>>>> +# @x-blockdev-change
> >>>>> +#
> >>>>> +# Dynamic reconfigure the block driver state graph. It can be used to
> >>>>> +# add, remove, insert, replace a block driver state. Currently only
> >>>>> +# the Quorum driver implements this feature to add and remove its 
> >>>>> child.
> >>>>> +# This is useful to fix a broken quorum child.
> >>>>> +#
> >>>>> +# @operation: the chanage operation. It can be add, delete.
> >>>>> +#
> >>>>> +# @parent: the id or node name of which node will be changed.
> >>>>> +#
> >>>>> +# @child: the child node-name which will be deleted.
> >>>>
> >>>> #optional
> >>>>
> >>>> Must be present for operation = delete, must not be present otherwise.
> >>>>
> >>>>> +# @node: the new node-name which will be added.
> >>>>
> >>>> #optional
> >>>>
> >>>> Must be present for operation = add, must not be present otherwise.
> >>>>
> >>>>> +#
> >>>>> +# Note: this command is experimental, and not a stable API.
> >>>>> +#
> >>>>> +# Since: 2.5
> >>>>> +##
> >>>>> +{ 'command': 'x-blockdev-change',
> >>>>> +  'data' : { 'operation': 'ChangeOperation',
> >>>>> +             'parent': 'str',
> >>>>> +             '*child': 'str',
> >>>>> +             '*node': 'str' } }
> >>>>
> >>>> Let me suggest this alternative:
> >>>>
> >>>> { 'command': 'x-blockdev-change',
> >>>>   'data' : { 'parent': 'str',
> >>>>              'child': 'str',
> >>>>              '*node': 'str' } }
> >>>>
> >>>> child doesn't describe a node name then, but a child name (adds a
> >>>> dependency on my patches which add a name to BdrvChild, though).
> >>>
> >>> Where is the patch? I don't find it.
> > 
> > The current developement branch version is here:
> > 
> > http://repo.or.cz/qemu/kevin.git/commitdiff/b8f3aba84160564576a5a068398f20eca13768af
> > 
> > I hope to get the series merged early in the 2.6 cycle.
> > 
> >>>> Depending on whether node is given and whether the child already exists,
> >>>> this may add, remove or replace a child.
> >>>
> >>> If the user wants to insert a filter driver between parent and child, we
> >>> also needs three parameters: parent, child, node. So it is why I add the
> >>> parameter operation.
> > 
> > The child node is uniquely identified with parent node and child name,
> > so my version can't describe less than something including the child
> > node name.
> > 
> > The reverse isn't true, though: In theory, the same node could be
> > attached twice to the same parent in different roles. Knowing the node
> > name doesn't uniquely identify the child name then.
> 
> Thanks for your explanation, I understand why we use the child name, not
> the node name.
> 
> Do we need the parameter "operation" now? Or add this patameter in the 
> furture?

I think it's enough to add it later if needed. Or in fact, we'll
probably just replace this experimental command by something else then.

Kevin