qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Markus Armbruster
Subject: Re: [Qemu-devel] [PATCH v3 4/5] qmp: add monitor command to add/remove a child
Date: Tue, 15 Sep 2015 09:49:07 +0200
User-agent: Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux)

Wen Congyang <address@hidden> writes:

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

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



reply via email to

[Prev in Thread] Current Thread [Next in Thread]