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

[Wen Congyang <wency@cn.fujitsu.com>]

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