[Qemu-devel] [PATCH v6 0/4] qapi: child add/delete support

[Qemu-devel] [PATCH v6 0/4] qapi: child add/delete support

[Wen Congyang]

If quorum's child is broken, we can use mirror job to replace it.
But sometimes, the user only need to remove the broken child, and
add it later when the problem is fixed.

It is based on the Kevin's bdrv_swap() related patch:
http://lists.nongnu.org/archive/html/qemu-devel/2015-10/msg02152.html

ChangLog:
v6:
1. Use a single qmp command x-blockdev-change to replace x-blockdev-child-add
   and x-blockdev-child-delete
v5:
1. Address Eric Blake's comments
v4:
1. drop nbd driver's implementation. We can use human-monitor-command
   to do it.
2. Rename the command name.
v3:
1. Don't open BDS in bdrv_add_child(). Use the existing BDS which is
   created by the QMP command blockdev-add.
2. The driver NBD can support filename, path, host:port now.
v2:
1. Use bdrv_get_device_or_node_name() instead of new function
   bdrv_get_id_or_node_name()
2. Update the error message
3. Update the documents in block-core.json


Wen Congyang (4):
  Add new block driver interface to add/delete a BDS's child
  quorum: implement bdrv_add_child() and bdrv_del_child()
  qmp: add monitor command to add/remove a child
  hmp: add monitor command to add/remove a child

 block.c                   | 56 ++++++++++++++++++++++++++++++++--
 block/quorum.c            | 59 ++++++++++++++++++++++++++++++++++--
 blockdev.c                | 76 +++++++++++++++++++++++++++++++++++++++++++++++
 hmp-commands.hx           | 17 +++++++++++
 hmp.c                     | 38 ++++++++++++++++++++++++
 hmp.h                     |  1 +
 include/block/block.h     |  8 +++++
 include/block/block_int.h |  5 ++++
 qapi/block-core.json      | 40 +++++++++++++++++++++++++
 qmp-commands.hx           | 50 +++++++++++++++++++++++++++++++
 10 files changed, 345 insertions(+), 5 deletions(-)

-- 
2.4.3

[Qemu-devel] [PATCH v6 1/4] Add new block driver interface to add/delete a BDS's child

[Wen Congyang]

In some cases, we want to take a quorum child offline, and take
another child online.

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>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 block.c                   | 50 +++++++++++++++++++++++++++++++++++++++++++++++
 include/block/block.h     |  5 +++++
 include/block/block_int.h |  5 +++++
 3 files changed, 60 insertions(+)

diff --git a/block.c b/block.c
index 62848ca..bcba22f 100644
--- a/block.c
+++ b/block.c
@@ -4183,3 +4183,53 @@ BlockAcctStats *bdrv_get_stats(BlockDriverState *bs)
 {
     return &bs->stats;
 }
+
+/*
+ * Hot add/remove a BDS's child. So the user can take a child offline when
+ * it is broken and take a new child online
+ */
+void bdrv_add_child(BlockDriverState *parent_bs, BlockDriverState *child_bs,
+                    Error **errp)
+{
+
+    if (!parent_bs->drv || !parent_bs->drv->bdrv_add_child) {
+        error_setg(errp, "The node %s doesn't support adding a child",
+                   bdrv_get_device_or_node_name(parent_bs));
+        return;
+    }
+
+    if (!QLIST_EMPTY(&child_bs->parents)) {
+        error_setg(errp, "The node %s already has a parent",
+                   child_bs->node_name);
+        return;
+    }
+
+    parent_bs->drv->bdrv_add_child(parent_bs, child_bs, errp);
+}
+
+void bdrv_del_child(BlockDriverState *parent_bs, BlockDriverState *child_bs,
+                    Error **errp)
+{
+    BdrvChild *child;
+
+    if (!parent_bs->drv || !parent_bs->drv->bdrv_del_child) {
+        error_setg(errp, "The node %s doesn't support removing a child",
+                   bdrv_get_device_or_node_name(parent_bs));
+        return;
+    }
+
+    QLIST_FOREACH(child, &parent_bs->children, next) {
+        if (child->bs == child_bs) {
+            break;
+        }
+    }
+
+    if (!child) {
+        error_setg(errp, "The node %s is not a child of %s",
+                   bdrv_get_device_or_node_name(child_bs),
+                   bdrv_get_device_or_node_name(parent_bs));
+        return;
+    }
+
+    parent_bs->drv->bdrv_del_child(parent_bs, child_bs, errp);
+}
diff --git a/include/block/block.h b/include/block/block.h
index 1520dee..ef84c87 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -624,4 +624,9 @@ void bdrv_flush_io_queue(BlockDriverState *bs);
 
 BlockAcctStats *bdrv_get_stats(BlockDriverState *bs);
 
+void bdrv_add_child(BlockDriverState *parent, BlockDriverState *child,
+                    Error **errp);
+void bdrv_del_child(BlockDriverState *parent, BlockDriverState *child,
+                    Error **errp);
+
 #endif
diff --git a/include/block/block_int.h b/include/block/block_int.h
index c0e6513..cd66223 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -287,6 +287,11 @@ struct BlockDriver {
      */
     int (*bdrv_probe_geometry)(BlockDriverState *bs, HDGeometry *geo);
 
+    void (*bdrv_add_child)(BlockDriverState *parent, BlockDriverState *child,
+                           Error **errp);
+    void (*bdrv_del_child)(BlockDriverState *parent, BlockDriverState *child,
+                           Error **errp);
+
     QLIST_ENTRY(BlockDriver) list;
 };
 
-- 
2.4.3

Re: [Qemu-devel] [PATCH v6 1/4] Add new block driver interface to add/delete a BDS's child

[Alberto Garcia]

On Fri 16 Oct 2015 10:57:43 AM CEST, Wen Congyang <wency@cn.fujitsu.com> wrote:
> In some cases, we want to take a quorum child offline, and take
> another child online.
>
> 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>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---

Reviewed-by: Alberto Garcia <berto@igalia.com>

Berto

[Qemu-devel] [PATCH v6 2/4] quorum: implement bdrv_add_child() and bdrv_del_child()

[Wen Congyang]

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>
---
 block.c               |  6 +++---
 block/quorum.c        | 59 +++++++++++++++++++++++++++++++++++++++++++++++++--
 include/block/block.h |  3 +++
 3 files changed, 63 insertions(+), 5 deletions(-)

diff --git a/block.c b/block.c
index bcba22f..d96d2cc 100644
--- a/block.c
+++ b/block.c
@@ -1079,9 +1079,9 @@ static int bdrv_fill_options(QDict **options, const char **pfilename,
     return 0;
 }
 
