Re: [Qemu-block] [PATCH v9 3/3] qmp: add monitor command to add/remove a child

[Max Reitz]

On 25.12.2015 10:22, Changlong Xie wrote:
> From: Wen Congyang <address@hidden>
> 
> The new QMP command name is x-blockdev-change. It's just for adding/removing
> quorum's child now, and doesn't support all kinds of children, all kinds of
> operations, nor all block drivers. So it is experimental now.
> 
> Signed-off-by: Wen Congyang <address@hidden>
> Signed-off-by: zhanghailiang <address@hidden>
> Signed-off-by: Gonglei <address@hidden>
> Signed-off-by: Changlong Xie <address@hidden>
> ---
>  blockdev.c           | 54 
> ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  qapi/block-core.json | 23 ++++++++++++++++++++++
>  qmp-commands.hx      | 47 +++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 124 insertions(+)
> 
> diff --git a/blockdev.c b/blockdev.c
> index 64dbfeb..4e62fdf 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -3836,6 +3836,60 @@ out:
>      aio_context_release(aio_context);
>  }
>  
> +static BlockDriverState *bdrv_find_child(BlockDriverState *parent_bs,
> +                                         const char *child_name)
> +{
> +    BdrvChild *child;
> +
> +    QLIST_FOREACH(child, &parent_bs->children, next) {
> +        if (strcmp(child->name, child_name) == 0) {
> +            return child->bs;
> +        }
> +    }
> +
> +    return NULL;
> +}
> +
> +void qmp_x_blockdev_change(const char *parent, bool has_child,
> +                           const char *child, bool has_node,
> +                           const char *node, Error **errp)
> +{
> +    BlockDriverState *parent_bs, *child_bs = NULL, *new_bs = NULL;
> +
> +    parent_bs = bdrv_lookup_bs(parent, parent, errp);
> +    if (!parent_bs) {
> +        return;
> +    }
> +
> +    if (has_child == has_node) {
> +        if (has_child) {
> +            error_setg(errp, "The paramter child and node is conflict");

"The parameters child and node are in conflict"

Or, more naturally:

"child and node may not be specified at the same time"

> +        } else {
> +            error_setg(errp, "Either child or node should be specified");

s/should/must/

> +        }
> +        return;
> +    }
> +
> +    if (has_child) {
> +        child_bs = bdrv_find_child(parent_bs, child);
> +        if (!child_bs) {
> +            error_setg(errp, "Node '%s' doesn't have child %s",

s/doesn't/does not/

(This is a personal opinion, but a pretty strong one.)

Also, if you put quotes around the node name, maybe you should do the
same around the child name.

> +                       parent, child);
> +            return;
> +        }
> +        bdrv_del_child(parent_bs, child_bs, errp);
> +    }
> +
> +    if (has_node) {
> +        new_bs = bdrv_find_node(node);
> +        if (!new_bs) {
> +            error_setg(errp, "Node '%s' not found", node);
> +            return;
> +        }
> +        bdrv_add_child(parent_bs, new_bs, errp);
> +    }
> +}
> +
>  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 1a5d9ce..fe63c6d 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -2408,3 +2408,26 @@
>  ##
>  { 'command': 'block-set-write-threshold',
>    'data': { 'node-name': 'str', 'write-threshold': 'uint64' } }
> +
> +##
> +# @x-blockdev-change
> +#
> +# Dynamically reconfigure the block driver state graph. It can be used
> +# to add, remove, insert or replace a block driver state. Currently only

I'd prefer "graph node" over BDS in this line.

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

I'd like a list here what this command does depending on the parameters
given. For instance:

If @(new-)node is specified, it will be inserted under @parent. @child
may not be specified in this case.

If both @parent and @child are specified but @(new-)node is not, @child
will be detached from @parent.

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

I don't know. The parent will actually not be changed, it's an edge that
will be changed; and the parent is the parent node of that edge. Just put

@parent: the id or name of the parent node

here.

> +#
> +# @child: #optional the name of the child that will be deleted.

For now. But maybe that will change in the future. Generally, it is just
the child node of the edge that will be changed. So just putting

@child: #optional the name of a child under the given parent node

(Let's hope this is clear enough that people know that this is not a
node name.)

> +#
> +# @node: #optional the name of the node will be added.

Maybe this should be named new-node instead.

Also: "...of the node that will be added."

> +#
> +# Note: this command is experimental, and its API is not stable.
> +#
> +# Since: 2.6
> +##
> +{ 'command': 'x-blockdev-change',
> +  'data' : { 'parent': 'str',
> +             '*child': 'str',
> +             '*node': 'str' } }
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 7b235ee..efee0ca 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -4293,6 +4293,53 @@ Example:
>  EQMP
>  
>      {
> +        .name       = "x-blockdev-change",
> +        .args_type  = "parent:B,child:B?,node:B?",
> +        .mhandler.cmd_new = qmp_marshal_x_blockdev_change,
> +    },
> +
> +SQMP
> +x-blockdev-change
> +-----------------
> +
> +Dynamically reconfigure the block driver state graph. It can be used to
> +add, remove, insert, or 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.
> +
> +Arguments:
> +- "parent": the id or node name of which node will be changed (json-string)
> +- "child": the child name which will be deleted (json-string, optional)
> +- "node": the new node-name which will be added (json-string, optional)
> +
> +Note: this command is experimental, and not a stable API. It doesn't
> +support all kinds of operations, all kinds of children, nor all block
> +drivers.

I think this second sentence should be present in qapi/block-core.json
as well.

Max

> +
> +Example:
> +
> +Add a new node to a quorum
> +-> { "execute": "blockdev-add",
> +    "arguments": { "options": { "driver": "raw",
> +                                "node-name": "new_node",
> +                                "id": "test_new_node",
> +                                "file": { "driver": "file",
> +                                          "filename": "test.raw" } } } }
> +<- { "return": {} }
> +-> { "execute": "x-blockdev-change",
> +    "arguments": { "parent": "disk1",
> +                   "node": "new_node" } }
> +<- { "return": {} }
> +
> +Delete a quorum's node
> +-> { "execute": "x-blockdev-change",
> +    "arguments": { "parent": "disk1",
> +                   "child": "children.1" } }
> +<- { "return": {} }
> +
> +EQMP
> +
> +    {
>          .name       = "query-named-block-nodes",
>          .args_type  = "",
>          .mhandler.cmd_new = qmp_marshal_query_named_block_nodes,
>

signature.asc
Description: OpenPGP digital signature