[v3,4/5] qmp: add monitor command to add/remove a child

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           | 47 ++++++++++++++++++++++++++++++++++++++++++++++
 qapi/block-core.json | 34 +++++++++++++++++++++++++++++++++
 qmp-commands.hx      | 53 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 134 insertions(+)

On Thu, Sep 10, 2015 at 05:55:04PM +0800, Wen Congyang wrote:
> 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           | 47 ++++++++++++++++++++++++++++++++++++++++++++++
>  qapi/block-core.json | 34 +++++++++++++++++++++++++++++++++
>  qmp-commands.hx      | 53 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 134 insertions(+)
> 
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index e68a59f..b959577 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -2272,3 +2272,37 @@
>  ##
>  { 'command': 'block-set-write-threshold',
>    'data': { 'node-name': 'str', 'write-threshold': 'uint64' } }
> +
> +##
> +# @x-child-add
> +#
> +# Add a new child to the parent BDS. Currently only the Quorum driver
> +# implements this feature. This is useful to fix a broken quorum child.
> +#
> +# @parent: graph node name or id which the child will be added to.
> +#
> +# @child: graph node name that will be added.
> +#
> +# Note: this command is experimental, and not a stable API.
> +#
> +# Since: 2.5
> +##
> +{ 'command': 'x-child-add',
> +  'data' : { 'parent': 'str', 'child': 'str' } }
> +
> +##
> +# @child-del
> +#
> +# Remove a child from the parent BDS. Currently only the Quorum driver
> +# implements this feature. This is useful to fix a broken quorum child.
> +# Note, you can't remove a child if it would bring the quorum below its
> +# threshold.
> +#
> +# @parent: graph node name or id from which the child will removed.
> +#
> +# @child: graph node name that will be removed.
> +#
> +# Since: 2.5
> +##
> +{ 'command': 'child-del',
> +  'data' : { 'parent': 'str', 'child': 'str' } }

These command names are faaaar too generic. If this only applies to
block devices, then I'd expect something like 'block' as a prefix for
the command names. Likewise with your next hmp patch

Regards,
Daniel

On 09/10/2015 06:04 PM, Daniel P. Berrange wrote:
> On Thu, Sep 10, 2015 at 05:55:04PM +0800, Wen Congyang wrote:
>> 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           | 47 ++++++++++++++++++++++++++++++++++++++++++++++
>>  qapi/block-core.json | 34 +++++++++++++++++++++++++++++++++
>>  qmp-commands.hx      | 53 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 134 insertions(+)
>>
>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>> index e68a59f..b959577 100644
>> --- a/qapi/block-core.json
>> +++ b/qapi/block-core.json
>> @@ -2272,3 +2272,37 @@
>>  ##
>>  { 'command': 'block-set-write-threshold',
>>    'data': { 'node-name': 'str', 'write-threshold': 'uint64' } }
>> +
>> +##
>> +# @x-child-add
>> +#
>> +# Add a new child to the parent BDS. Currently only the Quorum driver
>> +# implements this feature. This is useful to fix a broken quorum child.
>> +#
>> +# @parent: graph node name or id which the child will be added to.
>> +#
>> +# @child: graph node name that will be added.
>> +#
>> +# Note: this command is experimental, and not a stable API.
>> +#
>> +# Since: 2.5
>> +##
>> +{ 'command': 'x-child-add',
>> +  'data' : { 'parent': 'str', 'child': 'str' } }
>> +
>> +##
>> +# @child-del
>> +#
>> +# Remove a child from the parent BDS. Currently only the Quorum driver
>> +# implements this feature. This is useful to fix a broken quorum child.
>> +# Note, you can't remove a child if it would bring the quorum below its
>> +# threshold.
>> +#
>> +# @parent: graph node name or id from which the child will removed.
>> +#
>> +# @child: graph node name that will be removed.
>> +#
>> +# Since: 2.5
>> +##
>> +{ 'command': 'child-del',
>> +  'data' : { 'parent': 'str', 'child': 'str' } }
> 
> These command names are faaaar too generic. If this only applies to
> block devices, then I'd expect something like 'block' as a prefix for
> the command names. Likewise with your next hmp patch

OK, I will fix it in the next version. I guess blockdev may be better.

Thanks
Wen Congyang

> 
> Regards,
> Daniel
>

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