-static BdrvChild *bdrv_attach_child(BlockDriverState *parent_bs,
-                                    BlockDriverState *child_bs,
-                                    const BdrvChildRole *child_role)
+BdrvChild *bdrv_attach_child(BlockDriverState *parent_bs,
+                             BlockDriverState *child_bs,
+                             const BdrvChildRole *child_role)
 {
     BdrvChild *child = g_new(BdrvChild, 1);
     *child = (BdrvChild) {
diff --git a/block/quorum.c b/block/quorum.c
index c4cda32..a9e499c 100644
--- a/block/quorum.c
+++ b/block/quorum.c
@@ -875,9 +875,9 @@ static int quorum_open(BlockDriverState *bs, QDict *options, int flags,
         ret = -EINVAL;
         goto exit;
     }
-    if (s->num_children < 2) {
+    if (s->num_children < 1) {
         error_setg(&local_err,
-                   "Number of provided children must be greater than 1");
+                   "Number of provided children must be 1 or more");
         ret = -EINVAL;
         goto exit;
     }
@@ -997,6 +997,58 @@ static void quorum_attach_aio_context(BlockDriverState *bs,
     }
 }
 
+static void quorum_add_child(BlockDriverState *bs, BlockDriverState *child_bs,
+                             Error **errp)
+{
+    BDRVQuorumState *s = bs->opaque;
+    BdrvChild *child;
+
+    bdrv_drain(bs);
+
+    assert(s->num_children <= INT_MAX / sizeof(BdrvChild *));
+    if (s->num_children == INT_MAX / sizeof(BdrvChild *)) {
+        error_setg(errp, "Too many children");
+        return;
+    }
+    s->children = g_renew(BdrvChild *, s->children, s->num_children + 1);
+
+    bdrv_ref(child_bs);
+    child = bdrv_attach_child(bs, child_bs, &child_format);
+    s->children[s->num_children++] = child;
+}
+
+static void quorum_del_child(BlockDriverState *bs, BlockDriverState *child_bs,
+                             Error **errp)
+{
+    BDRVQuorumState *s = bs->opaque;
+    BdrvChild *child;
+    int i;
+
+    for (i = 0; i < s->num_children; i++) {
+        if (s->children[i]->bs == child_bs) {
+            break;
+        }
+    }
+
+    /* we have checked it in bdrv_del_child() */
+    assert(i < s->num_children);
+    child = s->children[i];
+
+    if (s->num_children <= s->threshold) {
+        error_setg(errp,
+            "The number of children cannot be lower than the vote threshold %d",
+            s->threshold);
+        return;
+    }
+
+    bdrv_drain(bs);
+    /* We can safely remove this child now */
+    memmove(&s->children[i], &s->children[i + 1],
+            (s->num_children - i - 1) * sizeof(void *));
+    s->children = g_renew(BdrvChild *, s->children, --s->num_children);
+    bdrv_unref_child(bs, child);
+}
+
 static void quorum_refresh_filename(BlockDriverState *bs)
 {
     BDRVQuorumState *s = bs->opaque;
@@ -1052,6 +1104,9 @@ static BlockDriver bdrv_quorum = {
     .bdrv_detach_aio_context            = quorum_detach_aio_context,
     .bdrv_attach_aio_context            = quorum_attach_aio_context,
 
+    .bdrv_add_child                     = quorum_add_child,
+    .bdrv_del_child                     = quorum_del_child,
+
     .is_filter                          = true,
     .bdrv_recurse_is_first_non_filter   = quorum_recurse_is_first_non_filter,
 };
diff --git a/include/block/block.h b/include/block/block.h
index ef84c87..f5bfb6b 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -516,6 +516,9 @@ void bdrv_disable_copy_on_read(BlockDriverState *bs);
 void bdrv_ref(BlockDriverState *bs);
 void bdrv_unref(BlockDriverState *bs);
 void bdrv_unref_child(BlockDriverState *parent, BdrvChild *child);
+BdrvChild *bdrv_attach_child(BlockDriverState *parent_bs,
+                             BlockDriverState *child_bs,
+                             const BdrvChildRole *child_role);
 
 bool bdrv_op_is_blocked(BlockDriverState *bs, BlockOpType op, Error **errp);
 void bdrv_op_block(BlockDriverState *bs, BlockOpType op, Error *reason);
-- 
2.4.3

Re: [Qemu-devel] [PATCH v6 2/4] quorum: implement bdrv_add_child() and bdrv_del_child()

[Alberto Garcia]

On Fri 16 Oct 2015 10:57:44 AM CEST, Wen Congyang wrote:
> +static void quorum_add_child(BlockDriverState *bs, BlockDriverState *child_bs,
> +                             Error **errp)
> +{
> +    BDRVQuorumState *s = bs->opaque;
> +    BdrvChild *child;
> +
> +    bdrv_drain(bs);
> +
> +    assert(s->num_children <= INT_MAX / sizeof(BdrvChild *));
> +    if (s->num_children == INT_MAX / sizeof(BdrvChild *)) {
> +        error_setg(errp, "Too many children");
> +        return;
> +    }

This limit guarantees that s->num_children <= INT_MAX and that
s->num_children * sizeof(BdrvChild *) <= SIZE_MAX, right?

> +    s->children = g_renew(BdrvChild *, s->children, s->num_children + 1);
> +
> +    bdrv_ref(child_bs);
> +    child = bdrv_attach_child(bs, child_bs, &child_format);
> +    s->children[s->num_children++] = child;
> +}

Maybe you want to use ++s->num_children in the g_renew() call to make it
symmetric to the one in quorum_del_child()...

> +    /* We can safely remove this child now */
> +    memmove(&s->children[i], &s->children[i + 1],
> +            (s->num_children - i - 1) * sizeof(void *));
> +    s->children = g_renew(BdrvChild *, s->children, --s->num_children);
> +    bdrv_unref_child(bs, child);
> +}

But it's not really important, so you can leave it as it is now if you
prefer.

Reviewed-by: Alberto Garcia <berto@igalia.com>

Berto

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

[Wen Congyang]

The new QMP command name is x-blockdev-change. It justs for adding/removing
quorum's child now, and don't support all kinds of children, all kinds of
operations, nor all block drivers. So it is experimental now.

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           | 76 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 qapi/block-core.json | 40 +++++++++++++++++++++++++++
 qmp-commands.hx      | 50 ++++++++++++++++++++++++++++++++++
 3 files changed, 166 insertions(+)

diff --git a/blockdev.c b/blockdev.c
index 6c8cce4..72efe5d 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3086,6 +3086,82 @@ fail:
     qmp_output_visitor_cleanup(ov);
 }
 
+void qmp_x_blockdev_change(ChangeOperation op, const char *parent,
+                           bool has_child, const char *child,
+                           bool has_new_node, const char *new_node,
+                           Error **errp)
+{
+    BlockDriverState *parent_bs, *child_bs, *new_bs;
+    Error *local_err = NULL;
+
+    parent_bs = bdrv_lookup_bs(parent, parent, &local_err);
+    if (!parent_bs) {
+        error_propagate(errp, local_err);
+        return;
+    }
+
+    switch(op) {
+    case CHANGE_OPERATION_ADD:
+        if (has_child) {
+            error_setg(errp, "The operation %s doesn't support the parameter child",
+                       ChangeOperation_lookup[op]);
+            return;
+        }
+        if (!has_new_node) {
+            error_setg(errp, "The operation %s needs the parameter new_node",
+                       ChangeOperation_lookup[op]);
+            return;
+        }
+        break;
+    case CHANGE_OPERATION_DELETE:
+        if (has_new_node) {
+            error_setg(errp, "The operation %s doesn't support the parameter node",
+                       ChangeOperation_lookup[op]);
+            return;
+        }
+        if (!has_child) {
+            error_setg(errp, "The operation %s needs the parameter child",
+                       ChangeOperation_lookup[op]);
+            return;
+        }
+    default:
+        break;
+    }
+
+    if (has_child) {
+        child_bs = bdrv_find_node(child);
+        if (!child_bs) {
+            error_setg(errp, "Node '%s' not found", child);
+            return;
+        }
+    }
+
+    if (has_new_node) {
+        new_bs = bdrv_find_node(new_node);
+        if (!new_bs) {
+            error_setg(errp, "Node '%s' not found", new_node);
+            return;
+        }
+    }
+
+    switch(op) {
+    case CHANGE_OPERATION_ADD:
+        bdrv_add_child(parent_bs, new_bs, &local_err);
+        if (local_err) {
+            error_propagate(errp, local_err);
+        }
+        break;
+    case CHANGE_OPERATION_DELETE:
+        bdrv_del_child(parent_bs, child_bs, &local_err);
+        if (local_err) {
+            error_propagate(errp, local_err);
+        }
+        break;
+    default:
+        break;
+    }
+}
+
 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..361588f 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2114,3 +2114,43 @@
 ##
 { 'command': 'block-set-write-threshold',
   'data': { 'node-name': 'str', 'write-threshold': 'uint64' } }
