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

[Wen Congyang]

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?

Thanks
Wen Congyang

> 
> Kevin
> .
>