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

[Wen Congyang]

On 09/14/2015 10:36 PM, Markus Armbruster wrote:
> Wen Congyang <address@hidden> writes:
> 
>> Signed-off-by: Wen Congyang <address@hidden>
>> Signed-off-by: zhanghailiang <address@hidden>
>> Signed-off-by: Gonglei <address@hidden>
>> ---
>>  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,
> .
>