+
+##
+# @ChangeOperation:
+#
+# An enumeration of block device change operation.
+#
+# @add: Add a new block driver state to a existed block driver state.
+#
+# @delete: Delete a block driver state's child.
+#
+# Since: 2.5
+##
+{ 'enum': 'ChangeOperation',
+  'data': [ 'add', 'delete' ] }
+
+##
+# @x-blockdev-change
+#
+# Dynamic reconfigure the block driver state graph. It can be used to
+# add, remove, insert, 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.
+#
+# @operation: the chanage operation. It can be add, delete.
+#
+# @parent: the id or node name of which node will be changed.
+#
+# @child: the child node-name which will be deleted.
+#
+# @node: the new node-name which will be added.
+#
+# Note: this command is experimental, and not a stable API.
+#
+# Since: 2.5
+##
+{ 'command': 'x-blockdev-change',
+  'data' : { 'operation': 'ChangeOperation',
+             'parent': 'str',
+             '*child': 'str',
+             '*node': 'str' } }
diff --git a/qmp-commands.hx b/qmp-commands.hx
index d2ba800..ede7b71 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -3921,6 +3921,56 @@ Example (2):
 EQMP
 
     {
+        .name       = "x-blockdev-change",
+        .args_type  = "operation:s,parent:B,child:B?,node:B?",
+        .mhandler.cmd_new = qmp_marshal_x_blockdev_change,
+    },
+
+SQMP
+x-blockdev-change
+------------
+
+Dynamic reconfigure the block driver state graph. It can be used to
+add, remove, insert, 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:
+- "operation": the chanage operation. It can be add, delete.
+- "parent": the id or node name of which node will be changed
+- "child": the child node-name which will be delete
+- "node": the new node-name which will be added
+
+Note: this command is experimental, and not a stable API. It doesn't
+support all kinds of operations, all kindes of children, nor all block
+drivers.
+
+Example:
+
+Add a new quorum's node
+-> { "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": { "operation": "add",
+                   "parent": "disk1",
+                   "node": "new_node" } }
+<- { "return": {} }
+
+Delete a quorum's node
+-> { "execute": "x-blockdev-change",
+    "arguments": { "operation": "delete",
+                   "parent": "disk1",
+                   "child": "new_node" } }
+<- { "return": {} }
+
+EQMP
+
+    {
         .name       = "query-named-block-nodes",
         .args_type  = "",
         .mhandler.cmd_new = qmp_marshal_query_named_block_nodes,
-- 
2.4.3

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

[Alberto Garcia]

On Fri 16 Oct 2015 10:57:45 AM CEST, Wen Congyang <wency@cn.fujitsu.com> wrote:

> The new QMP command name is x-blockdev-change. It justs for
> adding/removing quorum's child now, and don't support all kinds of
> children, all kinds of operations, nor all block drivers. So it is
> experimental now.

I might have missed some discussion, why were the -add and -delete 

> +# @x-blockdev-change
> +#
> +# Dynamic reconfigure the block driver state graph. It can be used to
> +# add, remove, insert, 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.
> +#
> +# @operation: the chanage operation. It can be add, delete.
> +#
> +# @parent: the id or node name of which node will be changed.
> +#
> +# @child: the child node-name which will be deleted.
> +#
> +# @node: the new node-name which will be added.
> +#
> +# Note: this command is experimental, and not a stable API.
> +#
> +# Since: 2.5
> +##
> +{ 'command': 'x-blockdev-change',
> +  'data' : { 'operation': 'ChangeOperation',
> +             'parent': 'str',
> +             '*child': 'str',
> +             '*node': 'str' } }

Do you really need two separate 'child' and 'node' parameters? If the
operation is 'add' you can only use 'node', if it is 'delete, you can
only use 'child'. It seems to me that you can simply have one 'node'
parameter and use it for both ...

Berto

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

[Wen Congyang]

On 11/05/2015 09:49 PM, Alberto Garcia wrote:
> On Fri 16 Oct 2015 10:57:45 AM CEST, Wen Congyang <wency@cn.fujitsu.com> wrote:
> 
>> The new QMP command name is x-blockdev-change. It justs for
>> adding/removing quorum's child now, and don't support all kinds of
>> children, all kinds of operations, nor all block drivers. So it is
>> experimental now.
> 
> I might have missed some discussion, why were the -add and -delete 

This monitor command can be used to implement: add, delete, insert, remove,
replace... Currently, I only implement add and delete operation.

> 
>> +# @x-blockdev-change
>> +#
>> +# Dynamic reconfigure the block driver state graph. It can be used to
>> +# add, remove, insert, 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.
>> +#
>> +# @operation: the chanage operation. It can be add, delete.
>> +#
>> +# @parent: the id or node name of which node will be changed.
>> +#
>> +# @child: the child node-name which will be deleted.
>> +#
>> +# @node: the new node-name which will be added.
>> +#
>> +# Note: this command is experimental, and not a stable API.
>> +#
>> +# Since: 2.5
>> +##
>> +{ 'command': 'x-blockdev-change',
>> +  'data' : { 'operation': 'ChangeOperation',
>> +             'parent': 'str',
>> +             '*child': 'str',
>> +             '*node': 'str' } }
> 
> Do you really need two separate 'child' and 'node' parameters? If the
> operation is 'add' you can only use 'node', if it is 'delete, you can
> only use 'child'. It seems to me that you can simply have one 'node'
> parameter and use it for both ...

parent and child already exist in the BDS graph, and node is a new node.
In the furture, we may need to implement insert opetioan, and this operation
needs such three BDSes.

Thanks
Wen Congyang

> 
> Berto
> .
> 

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

[Alberto Garcia]

Sorry again for the late review, here are my comments:

On Fri 16 Oct 2015 10:57:45 AM CEST, Wen Congyang wrote:
> +void qmp_x_blockdev_change(ChangeOperation op, const char *parent,
> +                           bool has_child, const char *child,
> +                           bool has_new_node, const char *new_node,
> +                           Error **errp)

You are using different names for the parameters here: 'op', 'parent',
'child', 'new_node'; in the JSON file the first and last one are named
'operation' and 'node'.

> +    parent_bs = bdrv_lookup_bs(parent, parent, &local_err);
> +    if (!parent_bs) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }

You don't need to change it if you don't want but you can use errp
directly here and spare the error_propagate() call.

