[v6,3/4] qmp: add monitor command to add/remove a child

The new QMP command name is x-blockdev-change. It justs for adding/removing
quorum's child now, and don't support all kinds of children, all kinds of
operations, nor all block drivers. So it is experimental now.

Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
Signed-off-by: Gonglei <arei.gonglei@huawei.com>
---
 blockdev.c           | 76 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 qapi/block-core.json | 40 +++++++++++++++++++++++++++
 qmp-commands.hx      | 50 ++++++++++++++++++++++++++++++++++
 3 files changed, 166 insertions(+)

On Fri 16 Oct 2015 10:57:45 AM CEST, Wen Congyang <wency@cn.fujitsu.com> wrote:

> The new QMP command name is x-blockdev-change. It justs for
> adding/removing quorum's child now, and don't support all kinds of
> children, all kinds of operations, nor all block drivers. So it is
> experimental now.

I might have missed some discussion, why were the -add and -delete 

> +# @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.
> +#
> +# @node: the new node-name which will be added.
> +#
> +# 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' } }

Do you really need two separate 'child' and 'node' parameters? If the
operation is 'add' you can only use 'node', if it is 'delete, you can
only use 'child'. It seems to me that you can simply have one 'node'
parameter and use it for both ...

Berto

On 11/05/2015 09:49 PM, Alberto Garcia wrote:
> On Fri 16 Oct 2015 10:57:45 AM CEST, Wen Congyang <wency@cn.fujitsu.com> wrote:
> 
>> The new QMP command name is x-blockdev-change. It justs for
>> adding/removing quorum's child now, and don't support all kinds of
>> children, all kinds of operations, nor all block drivers. So it is
>> experimental now.
> 
> I might have missed some discussion, why were the -add and -delete 

This monitor command can be used to implement: add, delete, insert, remove,
replace... Currently, I only implement add and delete operation.

> 
>> +# @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.
>> +#
>> +# @node: the new node-name which will be added.
>> +#
>> +# 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' } }
> 
> Do you really need two separate 'child' and 'node' parameters? If the
> operation is 'add' you can only use 'node', if it is 'delete, you can
> only use 'child'. It seems to me that you can simply have one 'node'
> parameter and use it for both ...

parent and child already exist in the BDS graph, and node is a new node.
In the furture, we may need to implement insert opetioan, and this operation
needs such three BDSes.

Thanks
Wen Congyang

> 
> Berto
> .
>

Sorry again for the late review, here are my comments:

On Fri 16 Oct 2015 10:57:45 AM CEST, Wen Congyang wrote:
> +void qmp_x_blockdev_change(ChangeOperation op, const char *parent,
> +                           bool has_child, const char *child,
> +                           bool has_new_node, const char *new_node,
> +                           Error **errp)

You are using different names for the parameters here: 'op', 'parent',
'child', 'new_node'; in the JSON file the first and last one are named
'operation' and 'node'.

> +    parent_bs = bdrv_lookup_bs(parent, parent, &local_err);
> +    if (!parent_bs) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }

You don't need to change it if you don't want but you can use errp
directly here and spare the error_propagate() call.

> +
> +    switch(op) {
> +    case CHANGE_OPERATION_ADD:
> +        if (has_child) {
> +            error_setg(errp, "The operation %s doesn't support the parameter child",
> +                       ChangeOperation_lookup[op]);
> +            return;
> +        }

This line goes over 80 columns, please use scripts/checkpatch.pl to
check the style of the code.

> +        if (!has_new_node) {
> +            error_setg(errp, "The operation %s needs the parameter new_node",
> +                       ChangeOperation_lookup[op]);
> +            return;
> +        }
> +        break;
> +    case CHANGE_OPERATION_DELETE:
> +        if (has_new_node) {
> +            error_setg(errp, "The operation %s doesn't support the parameter node",
> +                       ChangeOperation_lookup[op]);
> +            return;
> +        }
> +        if (!has_child) {
> +            error_setg(errp, "The operation %s needs the parameter child",
> +                       ChangeOperation_lookup[op]);
> +            return;
> +        }

I still think that having two optional parameters here makes things
unnecessarily complex.

If in the future we want to add a new operation that needs a new
parameter then we can add it then, but I think that both 'add' and
'delete' can work perfectly fine with a single 'node' parameter.

Does anyone else have an opinion about this?

> +    default:
> +        break;
> +    }