> 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           | 47 ++++++++++++++++++++++++++++++++++++++++++++++
>  qapi/block-core.json | 34 +++++++++++++++++++++++++++++++++
>  qmp-commands.hx      | 53 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 134 insertions(+)
>
> diff --git a/blockdev.c b/blockdev.c
> index bd47756..0a40607 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -3413,6 +3413,53 @@ fail:
>      qmp_output_visitor_cleanup(ov);
>  }
>  
> +void qmp_x_child_add(const char *parent, const char *child,
> +                     Error **errp)
> +{
> +    BlockDriverState *parent_bs, *child_bs;
> +    Error *local_err = NULL;
> +
> +    parent_bs = bdrv_lookup_bs(parent, parent, &local_err);
> +    if (!parent_bs) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
> +
> +    child_bs = bdrv_find_node(child);
> +    if (!child_bs) {
> +        error_setg(errp, "Node '%s' not found", child);
> +        return;
> +    }
> +
> +    bdrv_add_child(parent_bs, child_bs, &local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +    }
> +}
> +
> +void qmp_child_del(const char *parent, const char *child, Error **errp)
> +{
> +    BlockDriverState *parent_bs, *child_bs;
> +    Error *local_err = NULL;
> +
> +    parent_bs = bdrv_lookup_bs(parent, parent, &local_err);
> +    if (!parent_bs) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
> +
> +    child_bs = bdrv_find_node(child);
> +    if (!child_bs) {
> +        error_setg(errp, "Node '%s' not found", child);
> +        return;
> +    }
> +
> +    bdrv_del_child(parent_bs, child_bs, &local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +    }
> +}
> +
>  BlockJobInfoList *qmp_query_block_jobs(Error **errp)
>  {
>      BlockJobInfoList *head = NULL, **p_next = &head;
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index e68a59f..b959577 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -2272,3 +2272,37 @@
>  ##
>  { 'command': 'block-set-write-threshold',
>    'data': { 'node-name': 'str', 'write-threshold': 'uint64' } }
> +
> +##
> +# @x-child-add
> +#
> +# Add a new child to the parent BDS. Currently only the Quorum driver
> +# implements this feature. This is useful to fix a broken quorum child.
> +#
> +# @parent: graph node name or id which the child will be added to.
> +#
> +# @child: graph node name that will be added.
> +#
> +# Note: this command is experimental, and not a stable API.
> +#
> +# Since: 2.5
> +##
> +{ 'command': 'x-child-add',
> +  'data' : { 'parent': 'str', 'child': 'str' } }
> +
> +##
> +# @child-del
> +#
> +# Remove a child from the parent BDS. Currently only the Quorum driver
> +# implements this feature. This is useful to fix a broken quorum child.
> +# Note, you can't remove a child if it would bring the quorum below its
> +# threshold.
> +#
> +# @parent: graph node name or id from which the child will removed.
> +#
> +# @child: graph node name that will be removed.
> +#
> +# Since: 2.5
> +##
> +{ 'command': 'child-del',
> +  'data' : { 'parent': 'str', 'child': 'str' } }

Why is x-child-add experimental, but child-del isn't?  Please explain
both in the schema and in the commit message.

> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 495670b..139a23b 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -4053,6 +4053,59 @@ Example:
>  EQMP
>  
>      {
> +        .name       = "x-child-add",
> +        .args_type  = "parent:B,child:B",
> +        .mhandler.cmd_new = qmp_marshal_input_x_child_add,
> +    },
> +
> +SQMP
> +x-child-add
> +------------
> +
> +Add a child to a quorum node.
> +
> +Arguments:
> +
> +- "parent": the quorum's id or node name
> +- "child": the child node-name which will be added

Node name parameters are usually named node-name or, if there's more
than one, FOO-node-name.  Unless we want to abandon that convention,
this should therefore be node-name and child-node-name, or parent-node
name and child-node-name.

> +
> +Note: this command is experimental, and not a stable API.
> +
> +Example:
> +
> +-> { "execute": "x-child-add",
> +    "arguments": { "parent": "disk1", "child": "new_node" } }
> +<- { "return": {} }
> +
> +EQMP
> +
> +    {
> +        .name        = "child-del",

Documentation and schema have x-child-add, actual command is child-add.
Oops.

> +        .args_type   = "parent:B,child:B",
> +        .mhandler.cmd_new = qmp_marshal_input_child_del,
> +    },
> +
> +SQMP
> +child-del
> +------------
> +
> +Delete a child from a quorum node. It can be used to remove a broken
> +quorum child.
> +
> +Arguments:
> +
> +- "parent": the quorum's id or node name
> +- "child": the child node-name which will be removed

Same comment as on x-child-add's parameter names.

> +
> +Example:
> +
> +-> { "execute": "child-del",
> +    "arguments": { "parent": "disk1", "child": "new_node" } }
> +<- { "return": {} }
> +
> +EQMP
> +
> +    {
>          .name       = "query-named-block-nodes",
>          .args_type  = "",
>          .mhandler.cmd_new = qmp_marshal_input_query_named_block_nodes,

Am 14.09.2015 um 16:36 hat Markus Armbruster geschrieben:
> Wen Congyang <wency@cn.fujitsu.com> writes:
> > diff --git a/qmp-commands.hx b/qmp-commands.hx
> > index 495670b..139a23b 100644
> > --- a/qmp-commands.hx
> > +++ b/qmp-commands.hx
> > @@ -4053,6 +4053,59 @@ Example:
> >  EQMP
> >  
> >      {
> > +        .name       = "x-child-add",
> > +        .args_type  = "parent:B,child:B",
> > +        .mhandler.cmd_new = qmp_marshal_input_x_child_add,
> > +    },
> > +
> > +SQMP
> > +x-child-add
> > +------------
> > +
> > +Add a child to a quorum node.
> > +
> > +Arguments:
> > +
> > +- "parent": the quorum's id or node name
> > +- "child": the child node-name which will be added
> 
> Node name parameters are usually named node-name or, if there's more
> than one, FOO-node-name.  Unless we want to abandon that convention,
> this should therefore be node-name and child-node-name, or parent-node
> name and child-node-name.

Didn't we come to the conclusion that foo-node-name is redundant and
should just be foo, leaving things like node-name only for cases where
there is no foo?

Kevin

Am 10.09.2015 um 11:55 hat Wen Congyang geschrieben:
> +##
> +# @x-child-add
> +#
> +# Add a new child to the parent BDS. Currently only the Quorum driver
> +# implements this feature. This is useful to fix a broken quorum child.
> +#
> +# @parent: graph node name or id which the child will be added to.
> +#
> +# @child: graph node name that will be added.
> +#
> +# Note: this command is experimental, and not a stable API.
> +#
> +# Since: 2.5
> +##
> +{ 'command': 'x-child-add',
> +  'data' : { 'parent': 'str', 'child': 'str' } }

This is probably not future-proof and only made for the special case of
quorum. Specifically, one thing I'm missing is some way to specfiy what
kind of child the new node is when a node can take different types of
children (e.g. bs->file and bs->backing_hd).

Kevin

Kevin Wolf <kwolf@redhat.com> writes:

> Am 14.09.2015 um 16:36 hat Markus Armbruster geschrieben:
>> Wen Congyang <wency@cn.fujitsu.com> writes:
>> > diff --git a/qmp-commands.hx b/qmp-commands.hx
>> > index 495670b..139a23b 100644
>> > --- a/qmp-commands.hx
>> > +++ b/qmp-commands.hx
>> > @@ -4053,6 +4053,59 @@ Example:
>> >  EQMP
>> >  
>> >      {
>> > +        .name       = "x-child-add",
>> > +        .args_type  = "parent:B,child:B",
>> > +        .mhandler.cmd_new = qmp_marshal_input_x_child_add,
>> > +    },
>> > +
>> > +SQMP
>> > +x-child-add
>> > +------------
>> > +
>> > +Add a child to a quorum node.
>> > +
>> > +Arguments:
>> > +
>> > +- "parent": the quorum's id or node name
>> > +- "child": the child node-name which will be added
>> 
>> Node name parameters are usually named node-name or, if there's more
>> than one, FOO-node-name.  Unless we want to abandon that convention,
>> this should therefore be node-name and child-node-name, or parent-node
>> name and child-node-name.
>
> Didn't we come to the conclusion that foo-node-name is redundant and
> should just be foo, leaving things like node-name only for cases where
> there is no foo?

I don't remember :)

I'm fine with simpler names.

On 09/14/2015 11:37 PM, Kevin Wolf wrote:
> Am 10.09.2015 um 11:55 hat Wen Congyang geschrieben:
>> +##
>> +# @x-child-add
>> +#
>> +# Add a new child to the parent BDS. Currently only the Quorum driver
>> +# implements this feature. This is useful to fix a broken quorum child.
>> +#
>> +# @parent: graph node name or id which the child will be added to.
>> +#
>> +# @child: graph node name that will be added.
>> +#
>> +# Note: this command is experimental, and not a stable API.
>> +#
>> +# Since: 2.5
>> +##
>> +{ 'command': 'x-child-add',
>> +  'data' : { 'parent': 'str', 'child': 'str' } }
> 
> This is probably not future-proof and only made for the special case of
> quorum. Specifically, one thing I'm missing is some way to specfiy what
> kind of child the new node is when a node can take different types of
> children (e.g. bs->file and bs->backing_hd).

Currently, we only add/remove quorum's child. We can add a new parameter
to specify the type in the furture to support more things.

Thanks
Wen Congyang

> 
> Kevin
> .
>

On 09/14/2015 10:36 PM, Markus Armbruster wrote:
> Wen Congyang <wency@cn.fujitsu.com> writes:
> 
>> 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           | 47 ++++++++++++++++++++++++++++++++++++++++++++++
>>  qapi/block-core.json | 34 +++++++++++++++++++++++++++++++++
>>  qmp-commands.hx      | 53 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 134 insertions(+)
>>
>> diff --git a/blockdev.c b/blockdev.c
>> index bd47756..0a40607 100644
>> --- a/blockdev.c
>> +++ b/blockdev.c
>> @@ -3413,6 +3413,53 @@ fail:
>>      qmp_output_visitor_cleanup(ov);
>>  }
>>  
>> +void qmp_x_child_add(const char *parent, const char *child,
>> +                     Error **errp)
>> +{
>> +    BlockDriverState *parent_bs, *child_bs;
>> +    Error *local_err = NULL;
>> +
>> +    parent_bs = bdrv_lookup_bs(parent, parent, &local_err);
>> +    if (!parent_bs) {
>> +        error_propagate(errp, local_err);
>> +        return;
>> +    }
>> +
>> +    child_bs = bdrv_find_node(child);
>> +    if (!child_bs) {
>> +        error_setg(errp, "Node '%s' not found", child);
>> +        return;
>> +    }
>> +
>> +    bdrv_add_child(parent_bs, child_bs, &local_err);
>> +    if (local_err) {
>> +        error_propagate(errp, local_err);
>> +    }
>> +}
>> +
>> +void qmp_child_del(const char *parent, const char *child, Error **errp)
>> +{
>> +    BlockDriverState *parent_bs, *child_bs;
>> +    Error *local_err = NULL;
>> +
>> +    parent_bs = bdrv_lookup_bs(parent, parent, &local_err);
>> +    if (!parent_bs) {
>> +        error_propagate(errp, local_err);
>> +        return;
>> +    }
>> +
>> +    child_bs = bdrv_find_node(child);
>> +    if (!child_bs) {
>> +        error_setg(errp, "Node '%s' not found", child);
>> +        return;
>> +    }
>> +
>> +    bdrv_del_child(parent_bs, child_bs, &local_err);
>> +    if (local_err) {
>> +        error_propagate(errp, local_err);
>> +    }
>> +}
>> +
>>  BlockJobInfoList *qmp_query_block_jobs(Error **errp)
>>  {
>>      BlockJobInfoList *head = NULL, **p_next = &head;
>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>> index e68a59f..b959577 100644
>> --- a/qapi/block-core.json
>> +++ b/qapi/block-core.json
>> @@ -2272,3 +2272,37 @@
>>  ##
>>  { 'command': 'block-set-write-threshold',
>>    'data': { 'node-name': 'str', 'write-threshold': 'uint64' } }
>> +
>> +##
>> +# @x-child-add
>> +#
>> +# Add a new child to the parent BDS. Currently only the Quorum driver
>> +# implements this feature. This is useful to fix a broken quorum child.
>> +#
>> +# @parent: graph node name or id which the child will be added to.
>> +#
>> +# @child: graph node name that will be added.
>> +#
>> +# Note: this command is experimental, and not a stable API.
>> +#
>> +# Since: 2.5
>> +##
>> +{ 'command': 'x-child-add',
>> +  'data' : { 'parent': 'str', 'child': 'str' } }
>> +
>> +##
>> +# @child-del
>> +#
>> +# Remove a child from the parent BDS. Currently only the Quorum driver
>> +# implements this feature. This is useful to fix a broken quorum child.
>> +# Note, you can't remove a child if it would bring the quorum below its
>> +# threshold.
>> +#
>> +# @parent: graph node name or id from which the child will removed.
>> +#
>> +# @child: graph node name that will be removed.
>> +#
>> +# Since: 2.5
>> +##
>> +{ 'command': 'child-del',
>> +  'data' : { 'parent': 'str', 'child': 'str' } }
> 
> Why is x-child-add experimental, but child-del isn't?  Please explain
> both in the schema and in the commit message.

No special reason. Should I put child-del in experimental namespace?

> 
>> diff --git a/qmp-commands.hx b/qmp-commands.hx
>> index 495670b..139a23b 100644
>> --- a/qmp-commands.hx
>> +++ b/qmp-commands.hx
>> @@ -4053,6 +4053,59 @@ Example:
>>  EQMP
>>  
>>      {
>> +        .name       = "x-child-add",
>> +        .args_type  = "parent:B,child:B",
>> +        .mhandler.cmd_new = qmp_marshal_input_x_child_add,
>> +    },
>> +
>> +SQMP
>> +x-child-add
>> +------------
>> +
>> +Add a child to a quorum node.
>> +
>> +Arguments:
>> +
>> +- "parent": the quorum's id or node name
>> +- "child": the child node-name which will be added
> 
> Node name parameters are usually named node-name or, if there's more
> than one, FOO-node-name.  Unless we want to abandon that convention,
> this should therefore be node-name and child-node-name, or parent-node
> name and child-node-name.

parent can be top BDS, so it can be id. node-name is a very common name,
and I think child or child-node-name is better.

> 
>> +
>> +Note: this command is experimental, and not a stable API.
>> +
>> +Example:
>> +
>> +-> { "execute": "x-child-add",
>> +    "arguments": { "parent": "disk1", "child": "new_node" } }
>> +<- { "return": {} }
>> +
>> +EQMP
>> +
>> +    {
>> +        .name        = "child-del",
> 
> Documentation and schema have x-child-add, actual command is child-add.
> Oops.

Here is child-del....

> 
>> +        .args_type   = "parent:B,child:B",
>> +        .mhandler.cmd_new = qmp_marshal_input_child_del,
>> +    },
>> +
>> +SQMP
>> +child-del
>> +------------
>> +
>> +Delete a child from a quorum node. It can be used to remove a broken
>> +quorum child.
>> +
>> +Arguments:
>> +
>> +- "parent": the quorum's id or node name
>> +- "child": the child node-name which will be removed
> 
> Same comment as on x-child-add's parameter names.
> 
>> +
>> +Example:
>> +
>> +-> { "execute": "child-del",
>> +    "arguments": { "parent": "disk1", "child": "new_node" } }
>> +<- { "return": {} }
>> +
>> +EQMP
>> +
>> +    {
>>          .name       = "query-named-block-nodes",
>>          .args_type  = "",
>>          .mhandler.cmd_new = qmp_marshal_input_query_named_block_nodes,
> .
>

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

> On 09/14/2015 10:36 PM, Markus Armbruster wrote:
>> Wen Congyang <wency@cn.fujitsu.com> writes:
>> 
>>> 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           | 47 ++++++++++++++++++++++++++++++++++++++++++++++
>>>  qapi/block-core.json | 34 +++++++++++++++++++++++++++++++++
>>>  qmp-commands.hx      | 53 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>  3 files changed, 134 insertions(+)
>>>
>>> diff --git a/blockdev.c b/blockdev.c
>>> index bd47756..0a40607 100644
>>> --- a/blockdev.c
>>> +++ b/blockdev.c
>>> @@ -3413,6 +3413,53 @@ fail:
>>>      qmp_output_visitor_cleanup(ov);
>>>  }
>>>  
>>> +void qmp_x_child_add(const char *parent, const char *child,
>>> +                     Error **errp)
>>> +{
>>> +    BlockDriverState *parent_bs, *child_bs;
>>> +    Error *local_err = NULL;
>>> +
>>> +    parent_bs = bdrv_lookup_bs(parent, parent, &local_err);
>>> +    if (!parent_bs) {
>>> +        error_propagate(errp, local_err);
>>> +        return;
>>> +    }
>>> +
>>> +    child_bs = bdrv_find_node(child);
>>> +    if (!child_bs) {
>>> +        error_setg(errp, "Node '%s' not found", child);
>>> +        return;
>>> +    }
>>> +
>>> +    bdrv_add_child(parent_bs, child_bs, &local_err);
>>> +    if (local_err) {
>>> +        error_propagate(errp, local_err);
>>> +    }
>>> +}
>>> +
>>> +void qmp_child_del(const char *parent, const char *child, Error **errp)
>>> +{
>>> +    BlockDriverState *parent_bs, *child_bs;
>>> +    Error *local_err = NULL;
>>> +
>>> +    parent_bs = bdrv_lookup_bs(parent, parent, &local_err);
>>> +    if (!parent_bs) {
>>> +        error_propagate(errp, local_err);
>>> +        return;
>>> +    }
>>> +
>>> +    child_bs = bdrv_find_node(child);
>>> +    if (!child_bs) {
>>> +        error_setg(errp, "Node '%s' not found", child);
>>> +        return;
>>> +    }
>>> +
>>> +    bdrv_del_child(parent_bs, child_bs, &local_err);
>>> +    if (local_err) {
>>> +        error_propagate(errp, local_err);
>>> +    }
>>> +}
>>> +
>>>  BlockJobInfoList *qmp_query_block_jobs(Error **errp)
>>>  {
>>>      BlockJobInfoList *head = NULL, **p_next = &head;
>>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>>> index e68a59f..b959577 100644
>>> --- a/qapi/block-core.json
>>> +++ b/qapi/block-core.json
>>> @@ -2272,3 +2272,37 @@
>>>  ##
>>>  { 'command': 'block-set-write-threshold',
>>>    'data': { 'node-name': 'str', 'write-threshold': 'uint64' } }
>>> +
>>> +##
>>> +# @x-child-add
>>> +#
>>> +# Add a new child to the parent BDS. Currently only the Quorum driver
>>> +# implements this feature. This is useful to fix a broken quorum child.
>>> +#
>>> +# @parent: graph node name or id which the child will be added to.
>>> +#
>>> +# @child: graph node name that will be added.
>>> +#
>>> +# Note: this command is experimental, and not a stable API.
>>> +#
>>> +# Since: 2.5
>>> +##
>>> +{ 'command': 'x-child-add',
>>> +  'data' : { 'parent': 'str', 'child': 'str' } }
>>> +
>>> +##
>>> +# @child-del
>>> +#
>>> +# Remove a child from the parent BDS. Currently only the Quorum driver
>>> +# implements this feature. This is useful to fix a broken quorum child.
>>> +# Note, you can't remove a child if it would bring the quorum below its
>>> +# threshold.
>>> +#
>>> +# @parent: graph node name or id from which the child will removed.
>>> +#
>>> +# @child: graph node name that will be removed.
>>> +#
>>> +# Since: 2.5
>>> +##
>>> +{ 'command': 'child-del',
>>> +  'data' : { 'parent': 'str', 'child': 'str' } }
>> 
>> Why is x-child-add experimental, but child-del isn't?  Please explain
>> both in the schema and in the commit message.
>
> No special reason. Should I put child-del in experimental namespace?

I found the reason for x-child-add in your v2:

    child-add
    ------------

    Add a child to a quorum node.

    This command is still a work in progress. It doesn't support all
    block drivers. Stay away from it unless you want it to help with
    its development.

Eric suggested to rename it to x-child-add, and you did.  Good.  You
also shortened the "work in progress" note to just "Note: this command
is experimental, and not a stable API."  I'd like to have a more verbose
note explaining *why* the command is experimental, both here and in
qmp-commands.hx.  "It doesn't support all block drivers" is a reason.
Are the any others?

Is child-del similarly unfinished?  If yes, make it x-child-del to save
us from later grief.

If no: is child-del is only useful together with x-child-add?  Then make
it x-child-del regardless.

>>> diff --git a/qmp-commands.hx b/qmp-commands.hx
>>> index 495670b..139a23b 100644
>>> --- a/qmp-commands.hx
>>> +++ b/qmp-commands.hx
>>> @@ -4053,6 +4053,59 @@ Example:
>>>  EQMP
>>>  
>>>      {
>>> +        .name       = "x-child-add",
>>> +        .args_type  = "parent:B,child:B",
>>> +        .mhandler.cmd_new = qmp_marshal_input_x_child_add,
>>> +    },
>>> +
>>> +SQMP
>>> +x-child-add
>>> +------------
>>> +
>>> +Add a child to a quorum node.
>>> +
>>> +Arguments:
>>> +
>>> +- "parent": the quorum's id or node name
>>> +- "child": the child node-name which will be added
>> 
>> Node name parameters are usually named node-name or, if there's more
>> than one, FOO-node-name.  Unless we want to abandon that convention,
>> this should therefore be node-name and child-node-name, or parent-node
>> name and child-node-name.
>
> parent can be top BDS, so it can be id. node-name is a very common name,
> and I think child or child-node-name is better.

Kevin pointed out we want to move to names without a -node-name suffix.

>>> +
>>> +Note: this command is experimental, and not a stable API.
>>> +
>>> +Example:
>>> +
>>> +-> { "execute": "x-child-add",
>>> +    "arguments": { "parent": "disk1", "child": "new_node" } }
>>> +<- { "return": {} }
>>> +
>>> +EQMP
>>> +
>>> +    {
>>> +        .name        = "child-del",
>> 
>> Documentation and schema have x-child-add, actual command is child-add.
>> Oops.
>
> Here is child-del....

You're right.  I got confused...

>>> +        .args_type   = "parent:B,child:B",
>>> +        .mhandler.cmd_new = qmp_marshal_input_child_del,
>>> +    },
>>> +
>>> +SQMP
>>> +child-del
>>> +------------
>>> +
>>> +Delete a child from a quorum node. It can be used to remove a broken
>>> +quorum child.
>>> +
>>> +Arguments:
>>> +
>>> +- "parent": the quorum's id or node name
>>> +- "child": the child node-name which will be removed
>> 
>> Same comment as on x-child-add's parameter names.
>> 
>>> +
>>> +Example:
>>> +
>>> +-> { "execute": "child-del",
>>> +    "arguments": { "parent": "disk1", "child": "new_node" } }
>>> +<- { "return": {} }
>>> +
>>> +EQMP
>>> +
>>> +    {
>>>          .name       = "query-named-block-nodes",
>>>          .args_type  = "",
>>>          .mhandler.cmd_new = qmp_marshal_input_query_named_block_nodes,
>> .
>>

On 09/15/2015 03:49 PM, Markus Armbruster wrote:
> Wen Congyang <wency@cn.fujitsu.com> writes:
> 
>> On 09/14/2015 10:36 PM, Markus Armbruster wrote:
>>> Wen Congyang <wency@cn.fujitsu.com> writes:
>>>
>>>> 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           | 47 ++++++++++++++++++++++++++++++++++++++++++++++
>>>>  qapi/block-core.json | 34 +++++++++++++++++++++++++++++++++
>>>>  qmp-commands.hx      | 53 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>  3 files changed, 134 insertions(+)
>>>>
>>>> diff --git a/blockdev.c b/blockdev.c
>>>> index bd47756..0a40607 100644
>>>> --- a/blockdev.c
>>>> +++ b/blockdev.c
>>>> @@ -3413,6 +3413,53 @@ fail:
>>>>      qmp_output_visitor_cleanup(ov);
>>>>  }
>>>>  
>>>> +void qmp_x_child_add(const char *parent, const char *child,
>>>> +                     Error **errp)
>>>> +{
>>>> +    BlockDriverState *parent_bs, *child_bs;
>>>> +    Error *local_err = NULL;
>>>> +
>>>> +    parent_bs = bdrv_lookup_bs(parent, parent, &local_err);
>>>> +    if (!parent_bs) {
>>>> +        error_propagate(errp, local_err);
>>>> +        return;
>>>> +    }
>>>> +
>>>> +    child_bs = bdrv_find_node(child);
>>>> +    if (!child_bs) {
>>>> +        error_setg(errp, "Node '%s' not found", child);
>>>> +        return;
>>>> +    }
>>>> +
>>>> +    bdrv_add_child(parent_bs, child_bs, &local_err);
>>>> +    if (local_err) {
>>>> +        error_propagate(errp, local_err);
>>>> +    }
>>>> +}
>>>> +
>>>> +void qmp_child_del(const char *parent, const char *child, Error **errp)
>>>> +{
>>>> +    BlockDriverState *parent_bs, *child_bs;
>>>> +    Error *local_err = NULL;
>>>> +
>>>> +    parent_bs = bdrv_lookup_bs(parent, parent, &local_err);
>>>> +    if (!parent_bs) {
>>>> +        error_propagate(errp, local_err);
>>>> +        return;
>>>> +    }
>>>> +
>>>> +    child_bs = bdrv_find_node(child);
>>>> +    if (!child_bs) {
>>>> +        error_setg(errp, "Node '%s' not found", child);
>>>> +        return;
>>>> +    }
>>>> +
>>>> +    bdrv_del_child(parent_bs, child_bs, &local_err);
>>>> +    if (local_err) {
>>>> +        error_propagate(errp, local_err);
>>>> +    }
>>>> +}
>>>> +
>>>>  BlockJobInfoList *qmp_query_block_jobs(Error **errp)
>>>>  {
>>>>      BlockJobInfoList *head = NULL, **p_next = &head;
>>>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>>>> index e68a59f..b959577 100644
>>>> --- a/qapi/block-core.json
>>>> +++ b/qapi/block-core.json
>>>> @@ -2272,3 +2272,37 @@
>>>>  ##
>>>>  { 'command': 'block-set-write-threshold',
>>>>    'data': { 'node-name': 'str', 'write-threshold': 'uint64' } }
>>>> +
>>>> +##
>>>> +# @x-child-add
>>>> +#
>>>> +# Add a new child to the parent BDS. Currently only the Quorum driver
>>>> +# implements this feature. This is useful to fix a broken quorum child.
>>>> +#
>>>> +# @parent: graph node name or id which the child will be added to.
>>>> +#
>>>> +# @child: graph node name that will be added.
>>>> +#
>>>> +# Note: this command is experimental, and not a stable API.
>>>> +#
>>>> +# Since: 2.5
>>>> +##
>>>> +{ 'command': 'x-child-add',
>>>> +  'data' : { 'parent': 'str', 'child': 'str' } }
>>>> +
>>>> +##
>>>> +# @child-del
>>>> +#
>>>> +# Remove a child from the parent BDS. Currently only the Quorum driver
>>>> +# implements this feature. This is useful to fix a broken quorum child.
>>>> +# Note, you can't remove a child if it would bring the quorum below its
>>>> +# threshold.
>>>> +#
>>>> +# @parent: graph node name or id from which the child will removed.
>>>> +#
>>>> +# @child: graph node name that will be removed.
>>>> +#
>>>> +# Since: 2.5
>>>> +##
>>>> +{ 'command': 'child-del',
>>>> +  'data' : { 'parent': 'str', 'child': 'str' } }
>>>
>>> Why is x-child-add experimental, but child-del isn't?  Please explain
>>> both in the schema and in the commit message.
>>
>> No special reason. Should I put child-del in experimental namespace?
> 
> I found the reason for x-child-add in your v2:
> 
>     child-add
>     ------------
> 
>     Add a child to a quorum node.
> 
>     This command is still a work in progress. It doesn't support all
>     block drivers. Stay away from it unless you want it to help with
>     its development.
> 
> Eric suggested to rename it to x-child-add, and you did.  Good.  You
> also shortened the "work in progress" note to just "Note: this command
> is experimental, and not a stable API."  I'd like to have a more verbose
> note explaining *why* the command is experimental, both here and in
> qmp-commands.hx.  "It doesn't support all block drivers" is a reason.
> Are the any others?

Currently, it only for quorum. But in the future, we can use this command
to do more thing. For example: bs->file, bs->backing_hd, ...

> 
> Is child-del similarly unfinished?  If yes, make it x-child-del to save
> us from later grief.
> 
> If no: is child-del is only useful together with x-child-add?  Then make
> it x-child-del regardless.

child-del is only useful together with x-child-add. I will rename it to
x-child-del in the next version.

Thanks
Wen Congyang

> 
>>>> diff --git a/qmp-commands.hx b/qmp-commands.hx
>>>> index 495670b..139a23b 100644
>>>> --- a/qmp-commands.hx
>>>> +++ b/qmp-commands.hx
>>>> @@ -4053,6 +4053,59 @@ Example:
>>>>  EQMP
>>>>  
>>>>      {
>>>> +        .name       = "x-child-add",
>>>> +        .args_type  = "parent:B,child:B",
>>>> +        .mhandler.cmd_new = qmp_marshal_input_x_child_add,
>>>> +    },
>>>> +
>>>> +SQMP
>>>> +x-child-add
>>>> +------------
>>>> +
>>>> +Add a child to a quorum node.
>>>> +
>>>> +Arguments:
>>>> +
>>>> +- "parent": the quorum's id or node name
>>>> +- "child": the child node-name which will be added
>>>
>>> Node name parameters are usually named node-name or, if there's more
>>> than one, FOO-node-name.  Unless we want to abandon that convention,
>>> this should therefore be node-name and child-node-name, or parent-node
>>> name and child-node-name.
>>
>> parent can be top BDS, so it can be id. node-name is a very common name,
>> and I think child or child-node-name is better.
> 
> Kevin pointed out we want to move to names without a -node-name suffix.
> 
>>>> +
>>>> +Note: this command is experimental, and not a stable API.
>>>> +
>>>> +Example:
>>>> +
>>>> +-> { "execute": "x-child-add",
>>>> +    "arguments": { "parent": "disk1", "child": "new_node" } }
>>>> +<- { "return": {} }
>>>> +
>>>> +EQMP
>>>> +
>>>> +    {
>>>> +        .name        = "child-del",
>>>
>>> Documentation and schema have x-child-add, actual command is child-add.
>>> Oops.
>>
>> Here is child-del....
> 
> You're right.  I got confused...
> 
>>>> +        .args_type   = "parent:B,child:B",
>>>> +        .mhandler.cmd_new = qmp_marshal_input_child_del,
>>>> +    },
>>>> +
>>>> +SQMP
>>>> +child-del
>>>> +------------
>>>> +
>>>> +Delete a child from a quorum node. It can be used to remove a broken
>>>> +quorum child.
>>>> +
>>>> +Arguments:
>>>> +
>>>> +- "parent": the quorum's id or node name
>>>> +- "child": the child node-name which will be removed
>>>
>>> Same comment as on x-child-add's parameter names.
>>>
>>>> +
>>>> +Example:
>>>> +
>>>> +-> { "execute": "child-del",
>>>> +    "arguments": { "parent": "disk1", "child": "new_node" } }
>>>> +<- { "return": {} }
>>>> +
>>>> +EQMP
>>>> +
>>>> +    {
>>>>          .name       = "query-named-block-nodes",
>>>>          .args_type  = "",
>>>>          .mhandler.cmd_new = qmp_marshal_input_query_named_block_nodes,
>>> .
>>>
> .
>

On Mon 14 Sep 2015 05:37:00 PM CEST, Kevin Wolf <kwolf@redhat.com> wrote:

>> +{ 'command': 'x-child-add',
>> +  'data' : { 'parent': 'str', 'child': 'str' } }
>
> This is probably not future-proof and only made for the special case
> of quorum. Specifically, one thing I'm missing is some way to specfiy
> what kind of child the new node is when a node can take different
> types of children (e.g. bs->file and bs->backing_hd).

Children here are specified by child roles, aren't they?

We could have a ChildRole enum with 'file', 'format' and 'backing' that
would be passed to this command and use the same enumeration in
bdrv_open_image() rather than a pointer to BdrvChildRole.

Berto

Am 15.09.2015 um 10:56 hat Alberto Garcia geschrieben:
> On Mon 14 Sep 2015 05:37:00 PM CEST, Kevin Wolf <kwolf@redhat.com> wrote:
> 
> >> +{ 'command': 'x-child-add',
> >> +  'data' : { 'parent': 'str', 'child': 'str' } }
> >
> > This is probably not future-proof and only made for the special case
> > of quorum. Specifically, one thing I'm missing is some way to specfiy
> > what kind of child the new node is when a node can take different
> > types of children (e.g. bs->file and bs->backing_hd).
> 
> Children here are specified by child roles, aren't they?

Possibly, but actually, currently I'm inclined to think that they
aren't. Child roles are still relatively new, though, so it's hard to
say if this is just because we didn't use them in that way so far, or
because they are really the wrong tool.

What I can say is that traditionally children are identified by option
names. Block drivers assign a ChildRole when processing the option, and
both are equivalent only if the same ChildRole is never used for two
different children. I believe that that's currently true, but I'm
doubtful whether it will remain this way (even looking at blkverify,
which has one &child_file and one &child_format, things start to look a
bit confusing).

Possibly the right and consistent way to change the set of children
would be through a QMP command exposing bdrv_reopen(), which would also
be used for changing other options at runtime.

My current pull request adds the qemu-io (and therefore HMP) way of
doing this, but finding a good QMP interface will still be a challenge.

> We could have a ChildRole enum with 'file', 'format' and 'backing' that
> would be passed to this command and use the same enumeration in
> bdrv_open_image() rather than a pointer to BdrvChildRole.

Yes, that would be an option if ChildRole turned out to be enough.

Kevin

Am 15.09.2015 um 11:20 hat Kevin Wolf geschrieben:
> Am 15.09.2015 um 10:56 hat Alberto Garcia geschrieben:
> > On Mon 14 Sep 2015 05:37:00 PM CEST, Kevin Wolf <kwolf@redhat.com> wrote:
> > 
> > >> +{ 'command': 'x-child-add',
> > >> +  'data' : { 'parent': 'str', 'child': 'str' } }
> > >
> > > This is probably not future-proof and only made for the special case
> > > of quorum. Specifically, one thing I'm missing is some way to specfiy
> > > what kind of child the new node is when a node can take different
> > > types of children (e.g. bs->file and bs->backing_hd).
> > 
> > Children here are specified by child roles, aren't they?
> 
> Possibly, but actually, currently I'm inclined to think that they
> aren't. Child roles are still relatively new, though, so it's hard to
> say if this is just because we didn't use them in that way so far, or
> because they are really the wrong tool.
> 
> What I can say is that traditionally children are identified by option
> names. Block drivers assign a ChildRole when processing the option, and
> both are equivalent only if the same ChildRole is never used for two
> different children. I believe that that's currently true, but I'm
> doubtful whether it will remain this way (even looking at blkverify,
> which has one &child_file and one &child_format, things start to look a
> bit confusing).

What I wanted to mention, but forgot to do, is that obviously we could
use more specific ChildRoles than &child_file and &child_format,
essentially creating one ChildRole per option and making them equivalent
this way.

I'm not sure if that would be a good or a bad idea.

> Possibly the right and consistent way to change the set of children
> would be through a QMP command exposing bdrv_reopen(), which would also
> be used for changing other options at runtime.
> 
> My current pull request adds the qemu-io (and therefore HMP) way of
> doing this, but finding a good QMP interface will still be a challenge.
> 
> > We could have a ChildRole enum with 'file', 'format' and 'backing' that
> > would be passed to this command and use the same enumeration in
> > bdrv_open_image() rather than a pointer to BdrvChildRole.
> 
> Yes, that would be an option if ChildRole turned out to be enough.
> 
> Kevin
>

On 09/15/2015 03:49 PM, Markus Armbruster wrote:
> Wen Congyang <wency@cn.fujitsu.com> writes:
> 
>> On 09/14/2015 10:36 PM, Markus Armbruster wrote:
>>> Wen Congyang <wency@cn.fujitsu.com> writes:
>>>
>>>> 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           | 47 ++++++++++++++++++++++++++++++++++++++++++++++
>>>>  qapi/block-core.json | 34 +++++++++++++++++++++++++++++++++
>>>>  qmp-commands.hx      | 53 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>  3 files changed, 134 insertions(+)
>>>>
>>>> diff --git a/blockdev.c b/blockdev.c
>>>> index bd47756..0a40607 100644
>>>> --- a/blockdev.c
>>>> +++ b/blockdev.c
>>>> @@ -3413,6 +3413,53 @@ fail:
>>>>      qmp_output_visitor_cleanup(ov);
>>>>  }
>>>>  
>>>> +void qmp_x_child_add(const char *parent, const char *child,
>>>> +                     Error **errp)
>>>> +{
>>>> +    BlockDriverState *parent_bs, *child_bs;
>>>> +    Error *local_err = NULL;
>>>> +
>>>> +    parent_bs = bdrv_lookup_bs(parent, parent, &local_err);
>>>> +    if (!parent_bs) {
>>>> +        error_propagate(errp, local_err);
>>>> +        return;
>>>> +    }
>>>> +
>>>> +    child_bs = bdrv_find_node(child);
>>>> +    if (!child_bs) {
>>>> +        error_setg(errp, "Node '%s' not found", child);
>>>> +        return;
>>>> +    }
>>>> +
>>>> +    bdrv_add_child(parent_bs, child_bs, &local_err);
>>>> +    if (local_err) {
>>>> +        error_propagate(errp, local_err);
>>>> +    }
>>>> +}
>>>> +
>>>> +void qmp_child_del(const char *parent, const char *child, Error **errp)
>>>> +{
>>>> +    BlockDriverState *parent_bs, *child_bs;
>>>> +    Error *local_err = NULL;
>>>> +
>>>> +    parent_bs = bdrv_lookup_bs(parent, parent, &local_err);
>>>> +    if (!parent_bs) {
>>>> +        error_propagate(errp, local_err);
>>>> +        return;
>>>> +    }
>>>> +
>>>> +    child_bs = bdrv_find_node(child);
>>>> +    if (!child_bs) {
>>>> +        error_setg(errp, "Node '%s' not found", child);
>>>> +        return;
>>>> +    }
>>>> +
>>>> +    bdrv_del_child(parent_bs, child_bs, &local_err);
>>>> +    if (local_err) {
>>>> +        error_propagate(errp, local_err);
>>>> +    }
>>>> +}
>>>> +
>>>>  BlockJobInfoList *qmp_query_block_jobs(Error **errp)
>>>>  {
>>>>      BlockJobInfoList *head = NULL, **p_next = &head;
>>>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>>>> index e68a59f..b959577 100644
>>>> --- a/qapi/block-core.json
>>>> +++ b/qapi/block-core.json
>>>> @@ -2272,3 +2272,37 @@
>>>>  ##
>>>>  { 'command': 'block-set-write-threshold',
>>>>    'data': { 'node-name': 'str', 'write-threshold': 'uint64' } }
>>>> +
>>>> +##
>>>> +# @x-child-add
>>>> +#
>>>> +# Add a new child to the parent BDS. Currently only the Quorum driver
>>>> +# implements this feature. This is useful to fix a broken quorum child.
>>>> +#
>>>> +# @parent: graph node name or id which the child will be added to.
>>>> +#
>>>> +# @child: graph node name that will be added.
>>>> +#
>>>> +# Note: this command is experimental, and not a stable API.
>>>> +#
>>>> +# Since: 2.5
>>>> +##
>>>> +{ 'command': 'x-child-add',
>>>> +  'data' : { 'parent': 'str', 'child': 'str' } }
>>>> +
>>>> +##
>>>> +# @child-del
>>>> +#
>>>> +# Remove a child from the parent BDS. Currently only the Quorum driver
>>>> +# implements this feature. This is useful to fix a broken quorum child.
>>>> +# Note, you can't remove a child if it would bring the quorum below its
>>>> +# threshold.
>>>> +#
>>>> +# @parent: graph node name or id from which the child will removed.
>>>> +#
>>>> +# @child: graph node name that will be removed.
>>>> +#
>>>> +# Since: 2.5
>>>> +##
>>>> +{ 'command': 'child-del',
>>>> +  'data' : { 'parent': 'str', 'child': 'str' } }
>>>
>>> Why is x-child-add experimental, but child-del isn't?  Please explain
>>> both in the schema and in the commit message.
>>
>> No special reason. Should I put child-del in experimental namespace?
> 
> I found the reason for x-child-add in your v2:
> 
>     child-add
>     ------------
> 
>     Add a child to a quorum node.
> 
>     This command is still a work in progress. It doesn't support all
>     block drivers. Stay away from it unless you want it to help with
>     its development.
> 
> Eric suggested to rename it to x-child-add, and you did.  Good.  You
> also shortened the "work in progress" note to just "Note: this command
> is experimental, and not a stable API."  I'd like to have a more verbose
> note explaining *why* the command is experimental, both here and in
> qmp-commands.hx.  "It doesn't support all block drivers" is a reason.
> Are the any others?
> 
> Is child-del similarly unfinished?  If yes, make it x-child-del to save
> us from later grief.
> 
> If no: is child-del is only useful together with x-child-add?  Then make
> it x-child-del regardless.

I have another question: if the command is experimental, we have the prefix "x-".
Which prefix is used for hmp command?

Thanks
Wen Congyang

> 
>>>> diff --git a/qmp-commands.hx b/qmp-commands.hx
>>>> index 495670b..139a23b 100644
>>>> --- a/qmp-commands.hx
>>>> +++ b/qmp-commands.hx
>>>> @@ -4053,6 +4053,59 @@ Example:
>>>>  EQMP
>>>>  
>>>>      {
>>>> +        .name       = "x-child-add",
>>>> +        .args_type  = "parent:B,child:B",
>>>> +        .mhandler.cmd_new = qmp_marshal_input_x_child_add,
>>>> +    },
>>>> +
>>>> +SQMP
>>>> +x-child-add
>>>> +------------
>>>> +
>>>> +Add a child to a quorum node.
>>>> +
>>>> +Arguments:
>>>> +
>>>> +- "parent": the quorum's id or node name
>>>> +- "child": the child node-name which will be added
>>>
>>> Node name parameters are usually named node-name or, if there's more
>>> than one, FOO-node-name.  Unless we want to abandon that convention,
>>> this should therefore be node-name and child-node-name, or parent-node
>>> name and child-node-name.
>>
>> parent can be top BDS, so it can be id. node-name is a very common name,
>> and I think child or child-node-name is better.
> 
> Kevin pointed out we want to move to names without a -node-name suffix.
> 
>>>> +
>>>> +Note: this command is experimental, and not a stable API.
>>>> +
>>>> +Example:
>>>> +
>>>> +-> { "execute": "x-child-add",
>>>> +    "arguments": { "parent": "disk1", "child": "new_node" } }
>>>> +<- { "return": {} }
>>>> +
>>>> +EQMP
>>>> +
>>>> +    {
>>>> +        .name        = "child-del",
>>>
>>> Documentation and schema have x-child-add, actual command is child-add.
>>> Oops.
>>
>> Here is child-del....
> 
> You're right.  I got confused...
> 
>>>> +        .args_type   = "parent:B,child:B",
>>>> +        .mhandler.cmd_new = qmp_marshal_input_child_del,
>>>> +    },
>>>> +
>>>> +SQMP
>>>> +child-del
>>>> +------------
>>>> +
>>>> +Delete a child from a quorum node. It can be used to remove a broken
>>>> +quorum child.
>>>> +
>>>> +Arguments:
>>>> +
>>>> +- "parent": the quorum's id or node name
>>>> +- "child": the child node-name which will be removed
>>>
>>> Same comment as on x-child-add's parameter names.
>>>
>>>> +
>>>> +Example:
>>>> +
>>>> +-> { "execute": "child-del",
>>>> +    "arguments": { "parent": "disk1", "child": "new_node" } }
>>>> +<- { "return": {} }
>>>> +
>>>> +EQMP
>>>> +
>>>> +    {
>>>>          .name       = "query-named-block-nodes",
>>>>          .args_type  = "",
>>>>          .mhandler.cmd_new = qmp_marshal_input_query_named_block_nodes,
>>> .
>>>
> .
>

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

> On 09/15/2015 03:49 PM, Markus Armbruster wrote:
>> Wen Congyang <wency@cn.fujitsu.com> writes:
>> 
>>> On 09/14/2015 10:36 PM, Markus Armbruster wrote:
>>>> Wen Congyang <wency@cn.fujitsu.com> writes:
>>>>
>>>>> 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           | 47 ++++++++++++++++++++++++++++++++++++++++++++++
>>>>>  qapi/block-core.json | 34 +++++++++++++++++++++++++++++++++
>>>>>  qmp-commands.hx      | 53 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>>  3 files changed, 134 insertions(+)
>>>>>
>>>>> diff --git a/blockdev.c b/blockdev.c
>>>>> index bd47756..0a40607 100644
>>>>> --- a/blockdev.c
>>>>> +++ b/blockdev.c
>>>>> @@ -3413,6 +3413,53 @@ fail:
>>>>>      qmp_output_visitor_cleanup(ov);
>>>>>  }
>>>>>  
>>>>> +void qmp_x_child_add(const char *parent, const char *child,
>>>>> +                     Error **errp)
>>>>> +{
>>>>> +    BlockDriverState *parent_bs, *child_bs;
>>>>> +    Error *local_err = NULL;
>>>>> +
>>>>> +    parent_bs = bdrv_lookup_bs(parent, parent, &local_err);
>>>>> +    if (!parent_bs) {
>>>>> +        error_propagate(errp, local_err);
>>>>> +        return;
>>>>> +    }
>>>>> +
>>>>> +    child_bs = bdrv_find_node(child);
>>>>> +    if (!child_bs) {
>>>>> +        error_setg(errp, "Node '%s' not found", child);
>>>>> +        return;
>>>>> +    }
>>>>> +
>>>>> +    bdrv_add_child(parent_bs, child_bs, &local_err);
>>>>> +    if (local_err) {
>>>>> +        error_propagate(errp, local_err);
>>>>> +    }
>>>>> +}
>>>>> +
>>>>> +void qmp_child_del(const char *parent, const char *child, Error **errp)
>>>>> +{
>>>>> +    BlockDriverState *parent_bs, *child_bs;
>>>>> +    Error *local_err = NULL;
>>>>> +
>>>>> +    parent_bs = bdrv_lookup_bs(parent, parent, &local_err);
>>>>> +    if (!parent_bs) {
>>>>> +        error_propagate(errp, local_err);
>>>>> +        return;
>>>>> +    }
>>>>> +
>>>>> +    child_bs = bdrv_find_node(child);
>>>>> +    if (!child_bs) {
>>>>> +        error_setg(errp, "Node '%s' not found", child);
>>>>> +        return;
>>>>> +    }
>>>>> +
>>>>> +    bdrv_del_child(parent_bs, child_bs, &local_err);
>>>>> +    if (local_err) {
>>>>> +        error_propagate(errp, local_err);
>>>>> +    }
>>>>> +}
>>>>> +
>>>>>  BlockJobInfoList *qmp_query_block_jobs(Error **errp)
>>>>>  {
>>>>>      BlockJobInfoList *head = NULL, **p_next = &head;
>>>>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>>>>> index e68a59f..b959577 100644
>>>>> --- a/qapi/block-core.json
>>>>> +++ b/qapi/block-core.json
>>>>> @@ -2272,3 +2272,37 @@
>>>>>  ##
>>>>>  { 'command': 'block-set-write-threshold',
>>>>>    'data': { 'node-name': 'str', 'write-threshold': 'uint64' } }
>>>>> +
>>>>> +##
>>>>> +# @x-child-add
>>>>> +#
>>>>> +# Add a new child to the parent BDS. Currently only the Quorum driver
>>>>> +# implements this feature. This is useful to fix a broken quorum child.
>>>>> +#
>>>>> +# @parent: graph node name or id which the child will be added to.
>>>>> +#
>>>>> +# @child: graph node name that will be added.
>>>>> +#
>>>>> +# Note: this command is experimental, and not a stable API.
>>>>> +#
>>>>> +# Since: 2.5
>>>>> +##
>>>>> +{ 'command': 'x-child-add',
>>>>> +  'data' : { 'parent': 'str', 'child': 'str' } }
>>>>> +
>>>>> +##
>>>>> +# @child-del
>>>>> +#
>>>>> +# Remove a child from the parent BDS. Currently only the Quorum driver
>>>>> +# implements this feature. This is useful to fix a broken quorum child.
>>>>> +# Note, you can't remove a child if it would bring the quorum below its
>>>>> +# threshold.
>>>>> +#
>>>>> +# @parent: graph node name or id from which the child will removed.
>>>>> +#
>>>>> +# @child: graph node name that will be removed.
>>>>> +#
>>>>> +# Since: 2.5
>>>>> +##
>>>>> +{ 'command': 'child-del',
>>>>> +  'data' : { 'parent': 'str', 'child': 'str' } }
>>>>
>>>> Why is x-child-add experimental, but child-del isn't?  Please explain
>>>> both in the schema and in the commit message.
>>>
>>> No special reason. Should I put child-del in experimental namespace?
>> 
>> I found the reason for x-child-add in your v2:
>> 
>>     child-add
>>     ------------
>> 
>>     Add a child to a quorum node.
>> 
>>     This command is still a work in progress. It doesn't support all
>>     block drivers. Stay away from it unless you want it to help with
>>     its development.
>> 
>> Eric suggested to rename it to x-child-add, and you did.  Good.  You
>> also shortened the "work in progress" note to just "Note: this command
>> is experimental, and not a stable API."  I'd like to have a more verbose
>> note explaining *why* the command is experimental, both here and in
>> qmp-commands.hx.  "It doesn't support all block drivers" is a reason.
>> Are the any others?
>> 
>> Is child-del similarly unfinished?  If yes, make it x-child-del to save
>> us from later grief.
>> 
>> If no: is child-del is only useful together with x-child-add?  Then make
>> it x-child-del regardless.
>
> I have another question: if the command is experimental, we have the
> prefix "x-".
> Which prefix is used for hmp command?

HMP is not a stable interface, so generally don't bother marking
experimental interfaces.

That said, I'd probably keep HMP and QMP command name the same to
minimize confusion.

[...]