> +
> +    switch(op) {
> +    case CHANGE_OPERATION_ADD:
> +        if (has_child) {
> +            error_setg(errp, "The operation %s doesn't support the parameter child",
> +                       ChangeOperation_lookup[op]);
> +            return;
> +        }

This line goes over 80 columns, please use scripts/checkpatch.pl to
check the style of the code.

> +        if (!has_new_node) {
> +            error_setg(errp, "The operation %s needs the parameter new_node",
> +                       ChangeOperation_lookup[op]);
> +            return;
> +        }
> +        break;
> +    case CHANGE_OPERATION_DELETE:
> +        if (has_new_node) {
> +            error_setg(errp, "The operation %s doesn't support the parameter node",
> +                       ChangeOperation_lookup[op]);
> +            return;
> +        }
> +        if (!has_child) {
> +            error_setg(errp, "The operation %s needs the parameter child",
> +                       ChangeOperation_lookup[op]);
> +            return;
> +        }

I still think that having two optional parameters here makes things
unnecessarily complex.

If in the future we want to add a new operation that needs a new
parameter then we can add it then, but I think that both 'add' and
'delete' can work perfectly fine with a single 'node' parameter.

Does anyone else have an opinion about this?

> +    default:
> +        break;
> +    }

This is unreachable so you can add a g_assert_not_reached() here.

> +##
> +# @ChangeOperation:
> +#
> +# An enumeration of block device change operation.

How about something like "An enumeration of possible change operations
on a block device" ?

> +# @add: Add a new block driver state to a existed block driver state.

s/a existed/an existing/

> +# @x-blockdev-change
> +#
> +# Dynamic reconfigure the block driver state graph. It can be used to

"Dynamically reconfigure"

> +# add, remove, insert, replace a block driver state. Currently only

"insert or replace"

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

"add and remove its child" -> "add or remove a child"

> +#
> +# @operation: the chanage operation. It can be add, delete.

s/chanage/change/

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

How about "the id or name of the node that will be changed" ?

> +#
> +# @child: the child node-name which will be deleted.
> +#
> +# @node: the new node-name which will be added.

"The name of the node that will be deleted"
"The name of the node that will be added"

Or, if you merge both parameters, "...that will be added or deleted".

> +#
> +# Note: this command is experimental, and not a stable API.

"and not a stable API" -> "and does not have a stable API", or "and its
API is not stable".

Berto

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

[Wen Congyang]

On 11/09/2015 10:42 PM, Alberto Garcia wrote:
> Sorry again for the late review, here are my comments:
> 
> On Fri 16 Oct 2015 10:57:45 AM CEST, Wen Congyang wrote:
>> +void qmp_x_blockdev_change(ChangeOperation op, const char *parent,
>> +                           bool has_child, const char *child,
>> +                           bool has_new_node, const char *new_node,
>> +                           Error **errp)
> 
> You are using different names for the parameters here: 'op', 'parent',
> 'child', 'new_node'; in the JSON file the first and last one are named
> 'operation' and 'node'.

OK, I will fix it in the next version

> 
>> +    parent_bs = bdrv_lookup_bs(parent, parent, &local_err);
>> +    if (!parent_bs) {
>> +        error_propagate(errp, local_err);
>> +        return;
>> +    }
> 
> You don't need to change it if you don't want but you can use errp
> directly here and spare the error_propagate() call.

Too many codes in qemu use local_err and error_propagate(). I think
errp can be NOT NULL here(in which case?).

> 
>> +
>> +    switch(op) {
>> +    case CHANGE_OPERATION_ADD:
>> +        if (has_child) {
>> +            error_setg(errp, "The operation %s doesn't support the parameter child",
>> +                       ChangeOperation_lookup[op]);
>> +            return;
>> +        }
> 
> This line goes over 80 columns, please use scripts/checkpatch.pl to
> check the style of the code.

I forgot to do it...

> 
>> +        if (!has_new_node) {
>> +            error_setg(errp, "The operation %s needs the parameter new_node",
>> +                       ChangeOperation_lookup[op]);
>> +            return;
>> +        }
>> +        break;
>> +    case CHANGE_OPERATION_DELETE:
>> +        if (has_new_node) {
>> +            error_setg(errp, "The operation %s doesn't support the parameter node",
>> +                       ChangeOperation_lookup[op]);
>> +            return;
>> +        }
>> +        if (!has_child) {
>> +            error_setg(errp, "The operation %s needs the parameter child",
>> +                       ChangeOperation_lookup[op]);
>> +            return;
>> +        }
> 
> I still think that having two optional parameters here makes things
> unnecessarily complex.
> 
> If in the future we want to add a new operation that needs a new
> parameter then we can add it then, but I think that both 'add' and
> 'delete' can work perfectly fine with a single 'node' parameter.
> 
> Does anyone else have an opinion about this?
> 
>> +    default:
>> +        break;
>> +    }
> 
> This is unreachable so you can add a g_assert_not_reached() here.

OK

> 
>> +##
>> +# @ChangeOperation:
>> +#
>> +# An enumeration of block device change operation.
> 
> How about something like "An enumeration of possible change operations
> on a block device" ?
> 
>> +# @add: Add a new block driver state to a existed block driver state.
> 
> s/a existed/an existing/
> 
>> +# @x-blockdev-change
>> +#
>> +# Dynamic reconfigure the block driver state graph. It can be used to
> 
> "Dynamically reconfigure"
> 
>> +# add, remove, insert, replace a block driver state. Currently only
> 
> "insert or replace"
> 
>> +# the Quorum driver implements this feature to add and remove its child.
>> +# This is useful to fix a broken quorum child.
> 
> "add and remove its child" -> "add or remove a child"
> 
>> +#
>> +# @operation: the chanage operation. It can be add, delete.
> 
> s/chanage/change/
> 
>> +#
>> +# @parent: the id or node name of which node will be changed.
> 
> How about "the id or name of the node that will be changed" ?
> 
>> +#
>> +# @child: the child node-name which will be deleted.
>> +#
>> +# @node: the new node-name which will be added.
> 
> "The name of the node that will be deleted"
> "The name of the node that will be added"
> 
> Or, if you merge both parameters, "...that will be added or deleted".
> 
>> +#
>> +# Note: this command is experimental, and not a stable API.
> 
> "and not a stable API" -> "and does not have a stable API", or "and its
> API is not stable".

Thanks for your review, all will be fixed in the next version

Wen Congyang

> 
> Berto
> .
> 

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

[Markus Armbruster]

Wen Congyang <wency@cn.fujitsu.com> writes:

> On 11/09/2015 10:42 PM, Alberto Garcia wrote:
>> Sorry again for the late review, here are my comments:
>> 
>> On Fri 16 Oct 2015 10:57:45 AM CEST, Wen Congyang wrote:
>>> +void qmp_x_blockdev_change(ChangeOperation op, const char *parent,
>>> +                           bool has_child, const char *child,
>>> +                           bool has_new_node, const char *new_node,
>>> +                           Error **errp)
>> 
>> You are using different names for the parameters here: 'op', 'parent',
>> 'child', 'new_node'; in the JSON file the first and last one are named
>> 'operation' and 'node'.
>
> OK, I will fix it in the next version
>
>> 
>>> +    parent_bs = bdrv_lookup_bs(parent, parent, &local_err);
>>> +    if (!parent_bs) {
>>> +        error_propagate(errp, local_err);
>>> +        return;
>>> +    }
>> 
>> You don't need to change it if you don't want but you can use errp
>> directly here and spare the error_propagate() call.
>
> Too many codes in qemu use local_err and error_propagate(). I think
> errp can be NOT NULL here(in which case?).