This is unreachable so you can add a g_assert_not_reached() here.

> +##
> +# @ChangeOperation:
> +#
> +# An enumeration of block device change operation.

How about something like "An enumeration of possible change operations
on a block device" ?

> +# @add: Add a new block driver state to a existed block driver state.

s/a existed/an existing/

> +# @x-blockdev-change
> +#
> +# Dynamic reconfigure the block driver state graph. It can be used to

"Dynamically reconfigure"

> +# add, remove, insert, replace a block driver state. Currently only

"insert or replace"

> +# the Quorum driver implements this feature to add and remove its child.
> +# This is useful to fix a broken quorum child.

"add and remove its child" -> "add or remove a child"

> +#
> +# @operation: the chanage operation. It can be add, delete.

s/chanage/change/

> +#
> +# @parent: the id or node name of which node will be changed.

How about "the id or name of the node that will be changed" ?

> +#
> +# @child: the child node-name which will be deleted.
> +#
> +# @node: the new node-name which will be added.

"The name of the node that will be deleted"
"The name of the node that will be added"

Or, if you merge both parameters, "...that will be added or deleted".

> +#
> +# Note: this command is experimental, and not a stable API.

"and not a stable API" -> "and does not have a stable API", or "and its
API is not stable".

Berto

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).
Depending on whether node is given and whether the child already exists,
this may add, remove or replace a child.

Kevin

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.

> 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.

Thanks
Wen Congyang

> 
> Kevin
> .
>

On 11/09/2015 10:42 PM, Alberto Garcia wrote:
> Sorry again for the late review, here are my comments:
> 
> On Fri 16 Oct 2015 10:57:45 AM CEST, Wen Congyang wrote:
>> +void qmp_x_blockdev_change(ChangeOperation op, const char *parent,
>> +                           bool has_child, const char *child,
>> +                           bool has_new_node, const char *new_node,
>> +                           Error **errp)
> 
> You are using different names for the parameters here: 'op', 'parent',
> 'child', 'new_node'; in the JSON file the first and last one are named
> 'operation' and 'node'.

OK, I will fix it in the next version

> 
>> +    parent_bs = bdrv_lookup_bs(parent, parent, &local_err);
>> +    if (!parent_bs) {
>> +        error_propagate(errp, local_err);
>> +        return;
>> +    }
> 
> You don't need to change it if you don't want but you can use errp
> directly here and spare the error_propagate() call.

Too many codes in qemu use local_err and error_propagate(). I think
errp can be NOT NULL here(in which case?).

> 
>> +
>> +    switch(op) {
>> +    case CHANGE_OPERATION_ADD:
>> +        if (has_child) {
>> +            error_setg(errp, "The operation %s doesn't support the parameter child",
>> +                       ChangeOperation_lookup[op]);
>> +            return;
>> +        }
> 
> This line goes over 80 columns, please use scripts/checkpatch.pl to
> check the style of the code.

I forgot to do it...

> 
>> +        if (!has_new_node) {
>> +            error_setg(errp, "The operation %s needs the parameter new_node",
>> +                       ChangeOperation_lookup[op]);
>> +            return;
>> +        }
>> +        break;
>> +    case CHANGE_OPERATION_DELETE:
>> +        if (has_new_node) {
>> +            error_setg(errp, "The operation %s doesn't support the parameter node",
>> +                       ChangeOperation_lookup[op]);
>> +            return;
>> +        }
>> +        if (!has_child) {
>> +            error_setg(errp, "The operation %s needs the parameter child",
>> +                       ChangeOperation_lookup[op]);
>> +            return;
>> +        }
> 
> I still think that having two optional parameters here makes things
> unnecessarily complex.
> 
> If in the future we want to add a new operation that needs a new
> parameter then we can add it then, but I think that both 'add' and
> 'delete' can work perfectly fine with a single 'node' parameter.
> 
> Does anyone else have an opinion about this?
> 
>> +    default:
>> +        break;
>> +    }
> 
> This is unreachable so you can add a g_assert_not_reached() here.

