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

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 8469 bytes --]

On 22.09.2015 09:44, Wen Congyang wrote:
> The new QMP command name is x-blockdev-child-add, and x-blockdev-child-del.
> It justs for adding/removing quorum's child now, and don't support all
> kinds of children,

It does support all kinds of children for quorum, doesn't it?

>                    nor all block drivers. So it is experimental now.

Well, that is not really a reason why we would have to make it
experimental. For instance, blockdev-add (although some might argue it
actually is experimental...) doesn't support all block drivers either.

The reason I am hesitant of adding an experimental QMP interface that is
actually visible to the user (compare x-image in blkverify and blkdebug,
which are not documented and not to be used by the user) is twofold:

(1) At some point we have to say "OK, this is good enough now" and make
    it stable. What would that point be? Who can guarantee that we
    wouldn't want to make any interface changes after that point? Would
    we actually remember to revisit this function once in a while and
    consider making it stable?

(2) While marking things experimental *should* keep people from using it
    in their tools, nobody can guarantee that it *does* keep them from
    doing so. So we may find ourselves in the situation of having to
    keep a compatibility interface for an experimental feature...

For the second point, you should also consider how useful this feature
is to management tools. Just being able to remove and attach children
from a quorum node seems very useful on its own. I don't see why we
should wait for having support for other block drivers; also, for most
block drivers there is no meaningful way of adding or removing children
as nicely as that is possible for quorum.

E.g. you may have a block filter in the future where you want to
exchange its child BDS. This exchange should be an atomic operation, so
we cannot use this interface there anyway. For quorum, such an exchange
does not need to be atomic, since you can just add the new child first
and remove the old one afterwards.

So maybe in the future we get some block driver other than quorum for
which adding and removing children (as opposed to atomically exchanging
them) makes sense, but for now I can only see quorum. Therefore, that
this works for quorum only is in my opinion not a reason to make it
experimental. I think we actually want to keep it that way.

So the question would then be: What ways can you imagine to change this
interface, which would necessitate an incompatible change, therefore
calling for an experimental interface?

(My point is that with such an experimental interface, management tools
cannot use it, even though it'd be a very nice functionality to have)

> 
> 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           | 48 +++++++++++++++++++++++++++++++++++++++++
>  qapi/block-core.json | 34 +++++++++++++++++++++++++++++
>  qmp-commands.hx      | 61 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 143 insertions(+)
> 
> diff --git a/blockdev.c b/blockdev.c
> index 32b04b4..8da0ffb 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -3086,6 +3086,54 @@ fail:
>      qmp_output_visitor_cleanup(ov);
>  }
>  
> +void qmp_x_blockdev_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);
> +    }

You can just pass errp to bdrv_add_child().

> +}
> +
> +void qmp_x_blockdev_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);
> +    }

Same here.

Max

> +}
> +
>  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 bb2189e..9418f05 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -2114,3 +2114,37 @@
>  ##
>  { 'command': 'block-set-write-threshold',
>    'data': { 'node-name': 'str', 'write-threshold': 'uint64' } }
> +
> +##
> +# @x-blockdev-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-blockdev-child-add',
> +  'data' : { 'parent': 'str', 'child': 'str' } }
> +
> +##
> +# @x-blockdev-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': 'x-blockdev-child-del',
> +  'data' : { 'parent': 'str', 'child': 'str' } }
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 66f0300..36e75b1 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -3916,6 +3916,67 @@ Example (2):
>  EQMP
>  
>      {
> +        .name       = "x-blockdev-child-add",
> +        .args_type  = "parent:B,child:B",
> +        .mhandler.cmd_new = qmp_marshal_x_blockdev_child_add,
> +    },
> +
> +SQMP
> +x-blockdev-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
> +
> +Note: this command is experimental, and not a stable API. It doesn't
> +support all kinds of children, nor all block drivers.
> +
> +Example:
> +
> +-> { "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-child-add",
> +    "arguments": { "parent": "disk1", "child": "new_node" } }
> +<- { "return": {} }
> +
> +EQMP
> +
> +    {
> +        .name        = "x-blockdev-child-del",
> +        .args_type   = "parent:B,child:B",
> +        .mhandler.cmd_new = qmp_marshal_x_blockdev_child_del,
> +    },
> +
> +SQMP
> +x-blockdev-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
> +
> +Example:
> +
> +-> { "execute": "x-blockdev-child-del",
> +    "arguments": { "parent": "disk1", "child": "new_node" } }
> +<- { "return": {} }
> +
> +EQMP
> +
> +    {
>          .name       = "query-named-block-nodes",
>          .args_type  = "",
>          .mhandler.cmd_new = qmp_marshal_query_named_block_nodes,
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]