It's usually advisable not to rely on "all callers pass non-null value
to parameter errp" arguments, because they're non-local and tend to be
brittle.

error.h attempts to provide guidance:

 * Receive an error and pass it on to the caller:
 *     Error *err = NULL;
 *     foo(arg, &err);
 *     if (err) {
 *         handle the error...
 *         error_propagate(errp, err);
 *     }
 * where Error **errp is a parameter, by convention the last one.
 *
 * Do *not* "optimize" this to
 *     foo(arg, errp);
 *     if (*errp) { // WRONG!
 *         handle the error...
 *     }
 * because errp may be NULL!
 *
 * But when all you do with the error is pass it on, please use
 *     foo(arg, errp);
 * for readability.

Since all you do with local_err in the quoted code snippet is pass it
on, the last paragraph applies, and you can simplify to:

    parent_bs = bdrv_lookup_bs(parent, parent, errp);
    if (!parent_bs) {
        return;
    }

Whether errp can be null doesn't matter.

[...]

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

[Kevin Wolf]

Am 16.10.2015 um 10:57 hat Wen Congyang geschrieben:
> +##
> +# @ChangeOperation:
> +#
> +# An enumeration of block device change operation.
> +#
> +# @add: Add a new block driver state to a existed block driver state.
> +#
> +# @delete: Delete a block driver state's child.
> +#
> +# Since: 2.5
> +##
> +{ 'enum': 'ChangeOperation',
> +  'data': [ 'add', 'delete' ] }

What's the advantage of this enum compared to separate QMP commands? The
way it is specified here, ChangeOperation is already implicit by whether
or not child and node are given.

> +##
> +# @x-blockdev-change
> +#
> +# Dynamic reconfigure the block driver state graph. It can be used to
> +# add, remove, insert, 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.
> +#
> +# @operation: the chanage operation. It can be add, delete.
> +#
> +# @parent: the id or node name of which node will be changed.
> +#
> +# @child: the child node-name which will be deleted.

#optional

Must be present for operation = delete, must not be present otherwise.

> +# @node: the new node-name which will be added.

#optional

Must be present for operation = add, must not be present otherwise.

> +#
> +# Note: this command is experimental, and not a stable API.
> +#
> +# Since: 2.5
> +##
> +{ 'command': 'x-blockdev-change',
> +  'data' : { 'operation': 'ChangeOperation',
> +             'parent': 'str',
> +             '*child': 'str',
> +             '*node': 'str' } }

Let me suggest this alternative:

{ 'command': 'x-blockdev-change',
  'data' : { 'parent': 'str',
             'child': 'str',
             '*node': 'str' } }

child doesn't describe a node name then, but a child name (adds a
dependency on my patches which add a name to BdrvChild, though).
Depending on whether node is given and whether the child already exists,
this may add, remove or replace a child.

Kevin

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

[Wen Congyang]

On 11/10/2015 12:04 AM, Kevin Wolf wrote:
> Am 16.10.2015 um 10:57 hat Wen Congyang geschrieben:
>> +##
>> +# @ChangeOperation:
>> +#
>> +# An enumeration of block device change operation.
>> +#
>> +# @add: Add a new block driver state to a existed block driver state.
>> +#
>> +# @delete: Delete a block driver state's child.
>> +#
>> +# Since: 2.5
>> +##
>> +{ 'enum': 'ChangeOperation',
>> +  'data': [ 'add', 'delete' ] }
> 
> What's the advantage of this enum compared to separate QMP commands? The
> way it is specified here, ChangeOperation is already implicit by whether
> or not child and node are given.
> 
>> +##
>> +# @x-blockdev-change
>> +#
>> +# Dynamic reconfigure the block driver state graph. It can be used to
>> +# add, remove, insert, 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.
>> +#
>> +# @operation: the chanage operation. It can be add, delete.
>> +#
>> +# @parent: the id or node name of which node will be changed.
>> +#
>> +# @child: the child node-name which will be deleted.
> 
> #optional
> 
> Must be present for operation = delete, must not be present otherwise.
> 
>> +# @node: the new node-name which will be added.
> 
> #optional
> 
> Must be present for operation = add, must not be present otherwise.
> 
>> +#
>> +# Note: this command is experimental, and not a stable API.
>> +#
>> +# Since: 2.5
>> +##
>> +{ 'command': 'x-blockdev-change',
>> +  'data' : { 'operation': 'ChangeOperation',
>> +             'parent': 'str',
>> +             '*child': 'str',
>> +             '*node': 'str' } }
> 
> Let me suggest this alternative:
> 
> { 'command': 'x-blockdev-change',
>   'data' : { 'parent': 'str',
>              'child': 'str',
>              '*node': 'str' } }
> 
> child doesn't describe a node name then, but a child name (adds a
> dependency on my patches which add a name to BdrvChild, though).

Where is the patch? I don't find it.

> Depending on whether node is given and whether the child already exists,
> this may add, remove or replace a child.

If the user wants to insert a filter driver between parent and child, we
also needs three parameters: parent, child, node. So it is why I add the
parameter operation.

Thanks
Wen Congyang

> 
> Kevin
> .
> 

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

[Wen Congyang]

On 11/10/2015 09:40 AM, Wen Congyang wrote:
> On 11/10/2015 12:04 AM, Kevin Wolf wrote:
>> Am 16.10.2015 um 10:57 hat Wen Congyang geschrieben:
>>> +##
>>> +# @ChangeOperation:
>>> +#
>>> +# An enumeration of block device change operation.
>>> +#
>>> +# @add: Add a new block driver state to a existed block driver state.
>>> +#
>>> +# @delete: Delete a block driver state's child.
>>> +#
>>> +# Since: 2.5
>>> +##
>>> +{ 'enum': 'ChangeOperation',
>>> +  'data': [ 'add', 'delete' ] }
>>
>> What's the advantage of this enum compared to separate QMP commands? The
>> way it is specified here, ChangeOperation is already implicit by whether
>> or not child and node are given.
>>
>>> +##
>>> +# @x-blockdev-change
>>> +#
>>> +# Dynamic reconfigure the block driver state graph. It can be used to
>>> +# add, remove, insert, 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.
>>> +#
>>> +# @operation: the chanage operation. It can be add, delete.
>>> +#
>>> +# @parent: the id or node name of which node will be changed.
>>> +#
>>> +# @child: the child node-name which will be deleted.
>>
>> #optional
>>
>> Must be present for operation = delete, must not be present otherwise.
>>
>>> +# @node: the new node-name which will be added.
>>
>> #optional
>>
>> Must be present for operation = add, must not be present otherwise.
>>
>>> +#
>>> +# Note: this command is experimental, and not a stable API.
>>> +#
>>> +# Since: 2.5
>>> +##
>>> +{ 'command': 'x-blockdev-change',
>>> +  'data' : { 'operation': 'ChangeOperation',
>>> +             'parent': 'str',
>>> +             '*child': 'str',
>>> +             '*node': 'str' } }
>>
>> Let me suggest this alternative:
>>
>> { 'command': 'x-blockdev-change',
>>   'data' : { 'parent': 'str',
>>              'child': 'str',
>>              '*node': 'str' } }
>>
>> child doesn't describe a node name then, but a child name (adds a
>> dependency on my patches which add a name to BdrvChild, though).
> 
> Where is the patch? I don't find it.
> 
>> Depending on whether node is given and whether the child already exists,
>> this may add, remove or replace a child.
> 
> If the user wants to insert a filter driver between parent and child, we
> also needs three parameters: parent, child, node. So it is why I add the
> parameter operation.