OK

> 
>> +##
>> +# @ChangeOperation:
>> +#
>> +# An enumeration of block device change operation.
> 
> How about something like "An enumeration of possible change operations
> on a block device" ?
> 
>> +# @add: Add a new block driver state to a existed block driver state.
> 
> s/a existed/an existing/
> 
>> +# @x-blockdev-change
>> +#
>> +# Dynamic reconfigure the block driver state graph. It can be used to
> 
> "Dynamically reconfigure"
> 
>> +# add, remove, insert, replace a block driver state. Currently only
> 
> "insert or replace"
> 
>> +# the Quorum driver implements this feature to add and remove its child.
>> +# This is useful to fix a broken quorum child.
> 
> "add and remove its child" -> "add or remove a child"
> 
>> +#
>> +# @operation: the chanage operation. It can be add, delete.
> 
> s/chanage/change/
> 
>> +#
>> +# @parent: the id or node name of which node will be changed.
> 
> How about "the id or name of the node that will be changed" ?
> 
>> +#
>> +# @child: the child node-name which will be deleted.
>> +#
>> +# @node: the new node-name which will be added.
> 
> "The name of the node that will be deleted"
> "The name of the node that will be added"
> 
> Or, if you merge both parameters, "...that will be added or deleted".
> 
>> +#
>> +# Note: this command is experimental, and not a stable API.
> 
> "and not a stable API" -> "and does not have a stable API", or "and its
> API is not stable".

Thanks for your review, all will be fixed in the next version

Wen Congyang

> 
> Berto
> .
>

Wen Congyang <wency@cn.fujitsu.com> writes:

> On 11/09/2015 10:42 PM, Alberto Garcia wrote:
>> Sorry again for the late review, here are my comments:
>> 
>> On Fri 16 Oct 2015 10:57:45 AM CEST, Wen Congyang wrote:
>>> +void qmp_x_blockdev_change(ChangeOperation op, const char *parent,
>>> +                           bool has_child, const char *child,
>>> +                           bool has_new_node, const char *new_node,
>>> +                           Error **errp)
>> 
>> You are using different names for the parameters here: 'op', 'parent',
>> 'child', 'new_node'; in the JSON file the first and last one are named
>> 'operation' and 'node'.
>
> OK, I will fix it in the next version
>
>> 
>>> +    parent_bs = bdrv_lookup_bs(parent, parent, &local_err);
>>> +    if (!parent_bs) {
>>> +        error_propagate(errp, local_err);
>>> +        return;
>>> +    }
>> 
>> You don't need to change it if you don't want but you can use errp
>> directly here and spare the error_propagate() call.
>
> Too many codes in qemu use local_err and error_propagate(). I think
> errp can be NOT NULL here(in which case?).

It's usually advisable not to rely on "all callers pass non-null value
to parameter errp" arguments, because they're non-local and tend to be
brittle.

error.h attempts to provide guidance:

 * Receive an error and pass it on to the caller:
 *     Error *err = NULL;
 *     foo(arg, &err);
 *     if (err) {
 *         handle the error...
 *         error_propagate(errp, err);
 *     }
 * where Error **errp is a parameter, by convention the last one.
 *
 * Do *not* "optimize" this to
 *     foo(arg, errp);
 *     if (*errp) { // WRONG!
 *         handle the error...
 *     }
 * because errp may be NULL!
 *
 * But when all you do with the error is pass it on, please use
 *     foo(arg, errp);
 * for readability.

Since all you do with local_err in the quoted code snippet is pass it
on, the last paragraph applies, and you can simplify to:

    parent_bs = bdrv_lookup_bs(parent, parent, errp);
    if (!parent_bs) {
        return;
    }

Whether errp can be null doesn't matter.

[...]

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.
> 
>> 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.

Hi kevin, I still wait for your reply...

Thanks
Wen Congyang

> 
> Thanks
> Wen Congyang
> 
>>
>> Kevin
>> .
>>
> 
> 
> .
>

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.

Kevin

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
> .
>

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