Hi kevin, I still wait for your reply...

Thanks
Wen Congyang

> 
> Thanks
> Wen Congyang
> 
>>
>> Kevin
>> .
>>
> 
> 
> .
> 

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

[Kevin Wolf]

Am 13.11.2015 um 11:25 hat Wen Congyang geschrieben:
> On 11/10/2015 09:40 AM, Wen Congyang wrote:
> > On 11/10/2015 12:04 AM, Kevin Wolf wrote:
> >> Am 16.10.2015 um 10:57 hat Wen Congyang geschrieben:
> >>> +##
> >>> +# @ChangeOperation:
> >>> +#
> >>> +# An enumeration of block device change operation.
> >>> +#
> >>> +# @add: Add a new block driver state to a existed block driver state.
> >>> +#
> >>> +# @delete: Delete a block driver state's child.
> >>> +#
> >>> +# Since: 2.5
> >>> +##
> >>> +{ 'enum': 'ChangeOperation',
> >>> +  'data': [ 'add', 'delete' ] }
> >>
> >> What's the advantage of this enum compared to separate QMP commands? The
> >> way it is specified here, ChangeOperation is already implicit by whether
> >> or not child and node are given.
> >>
> >>> +##
> >>> +# @x-blockdev-change
> >>> +#
> >>> +# Dynamic reconfigure the block driver state graph. It can be used to
> >>> +# add, remove, insert, 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.
> >>> +#
> >>> +# @operation: the chanage operation. It can be add, delete.
> >>> +#
> >>> +# @parent: the id or node name of which node will be changed.
> >>> +#
> >>> +# @child: the child node-name which will be deleted.
> >>
> >> #optional
> >>
> >> Must be present for operation = delete, must not be present otherwise.
> >>
> >>> +# @node: the new node-name which will be added.
> >>
> >> #optional
> >>
> >> Must be present for operation = add, must not be present otherwise.
> >>
> >>> +#
> >>> +# Note: this command is experimental, and not a stable API.
> >>> +#
> >>> +# Since: 2.5
> >>> +##
> >>> +{ 'command': 'x-blockdev-change',
> >>> +  'data' : { 'operation': 'ChangeOperation',
> >>> +             'parent': 'str',
> >>> +             '*child': 'str',
> >>> +             '*node': 'str' } }
> >>
> >> Let me suggest this alternative:
> >>
> >> { 'command': 'x-blockdev-change',
> >>   'data' : { 'parent': 'str',
> >>              'child': 'str',
> >>              '*node': 'str' } }
> >>
> >> child doesn't describe a node name then, but a child name (adds a
> >> dependency on my patches which add a name to BdrvChild, though).
> > 
> > Where is the patch? I don't find it.

The current developement branch version is here:

http://repo.or.cz/qemu/kevin.git/commitdiff/b8f3aba84160564576a5a068398f20eca13768af

I hope to get the series merged early in the 2.6 cycle.

> >> Depending on whether node is given and whether the child already exists,
> >> this may add, remove or replace a child.
> > 
> > If the user wants to insert a filter driver between parent and child, we
> > also needs three parameters: parent, child, node. So it is why I add the
> > parameter operation.

The child node is uniquely identified with parent node and child name,
so my version can't describe less than something including the child
node name.

The reverse isn't true, though: In theory, the same node could be
attached twice to the same parent in different roles. Knowing the node
name doesn't uniquely identify the child name then.

Kevin

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

[Wen Congyang]

On 11/13/2015 06:53 PM, Kevin Wolf wrote:
> Am 13.11.2015 um 11:25 hat Wen Congyang geschrieben:
>> On 11/10/2015 09:40 AM, Wen Congyang wrote:
>>> On 11/10/2015 12:04 AM, Kevin Wolf wrote:
>>>> Am 16.10.2015 um 10:57 hat Wen Congyang geschrieben:
>>>>> +##
>>>>> +# @ChangeOperation:
>>>>> +#
>>>>> +# An enumeration of block device change operation.
>>>>> +#
>>>>> +# @add: Add a new block driver state to a existed block driver state.
>>>>> +#
>>>>> +# @delete: Delete a block driver state's child.
>>>>> +#
>>>>> +# Since: 2.5
>>>>> +##
>>>>> +{ 'enum': 'ChangeOperation',
>>>>> +  'data': [ 'add', 'delete' ] }
>>>>
>>>> What's the advantage of this enum compared to separate QMP commands? The
>>>> way it is specified here, ChangeOperation is already implicit by whether
>>>> or not child and node are given.
>>>>
>>>>> +##
>>>>> +# @x-blockdev-change
>>>>> +#
>>>>> +# Dynamic reconfigure the block driver state graph. It can be used to
>>>>> +# add, remove, insert, 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.
>>>>> +#
>>>>> +# @operation: the chanage operation. It can be add, delete.
>>>>> +#
>>>>> +# @parent: the id or node name of which node will be changed.
>>>>> +#
>>>>> +# @child: the child node-name which will be deleted.
>>>>
>>>> #optional
>>>>
>>>> Must be present for operation = delete, must not be present otherwise.
>>>>
>>>>> +# @node: the new node-name which will be added.
>>>>
>>>> #optional
>>>>
>>>> Must be present for operation = add, must not be present otherwise.
>>>>
>>>>> +#
>>>>> +# Note: this command is experimental, and not a stable API.
>>>>> +#
>>>>> +# Since: 2.5
>>>>> +##
>>>>> +{ 'command': 'x-blockdev-change',
>>>>> +  'data' : { 'operation': 'ChangeOperation',
>>>>> +             'parent': 'str',
>>>>> +             '*child': 'str',
>>>>> +             '*node': 'str' } }
>>>>
>>>> Let me suggest this alternative:
>>>>
>>>> { 'command': 'x-blockdev-change',
>>>>   'data' : { 'parent': 'str',
>>>>              'child': 'str',
>>>>              '*node': 'str' } }
>>>>
>>>> child doesn't describe a node name then, but a child name (adds a
>>>> dependency on my patches which add a name to BdrvChild, though).
>>>
>>> Where is the patch? I don't find it.
> 
> The current developement branch version is here:
> 
> http://repo.or.cz/qemu/kevin.git/commitdiff/b8f3aba84160564576a5a068398f20eca13768af
> 
> I hope to get the series merged early in the 2.6 cycle.
> 
>>>> Depending on whether node is given and whether the child already exists,
>>>> this may add, remove or replace a child.
>>>
>>> If the user wants to insert a filter driver between parent and child, we
>>> also needs three parameters: parent, child, node. So it is why I add the
>>> parameter operation.
> 
> The child node is uniquely identified with parent node and child name,
> so my version can't describe less than something including the child
> node name.
> 
> The reverse isn't true, though: In theory, the same node could be
> attached twice to the same parent in different roles. Knowing the node
> name doesn't uniquely identify the child name then.

Thanks for your explanation, I understand why we use the child name, not
the node name.

Do we need the parameter "operation" now? Or add this patameter in the furture?

Thanks
Wen Congyang

> 
> Kevin
> .
> 

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

[Kevin Wolf]

Am 13.11.2015 um 12:19 hat Wen Congyang geschrieben:
> On 11/13/2015 06:53 PM, Kevin Wolf wrote:
> > Am 13.11.2015 um 11:25 hat Wen Congyang geschrieben:
> >> On 11/10/2015 09:40 AM, Wen Congyang wrote:
> >>> On 11/10/2015 12:04 AM, Kevin Wolf wrote:
> >>>> Am 16.10.2015 um 10:57 hat Wen Congyang geschrieben:
> >>>>> +##
> >>>>> +# @ChangeOperation:
> >>>>> +#
> >>>>> +# An enumeration of block device change operation.
> >>>>> +#
> >>>>> +# @add: Add a new block driver state to a existed block driver state.
> >>>>> +#
> >>>>> +# @delete: Delete a block driver state's child.
> >>>>> +#
> >>>>> +# Since: 2.5
> >>>>> +##
> >>>>> +{ 'enum': 'ChangeOperation',
> >>>>> +  'data': [ 'add', 'delete' ] }
> >>>>
> >>>> What's the advantage of this enum compared to separate QMP commands? The
> >>>> way it is specified here, ChangeOperation is already implicit by whether
> >>>> or not child and node are given.
> >>>>
> >>>>> +##
> >>>>> +# @x-blockdev-change
> >>>>> +#
> >>>>> +# Dynamic reconfigure the block driver state graph. It can be used to
> >>>>> +# add, remove, insert, 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.
> >>>>> +#
> >>>>> +# @operation: the chanage operation. It can be add, delete.
> >>>>> +#
> >>>>> +# @parent: the id or node name of which node will be changed.
> >>>>> +#
> >>>>> +# @child: the child node-name which will be deleted.
> >>>>
> >>>> #optional
> >>>>
> >>>> Must be present for operation = delete, must not be present otherwise.
> >>>>
> >>>>> +# @node: the new node-name which will be added.
> >>>>
> >>>> #optional
> >>>>
> >>>> Must be present for operation = add, must not be present otherwise.
> >>>>
> >>>>> +#
> >>>>> +# Note: this command is experimental, and not a stable API.
> >>>>> +#
> >>>>> +# Since: 2.5
> >>>>> +##
> >>>>> +{ 'command': 'x-blockdev-change',
> >>>>> +  'data' : { 'operation': 'ChangeOperation',
> >>>>> +             'parent': 'str',
> >>>>> +             '*child': 'str',
> >>>>> +             '*node': 'str' } }
> >>>>
> >>>> Let me suggest this alternative:
> >>>>
> >>>> { 'command': 'x-blockdev-change',
> >>>>   'data' : { 'parent': 'str',
> >>>>              'child': 'str',
> >>>>              '*node': 'str' } }
> >>>>
> >>>> child doesn't describe a node name then, but a child name (adds a
> >>>> dependency on my patches which add a name to BdrvChild, though).
> >>>
> >>> Where is the patch? I don't find it.
> > 
> > The current developement branch version is here:
> > 
> > http://repo.or.cz/qemu/kevin.git/commitdiff/b8f3aba84160564576a5a068398f20eca13768af
> > 
> > I hope to get the series merged early in the 2.6 cycle.
> > 
> >>>> Depending on whether node is given and whether the child already exists,
> >>>> this may add, remove or replace a child.
> >>>
> >>> If the user wants to insert a filter driver between parent and child, we
> >>> also needs three parameters: parent, child, node. So it is why I add the
> >>> parameter operation.
> > 
> > The child node is uniquely identified with parent node and child name,
> > so my version can't describe less than something including the child
> > node name.
> > 
> > The reverse isn't true, though: In theory, the same node could be
> > attached twice to the same parent in different roles. Knowing the node
> > name doesn't uniquely identify the child name then.
> 
> Thanks for your explanation, I understand why we use the child name, not
> the node name.
> 
> Do we need the parameter "operation" now? Or add this patameter in the furture?

I think it's enough to add it later if needed. Or in fact, we'll
probably just replace this experimental command by something else then.

Kevin

[Qemu-devel] [PATCH v6 4/4] hmp: add monitor command to add/remove a child

[Wen Congyang]

The new command is blockdev_change. It does the same
thing as the QMP command x-blockdev-change.

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>
Cc: Luiz Capitulino <lcapitulino@redhat.com>
---
 hmp-commands.hx | 17 +++++++++++++++++
 hmp.c           | 38 ++++++++++++++++++++++++++++++++++++++
 hmp.h           |  1 +
 3 files changed, 56 insertions(+)

diff --git a/hmp-commands.hx b/hmp-commands.hx
index 3a4ae39..57475cc 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -193,6 +193,23 @@ actions (drive options rerror, werror).
 ETEXI
 
     {
+        .name       = "blockdev_change",
+        .args_type  = "op:s,parent:B,child:B?,node:?",
+        .params     = "operation parent [child] [node]",
+        .help       = "Dynamic reconfigure the block driver state graph",
+        .mhandler.cmd = hmp_blockdev_change,
+    },
+
+STEXI
+@item blockdev_change @var{operation} @var{parent} [@var{child}] [@var{node}]
+@findex blockdev_change
+Dynamic reconfigure the block driver state graph. It can be used to
+add, remove, insert, 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.
+ETEXI
+
+    {
         .name       = "change",
         .args_type  = "device:B,target:F,arg:s?",
         .params     = "device filename [format]",
diff --git a/hmp.c b/hmp.c
index 5048eee..fc58ae2 100644
--- a/hmp.c
+++ b/hmp.c
@@ -2346,3 +2346,41 @@ void hmp_rocker_of_dpa_groups(Monitor *mon, const QDict *qdict)
 
     qapi_free_RockerOfDpaGroupList(list);
 }
+
+void hmp_blockdev_change(Monitor *mon, const QDict *qdict)
+{
+    const char *operation = qdict_get_str(qdict, "op");
+    const char *parent = qdict_get_str(qdict, "parent");
+    const char *child = qdict_get_try_str(qdict, "child");
+    const char *node = qdict_get_try_str(qdict, "node");
+    ChangeOperation op = CHANGE_OPERATION_ADD;
+    Error *local_err = NULL;
+    bool has_child = !!child;
+    bool has_node = !!node;
+
+    while (ChangeOperation_lookup[op] != NULL) {
+        if (strcmp(ChangeOperation_lookup[op], operation) == 0) {
+            break;
+        }
+        op++;
+    }
+
+    if (ChangeOperation_lookup[op] == NULL) {
+        error_setg(&local_err, "Invalid parameter '%s'", "operation");
+        goto out;
+    }
+
+    /*
+     * FIXME: we must specify the parameter child, otherwise,
+     * we can't specify the parameter node.
+     */
+    if (op == CHANGE_OPERATION_ADD) {
+        has_child = false;
+    }
+
+    qmp_x_blockdev_change(op, parent, has_child, child,
+                          has_node, node, &local_err);
+
+out:
+    hmp_handle_error(mon, &local_err);
+}
diff --git a/hmp.h b/hmp.h
index 81656c3..80a1faf 100644
--- a/hmp.h
+++ b/hmp.h
@@ -130,5 +130,6 @@ void hmp_rocker(Monitor *mon, const QDict *qdict);
 void hmp_rocker_ports(Monitor *mon, const QDict *qdict);
 void hmp_rocker_of_dpa_flows(Monitor *mon, const QDict *qdict);
 void hmp_rocker_of_dpa_groups(Monitor *mon, const QDict *qdict);
+void hmp_blockdev_change(Monitor *mon, const QDict *qdict);
 
 #endif
-- 
2.4.3

Re: [Qemu-devel] [PATCH v6 4/4] hmp: add monitor command to add/remove a child

[Alberto Garcia]

On Fri 16 Oct 2015 10:57:46 AM CEST, Wen Congyang wrote:

> +        .name       = "blockdev_change",
> +        .args_type  = "op:s,parent:B,child:B?,node:?",
> +        .params     = "operation parent [child] [node]",
  [...]
> +    /*
> +     * FIXME: we must specify the parameter child, otherwise,
> +     * we can't specify the parameter node.
> +     */
> +    if (op == CHANGE_OPERATION_ADD) {
> +        has_child = false;
> +    }

So if you want to perform the 'add' operation you must pass both 'child'
and 'node' but the former will be discarded.

I don't think you really need to do this for the HMP interface, but it's
anyway one more good reason to merge 'child' and 'node'.

Berto

Re: [Qemu-devel] [PATCH v6 4/4] hmp: add monitor command to add/remove a child

[Wen Congyang]

On 11/09/2015 10:54 PM, Alberto Garcia wrote:
> On Fri 16 Oct 2015 10:57:46 AM CEST, Wen Congyang wrote:
> 
>> +        .name       = "blockdev_change",
>> +        .args_type  = "op:s,parent:B,child:B?,node:?",
>> +        .params     = "operation parent [child] [node]",
>   [...]
>> +    /*
>> +     * FIXME: we must specify the parameter child, otherwise,
>> +     * we can't specify the parameter node.
>> +     */
>> +    if (op == CHANGE_OPERATION_ADD) {
>> +        has_child = false;
>> +    }
> 
> So if you want to perform the 'add' operation you must pass both 'child'
> and 'node' but the former will be discarded.
> 
> I don't think you really need to do this for the HMP interface, but it's
> anyway one more good reason to merge 'child' and 'node'.

Do you mean there is no need to implement the HMP interface?

Thanks
Wen Congyang

> 
> Berto
> .
> 

Re: [Qemu-devel] [PATCH v6 0/4] qapi: child add/delete support

[Wen Congyang]

Ping...

On 10/16/2015 04:57 PM, Wen Congyang wrote:
> If quorum's child is broken, we can use mirror job to replace it.
> But sometimes, the user only need to remove the broken child, and
> add it later when the problem is fixed.
> 
> It is based on the Kevin's bdrv_swap() related patch:
> http://lists.nongnu.org/archive/html/qemu-devel/2015-10/msg02152.html
> 
> ChangLog:
> v6:
> 1. Use a single qmp command x-blockdev-change to replace x-blockdev-child-add
>    and x-blockdev-child-delete
> v5:
> 1. Address Eric Blake's comments
> v4:
> 1. drop nbd driver's implementation. We can use human-monitor-command
>    to do it.
> 2. Rename the command name.
> v3:
> 1. Don't open BDS in bdrv_add_child(). Use the existing BDS which is
>    created by the QMP command blockdev-add.
> 2. The driver NBD can support filename, path, host:port now.
> v2:
> 1. Use bdrv_get_device_or_node_name() instead of new function
>    bdrv_get_id_or_node_name()
> 2. Update the error message
> 3. Update the documents in block-core.json
> 
> 
> Wen Congyang (4):
>   Add new block driver interface to add/delete a BDS's child
>   quorum: implement bdrv_add_child() and bdrv_del_child()
>   qmp: add monitor command to add/remove a child
>   hmp: add monitor command to add/remove a child
> 
>  block.c                   | 56 ++++++++++++++++++++++++++++++++--
>  block/quorum.c            | 59 ++++++++++++++++++++++++++++++++++--
>  blockdev.c                | 76 +++++++++++++++++++++++++++++++++++++++++++++++
>  hmp-commands.hx           | 17 +++++++++++
>  hmp.c                     | 38 ++++++++++++++++++++++++
>  hmp.h                     |  1 +
>  include/block/block.h     |  8 +++++
>  include/block/block_int.h |  5 ++++
>  qapi/block-core.json      | 40 +++++++++++++++++++++++++
>  qmp-commands.hx           | 50 +++++++++++++++++++++++++++++++
>  10 files changed, 345 insertions(+), 5 deletions(-)
> 

Re: [Qemu-devel] [Qemu-block] [PATCH v6 0/4] qapi: child add/delete support

[Stefan Hajnoczi]

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

On Fri, Oct 30, 2015 at 02:11:30PM +0800, Wen Congyang wrote:
> Ping...

Tips for faster code review:

It helps to mention the specific person you are expecting review from
when the CC list is long.  For example, "Kevin: ping".

Keeping the CC list short can result in faster code review than a long
CC list because it's obvious who needs to reply.

Hope this helps.

Stefan

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

Re: [Qemu-devel] [Qemu-block] [PATCH v6 0/4] qapi: child add/delete support

[Wen Congyang]

On 11/13/2015 05:28 PM, Stefan Hajnoczi wrote:
> On Fri, Oct 30, 2015 at 02:11:30PM +0800, Wen Congyang wrote:
>> Ping...
> 
> Tips for faster code review:
> 
> It helps to mention the specific person you are expecting review from
> when the CC list is long.  For example, "Kevin: ping".

Do you mean when I ping a patch, I need write 'xxx: ping' if the CC list
is long?

> 
> Keeping the CC list short can result in faster code review than a long
> CC list because it's obvious who needs to reply.

Thanks, I always put them in the TO list, and the CC list contains the
people who is interested this patch.

Wen Congyang

> 
> Hope this helps.
> 
> Stefan
> 

Re: [Qemu-devel] [Qemu-block] [PATCH v6 0/4] qapi: child add/delete support

[Stefan Hajnoczi]

On Fri, Nov 13, 2015 at 5:37 PM, Wen Congyang <wency@cn.fujitsu.com> wrote:
> On 11/13/2015 05:28 PM, Stefan Hajnoczi wrote:
>> On Fri, Oct 30, 2015 at 02:11:30PM +0800, Wen Congyang wrote:
>>> Ping...
>>
>> Tips for faster code review:
>>
>> It helps to mention the specific person you are expecting review from
>> when the CC list is long.  For example, "Kevin: ping".
>
> Do you mean when I ping a patch, I need write 'xxx: ping' if the CC list
> is long?
>
>>
>> Keeping the CC list short can result in faster code review than a long
>> CC list because it's obvious who needs to reply.
>
> Thanks, I always put them in the TO list, and the CC list contains the
> people who is interested this patch.

That is good practice and usually enough.  In this case it seems
reviewers didn't act for some time and I guess it was because they
were waiting for the other reviewers (deadlock).

If you identify the person who needs to take the next action by
including their name in the ping or keeping the CC list small, then
deadlock is unlikely.

It's just a suggestion that I think will help because I noticed this
patch (and other COLO patches) sometimes don't receive review comments
for a long time.

Stefan