[Qemu-devel] [PATCH v10 0/3] qapi: child add/delete support

[Qemu-devel] [PATCH v10 0/3] qapi: child add/delete support

[Changlong Xie]

ChangLog:
v9~v10:
1. Rebase to the newest codes
2. Address comments from Berto and Max, update the documents in block-core.json 
   and qmp-commands.hx 
3. Remove redundant codes in quorum_add_child() and quorum_del_child()
v8:
1. Rebase to the newest codes
2. Address the comments from Eric Blake
v7:
1. Remove the qmp command x-blockdev-change's parameter operation according
   to Kevin's comments.
2. Remove the hmp command.
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 (3):
  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

 block.c                   |  58 ++++++++++++++++++++--
 block/quorum.c            | 122 +++++++++++++++++++++++++++++++++++++++++++++-
 blockdev.c                |  54 ++++++++++++++++++++
 include/block/block.h     |   9 ++++
 include/block/block_int.h |   5 ++
 qapi/block-core.json      |  32 ++++++++++++
 qmp-commands.hx           |  50 +++++++++++++++++++
 7 files changed, 324 insertions(+), 6 deletions(-)

-- 
1.9.3

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

[Changlong Xie]

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

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>
Signed-off-by: Changlong Xie <xiecl.fnst@cn.fujitsu.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Alberto Garcia <berto@igalia.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 efc3c43..08aa979 100644
--- a/block.c
+++ b/block.c
@@ -4399,3 +4399,53 @@ void bdrv_refresh_filename(BlockDriverState *bs)
         QDECREF(json);
     }
 }
+
+/*
+ * 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 1c4f4d8..ecde190 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -582,4 +582,9 @@ void bdrv_drained_begin(BlockDriverState *bs);
  */
 void bdrv_drained_end(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 9ef823a..89ec138 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -305,6 +305,11 @@ struct BlockDriver {
      */
     void (*bdrv_drain)(BlockDriverState *bs);
 
+    void (*bdrv_add_child)(BlockDriverState *parent, BlockDriverState *child,
+                           Error **errp);
+    void (*bdrv_del_child)(BlockDriverState *parent, BlockDriverState *child,
+                           Error **errp);
+
     QLIST_ENTRY(BlockDriver) list;
 };
 
-- 
1.9.3

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

[Changlong Xie]

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

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>
Signed-off-by: Changlong Xie <xiecl.fnst@cn.fujitsu.com>
---
 block.c               |   8 ++--
 block/quorum.c        | 122 +++++++++++++++++++++++++++++++++++++++++++++++++-
 include/block/block.h |   4 ++
 3 files changed, 128 insertions(+), 6 deletions(-)

diff --git a/block.c b/block.c
index 08aa979..c3c9dc0 100644
--- a/block.c
+++ b/block.c
@@ -1198,10 +1198,10 @@ static int bdrv_fill_options(QDict **options, const char *filename,
     return 0;
 }
 
-static BdrvChild *bdrv_attach_child(BlockDriverState *parent_bs,
-                                    BlockDriverState *child_bs,
-                                    const char *child_name,
-                                    const BdrvChildRole *child_role)
+BdrvChild *bdrv_attach_child(BlockDriverState *parent_bs,
+                             BlockDriverState *child_bs,
+                             const char *child_name,
+                             const BdrvChildRole *child_role)
 {
     BdrvChild *child = g_new(BdrvChild, 1);
     *child = (BdrvChild) {
diff --git a/block/quorum.c b/block/quorum.c
index a5ae4b8..e5a7e4f 100644
--- a/block/quorum.c
+++ b/block/quorum.c
@@ -24,6 +24,7 @@
 #include "qapi/qmp/qstring.h"
 #include "qapi-event.h"
 #include "crypto/hash.h"
+#include "qemu/bitmap.h"
 
 #define HASH_LENGTH 32
 
@@ -81,6 +82,8 @@ typedef struct BDRVQuorumState {
     bool rewrite_corrupted;/* true if the driver must rewrite-on-read corrupted
                             * block if Quorum is reached.
                             */
+    unsigned long *index_bitmap;
+    int bsize;
 
     QuorumReadPattern read_pattern;
 } BDRVQuorumState;
@@ -876,9 +879,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;
     }
@@ -927,6 +930,7 @@ static int quorum_open(BlockDriverState *bs, QDict *options, int flags,
     /* allocate the children array */
     s->children = g_new0(BdrvChild *, s->num_children);
     opened = g_new0(bool, s->num_children);
+    s->index_bitmap = bitmap_new(s->num_children);
 
     for (i = 0; i < s->num_children; i++) {
         char indexstr[32];
@@ -942,6 +946,8 @@ static int quorum_open(BlockDriverState *bs, QDict *options, int flags,
 
         opened[i] = true;
     }
+    bitmap_set(s->index_bitmap, 0, s->num_children);
+    s->bsize = s->num_children;
 
     g_free(opened);
     goto exit;
@@ -998,6 +1004,115 @@ static void quorum_attach_aio_context(BlockDriverState *bs,
     }
 }
 
+static int get_new_child_index(BDRVQuorumState *s)
+{
+    int index;
+
+    index = find_next_zero_bit(s->index_bitmap, s->bsize, 0);
+    if (index < s->bsize) {
+        return index;
+    }
+
+    if ((s->bsize % BITS_PER_LONG) == 0) {
+        s->index_bitmap = bitmap_zero_extend(s->index_bitmap, s->bsize,
+                                             s->bsize + 1);
+    }
+
+    return s->bsize++;
+}
+
+static void remove_child_index(BDRVQuorumState *s, int index)
+{
+    int last_index;
+    long new_len;
+
+    assert(index < s->bsize);
+
+    clear_bit(index, s->index_bitmap);
+    if (index < s->bsize - 1) {
+        /*
+         * The last bit is always set, and we don't clear
+         * the last bit.
+         */
+        return;
+    }
+
+    last_index = find_last_bit(s->index_bitmap, s->bsize);
+    s->bsize = last_index + 1;
+    if (BITS_TO_LONGS(last_index + 1) == BITS_TO_LONGS(s->bsize)) {
+        return;
+    }
+
+    new_len = BITS_TO_LONGS(last_index + 1) * sizeof(unsigned long);
+    s->index_bitmap = g_realloc(s->index_bitmap, new_len);
+}
+
+static void quorum_add_child(BlockDriverState *bs, BlockDriverState *child_bs,
+                             Error **errp)
+{
+    BDRVQuorumState *s = bs->opaque;
+    BdrvChild *child;
+    char indexstr[32];
+    int index, ret;
+
+    index = get_new_child_index(s);
+    ret = snprintf(indexstr, 32, "children.%d", index);
+    if (ret < 0 || ret >= 32) {
+        error_setg(errp, "cannot generate child name");
+        return;
+    }
+
+    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, indexstr, &child_format);
+    s->children[s->num_children++] = child;
+    set_bit(index, s->index_bitmap);
+}
+
+static void quorum_del_child(BlockDriverState *bs, BlockDriverState *child_bs,
+                             Error **errp)
+{
+    BDRVQuorumState *s = bs->opaque;
+    BdrvChild *child;
+    int i, index;
+
+    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;
+    }
+
+    /* child->name is "children.%d" */
+    index = atoi(child->name + 9);
+
+    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);
+    remove_child_index(s, index);
+    bdrv_unref_child(bs, child);
+}
+
 static void quorum_refresh_filename(BlockDriverState *bs, QDict *options)
 {
     BDRVQuorumState *s = bs->opaque;
@@ -1053,6 +1168,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 ecde190..4b787d2 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -517,6 +517,10 @@ 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 char *child_name,
+                             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);
-- 
1.9.3

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

[Changlong Xie]

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

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 <wency@cn.fujitsu.com>
Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
Signed-off-by: Gonglei <arei.gonglei@huawei.com>
Signed-off-by: Changlong Xie <xiecl.fnst@cn.fujitsu.com>
---
 blockdev.c           | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 qapi/block-core.json | 32 +++++++++++++++++++++++++++++++
 qmp-commands.hx      | 50 ++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 136 insertions(+)

diff --git a/blockdev.c b/blockdev.c
index 1f73478..ca040b0 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3983,6 +3983,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 parameters child and node are in conflict");
+        } else {
+            error_setg(errp, "Either child or node must be specified");
+        }
+        return;
+    }
+
+    if (has_child) {
+        child_bs = bdrv_find_child(parent_bs, child);
+        if (!child_bs) {
+            error_setg(errp, "Node '%s' does not have child '%s'",
+                       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 33012b8..92eb7fe 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2482,3 +2482,35 @@
 ##
 { '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 graph node. Currently only the
+# Quorum driver implements this feature to add or remove its child. This
+# is useful to fix a broken quorum child.
+#
+# If @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 @node is not, @child will be detached from @parent.
+#
+# @parent: the id or name of the parent node.
+#
+# @child: #optional the name of a child under the given parent node.
+#
+# @node: #optional the name of the node that will be added.
+#
+# Note: this command is experimental, and its API is not stable. It
+# does not support all kinds of operations, all kinds of children, nor
+# all block drivers.
+#
+# Warning: The data in a new quorum child MUST be consistent with that of
+# the rest of the array.
+#
+# 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 020e5ee..1c9a06f 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -4364,6 +4364,56 @@ 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 graph node. Currently only the
+Quorum driver implements this feature to add or remove its child. This
+is useful to fix a broken quorum child.
+
+Arguments:
+- "parent": the id or name of the parent node (json-string)
+- "child": the name of a child under the given parent node (json-string, optional)
+- "node": the name of the node that 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.
+
+Warning: The data in a new quorum child MUST be consistent with that of
+the rest of the array.
+
+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,
-- 
1.9.3

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

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 5728 bytes --]

Sorry that I wasn't so pedantic last time; or maybe I should rather be
sorry that I'm so pedantic this time.

On 16.02.2016 10:37, Changlong Xie wrote:
> From: Wen Congyang <wency@cn.fujitsu.com>
> 
> 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>
> Signed-off-by: Changlong Xie <xiecl.fnst@cn.fujitsu.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Reviewed-by: Alberto Garcia <berto@igalia.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 efc3c43..08aa979 100644
> --- a/block.c
> +++ b/block.c
> @@ -4399,3 +4399,53 @@ void bdrv_refresh_filename(BlockDriverState *bs)
>          QDECREF(json);
>      }
>  }
> +
> +/*
> + * 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",

As I said in my reply to v9's patch 3 (and I see you've followed it), I
don't quite like contractions in error messages, so I'd rather like this
to be "does not" instead of "doesn't".

If you don't decide to change this patch, then feel free to leave this
as it is, because that way you can keep Eric's and Berto's R-bs.

> +                   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)

I wondered before and now I'm wondering why I didn't say anything. Why
is this function taking a BDS pointer as child_bs? Why not just
"BdrvChild *child"? Its only caller will be qmp_x_blockdev_change()
which gets the child BDS from bdrv_find_child(), which could just as
well return a BdrvChild instead of a BDS pointer.

I see two advantages to this: First, it's a bit clearer since this
operation actually does not delete the child *BDS* but only the *edge*
between parent and child, and that edge is what a BdrvChild is.

And second, some block drivers may prefer the BdrvChild over the BDS
itself. They can always trivially get the BDS from the BdrvChild
structure, but the other way around is difficult.

The only disadvantage I can see is that it then becomes asymmetric to
bdrv_add_child(). But that's fine, it's a different function after all.
bdrv_add_child() creates a BdrvChild object (an edge), bdrv_del_child()
deletes it.

(I don't know whether you had this discussion with someone else before,
though. Sorry if you did, I missed it, then.)

> +{
> +    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));

Again, optional s/doesn't/does not/.

> +        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));

Is there a special reason why you are using
bdrv_get_device_or_node_name() for the child here, while in
bdrv_add_child() you directly use the node name?

Neither seems wrong to me. A child is unlikely to have a BB of its own,
but if it does, printing its name instead of the node name may be
appropriate. I'm just wondering about the difference between
bdrv_add_child() and bdrv_del_child().

Max

> +        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 1c4f4d8..ecde190 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -582,4 +582,9 @@ void bdrv_drained_begin(BlockDriverState *bs);
>   */
>  void bdrv_drained_end(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 9ef823a..89ec138 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -305,6 +305,11 @@ struct BlockDriver {
>       */
>      void (*bdrv_drain)(BlockDriverState *bs);
>  
> +    void (*bdrv_add_child)(BlockDriverState *parent, BlockDriverState *child,
> +                           Error **errp);
> +    void (*bdrv_del_child)(BlockDriverState *parent, BlockDriverState *child,
> +                           Error **errp);
> +
>      QLIST_ENTRY(BlockDriver) list;
>  };
>  
> 



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

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

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 9492 bytes --]

On 16.02.2016 10:37, Changlong Xie wrote:
> From: Wen Congyang <wency@cn.fujitsu.com>
> 
> 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>
> Signed-off-by: Changlong Xie <xiecl.fnst@cn.fujitsu.com>
> ---
>  block.c               |   8 ++--
>  block/quorum.c        | 122 +++++++++++++++++++++++++++++++++++++++++++++++++-
>  include/block/block.h |   4 ++
>  3 files changed, 128 insertions(+), 6 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 08aa979..c3c9dc0 100644
> --- a/block.c
> +++ b/block.c
> @@ -1198,10 +1198,10 @@ static int bdrv_fill_options(QDict **options, const char *filename,
>      return 0;
>  }
>  
> -static BdrvChild *bdrv_attach_child(BlockDriverState *parent_bs,
> -                                    BlockDriverState *child_bs,
> -                                    const char *child_name,
> -                                    const BdrvChildRole *child_role)
> +BdrvChild *bdrv_attach_child(BlockDriverState *parent_bs,
> +                             BlockDriverState *child_bs,
> +                             const char *child_name,
> +                             const BdrvChildRole *child_role)
>  {
>      BdrvChild *child = g_new(BdrvChild, 1);
>      *child = (BdrvChild) {
> diff --git a/block/quorum.c b/block/quorum.c
> index a5ae4b8..e5a7e4f 100644
> --- a/block/quorum.c
> +++ b/block/quorum.c
> @@ -24,6 +24,7 @@
>  #include "qapi/qmp/qstring.h"
>  #include "qapi-event.h"
>  #include "crypto/hash.h"
> +#include "qemu/bitmap.h"
>  
>  #define HASH_LENGTH 32
>  
> @@ -81,6 +82,8 @@ typedef struct BDRVQuorumState {
>      bool rewrite_corrupted;/* true if the driver must rewrite-on-read corrupted
>                              * block if Quorum is reached.
>                              */
> +    unsigned long *index_bitmap;
> +    int bsize;
>  
>      QuorumReadPattern read_pattern;
>  } BDRVQuorumState;
> @@ -876,9 +879,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");

Side note: Actually, we could work with 0 children, too. Quorum would
then need to implement bdrv_is_inserted() and return false if there are
no children.

But that is something that can be implemented later on if the need arises.

>          ret = -EINVAL;
>          goto exit;
>      }
> @@ -927,6 +930,7 @@ static int quorum_open(BlockDriverState *bs, QDict *options, int flags,
>      /* allocate the children array */
>      s->children = g_new0(BdrvChild *, s->num_children);
>      opened = g_new0(bool, s->num_children);
> +    s->index_bitmap = bitmap_new(s->num_children);
>  
>      for (i = 0; i < s->num_children; i++) {
>          char indexstr[32];
> @@ -942,6 +946,8 @@ static int quorum_open(BlockDriverState *bs, QDict *options, int flags,
>  
>          opened[i] = true;
>      }
> +    bitmap_set(s->index_bitmap, 0, s->num_children);
> +    s->bsize = s->num_children;
>  
>      g_free(opened);
>      goto exit;
> @@ -998,6 +1004,115 @@ static void quorum_attach_aio_context(BlockDriverState *bs,
>      }
>  }
>  
> +static int get_new_child_index(BDRVQuorumState *s)
> +{
> +    int index;
> +
> +    index = find_next_zero_bit(s->index_bitmap, s->bsize, 0);
> +    if (index < s->bsize) {
> +        return index;
> +    }
> +
> +    if ((s->bsize % BITS_PER_LONG) == 0) {
> +        s->index_bitmap = bitmap_zero_extend(s->index_bitmap, s->bsize,
> +                                             s->bsize + 1);

I think this function needs to be called unconditionally. Looking into
its implementation, its call to g_realloc() will not do anything (and it
will probably be pretty quick at that), but the following bitmap_clear()
will only clear the bits from old_nbits (s->bsize) to new_nbits
(s->bsize + 1).

Thus, if you only call this function every 32nd/64th child, only that
child's bit will be initialized to zero. All the rest is undefined.

You probably didn't notice because bitmap_new() returns a
zero-initialized bitmap, and thus you'd have to create around 64
children (on an x64 machine) to notice.

> +    }
> +
> +    return s->bsize++;
> +}
> +
> +static void remove_child_index(BDRVQuorumState *s, int index)
> +{
> +    int last_index;
> +    long new_len;

size_t would be the more appropriate type.

> +
> +    assert(index < s->bsize);
> +
> +    clear_bit(index, s->index_bitmap);
> +    if (index < s->bsize - 1) {
> +        /*
> +         * The last bit is always set, and we don't clear

s/don't/didn't/

> +         * the last bit.
> +         */
> +        return;
> +    }
> +
> +    last_index = find_last_bit(s->index_bitmap, s->bsize);

An assert(last_index < s->bsize); here wouldn't hurt.

(last_index == s->bsize would be the case if no bit is set in
s->index_bitmap anymore, which should be impossible.)

> +    s->bsize = last_index + 1;
> +    if (BITS_TO_LONGS(last_index + 1) == BITS_TO_LONGS(s->bsize)) {
> +        return;
> +    }
> +
> +    new_len = BITS_TO_LONGS(last_index + 1) * sizeof(unsigned long);

s/last_index + 1/s->bsize/ looks better to me.

> +    s->index_bitmap = g_realloc(s->index_bitmap, new_len);
> +}
> +
> +static void quorum_add_child(BlockDriverState *bs, BlockDriverState *child_bs,
> +                             Error **errp)
> +{
> +    BDRVQuorumState *s = bs->opaque;
> +    BdrvChild *child;
> +    char indexstr[32];
> +    int index, ret;
> +
> +    index = get_new_child_index(s);
> +    ret = snprintf(indexstr, 32, "children.%d", index);
> +    if (ret < 0 || ret >= 32) {
> +        error_setg(errp, "cannot generate child name");
> +        return;
> +    }
> +
> +    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, indexstr, &child_format);
> +    s->children[s->num_children++] = child;
> +    set_bit(index, s->index_bitmap);
> +}
> +
> +static void quorum_del_child(BlockDriverState *bs, BlockDriverState *child_bs,
> +                             Error **errp)
> +{
> +    BDRVQuorumState *s = bs->opaque;
> +    BdrvChild *child;
> +    int i, index;
> +
> +    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;
> +    }
> +
> +    /* child->name is "children.%d" */

Optional: assert(!strncmp(child->name, "children.", 9));

> +    index = atoi(child->name + 9);

Optional: Assert absence of an error:

unsigned long index;
char *endptr;

index = strtoul(child->name + 9, &endptr, 10);
assert(index >= 0 && !*endptr);

Max

> +
> +    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);
> +    remove_child_index(s, index);
> +    bdrv_unref_child(bs, child);
> +}
> +
>  static void quorum_refresh_filename(BlockDriverState *bs, QDict *options)
>  {
>      BDRVQuorumState *s = bs->opaque;
> @@ -1053,6 +1168,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 ecde190..4b787d2 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -517,6 +517,10 @@ 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 char *child_name,
> +                             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);
> 



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

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

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 6787 bytes --]

On 16.02.2016 10:37, Changlong Xie wrote:
> From: Wen Congyang <wency@cn.fujitsu.com>
> 
> 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 <wency@cn.fujitsu.com>
> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> Signed-off-by: Changlong Xie <xiecl.fnst@cn.fujitsu.com>
> ---
>  blockdev.c           | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  qapi/block-core.json | 32 +++++++++++++++++++++++++++++++
>  qmp-commands.hx      | 50 ++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 136 insertions(+)
> 
> diff --git a/blockdev.c b/blockdev.c
> index 1f73478..ca040b0 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -3983,6 +3983,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;
> +}

As I said for patch 1, making this function return a BdrvChild would be
trivial...

> +
> +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 parameters child and node are in conflict");
> +        } else {
> +            error_setg(errp, "Either child or node must be specified");
> +        }
> +        return;
> +    }
> +
> +    if (has_child) {
> +        child_bs = bdrv_find_child(parent_bs, child);
> +        if (!child_bs) {
> +            error_setg(errp, "Node '%s' does not have child '%s'",
> +                       parent, child);
> +            return;
> +        }
> +        bdrv_del_child(parent_bs, child_bs, errp);

...and then we could pass the BdrvChild here.

(It's your choice.)

> +    }
> +
> +    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 33012b8..92eb7fe 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -2482,3 +2482,35 @@
>  ##
>  { '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 graph node. Currently only the
> +# Quorum driver implements this feature to add or remove its child. This
> +# is useful to fix a broken quorum child.
> +#
> +# If @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 @node is not, @child will be detached from @parent.
> +#
> +# @parent: the id or name of the parent node.
> +#
> +# @child: #optional the name of a child under the given parent node.
> +#
> +# @node: #optional the name of the node that will be added.
> +#
> +# Note: this command is experimental, and its API is not stable. It
> +# does not support all kinds of operations, all kinds of children, nor
> +# all block drivers.
> +#
> +# Warning: The data in a new quorum child MUST be consistent with that of
> +# the rest of the array.
> +#
> +# 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 020e5ee..1c9a06f 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -4364,6 +4364,56 @@ 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 graph node. Currently only the
> +Quorum driver implements this feature to add or remove its child. This
> +is useful to fix a broken quorum child.

The explanation of its behavior is missing here (the second paragraph in
the qapi/block-core.json comment).

Max

> +
> +Arguments:
> +- "parent": the id or name of the parent node (json-string)
> +- "child": the name of a child under the given parent node (json-string, optional)
> +- "node": the name of the node that 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.
> +
> +Warning: The data in a new quorum child MUST be consistent with that of
> +the rest of the array.
> +
> +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,
> 



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

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

[Changlong Xie]

On 03/06/2016 01:27 AM, Max Reitz wrote:
> Sorry that I wasn't so pedantic last time; or maybe I should rather be
> sorry that I'm so pedantic this time.

Hi Max
	Welcome all your comments : )

>
> On 16.02.2016 10:37, Changlong Xie wrote:
>> From: Wen Congyang <wency@cn.fujitsu.com>
>>
>> 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>
>> Signed-off-by: Changlong Xie <xiecl.fnst@cn.fujitsu.com>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>> Reviewed-by: Alberto Garcia <berto@igalia.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 efc3c43..08aa979 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -4399,3 +4399,53 @@ void bdrv_refresh_filename(BlockDriverState *bs)
>>           QDECREF(json);
>>       }
>>   }
>> +
>> +/*
>> + * 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",
>
> As I said in my reply to v9's patch 3 (and I see you've followed it), I
> don't quite like contractions in error messages, so I'd rather like this
> to be "does not" instead of "doesn't".

Okay

>
> If you don't decide to change this patch, then feel free to leave this
> as it is, because that way you can keep Eric's and Berto's R-bs.
>
>> +                   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)
>
> I wondered before and now I'm wondering why I didn't say anything. Why
> is this function taking a BDS pointer as child_bs? Why not just
> "BdrvChild *child"? Its only caller will be qmp_x_blockdev_change()
> which gets the child BDS from bdrv_find_child(), which could just as
> well return a BdrvChild instead of a BDS pointer.
>
> I see two advantages to this: First, it's a bit clearer since this
> operation actually does not delete the child *BDS* but only the *edge*
> between parent and child, and that edge is what a BdrvChild is.

That's convincing.

>
> And second, some block drivers may prefer the BdrvChild over the BDS
> itself. They can always trivially get the BDS from the BdrvChild
> structure, but the other way around is difficult.
>
> The only disadvantage I can see is that it then becomes asymmetric to
> bdrv_add_child(). But that's fine, it's a different function after all.

As you said, they are different fuctions at all. So i don't think it's a 
big deal.

> bdrv_add_child() creates a BdrvChild object (an edge), bdrv_del_child()
> deletes it.
>
> (I don't know whether you had this discussion with someone else before,
> though. Sorry if you did, I missed it, then.)
>
>> +{
>> +    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));
>
> Again, optional s/doesn't/does not/.

okay

>
>> +        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));
>
> Is there a special reason why you are using
> bdrv_get_device_or_node_name() for the child here, while in
> bdrv_add_child() you directly use the node name?
>

Although we directly use the node name in bdrv_add_child(), but we still 
need treat bdrv_del_child() as a separate function, right? In up 
condition, we can't determine if child->bs supports BB or not, so i 
think bdrv_get_device_or_node_name(child->bs) is ok here.

> Neither seems wrong to me. A child is unlikely to have a BB of its own,
> but if it does, printing its name instead of the node name may be

bdrv_get_device_or_node_name() can do that.

Thanks
	-Xie

> appropriate. I'm just wondering about the difference between
> bdrv_add_child() and bdrv_del_child().
>
> Max
>
>> +        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 1c4f4d8..ecde190 100644
>> --- a/include/block/block.h
>> +++ b/include/block/block.h
>> @@ -582,4 +582,9 @@ void bdrv_drained_begin(BlockDriverState *bs);
>>    */
>>   void bdrv_drained_end(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 9ef823a..89ec138 100644
>> --- a/include/block/block_int.h
>> +++ b/include/block/block_int.h
>> @@ -305,6 +305,11 @@ struct BlockDriver {
>>        */
>>       void (*bdrv_drain)(BlockDriverState *bs);
>>
>> +    void (*bdrv_add_child)(BlockDriverState *parent, BlockDriverState *child,
>> +                           Error **errp);
>> +    void (*bdrv_del_child)(BlockDriverState *parent, BlockDriverState *child,
>> +                           Error **errp);
>> +
>>       QLIST_ENTRY(BlockDriver) list;
>>   };
>>
>>
>
>

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

[Changlong Xie]

On 03/06/2016 02:13 AM, Max Reitz wrote:
> On 16.02.2016 10:37, Changlong Xie wrote:
>> From: Wen Congyang <wency@cn.fujitsu.com>
>>
>> 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>
>> Signed-off-by: Changlong Xie <xiecl.fnst@cn.fujitsu.com>
>> ---
>>   block.c               |   8 ++--
>>   block/quorum.c        | 122 +++++++++++++++++++++++++++++++++++++++++++++++++-
>>   include/block/block.h |   4 ++
>>   3 files changed, 128 insertions(+), 6 deletions(-)
>>
>> diff --git a/block.c b/block.c
>> index 08aa979..c3c9dc0 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -1198,10 +1198,10 @@ static int bdrv_fill_options(QDict **options, const char *filename,
>>       return 0;
>>   }
>>
>> -static BdrvChild *bdrv_attach_child(BlockDriverState *parent_bs,
>> -                                    BlockDriverState *child_bs,
>> -                                    const char *child_name,
>> -                                    const BdrvChildRole *child_role)
>> +BdrvChild *bdrv_attach_child(BlockDriverState *parent_bs,
>> +                             BlockDriverState *child_bs,
>> +                             const char *child_name,
>> +                             const BdrvChildRole *child_role)
>>   {
>>       BdrvChild *child = g_new(BdrvChild, 1);
>>       *child = (BdrvChild) {
>> diff --git a/block/quorum.c b/block/quorum.c
>> index a5ae4b8..e5a7e4f 100644
>> --- a/block/quorum.c
>> +++ b/block/quorum.c
>> @@ -24,6 +24,7 @@
>>   #include "qapi/qmp/qstring.h"
>>   #include "qapi-event.h"
>>   #include "crypto/hash.h"
>> +#include "qemu/bitmap.h"
>>
>>   #define HASH_LENGTH 32
>>
>> @@ -81,6 +82,8 @@ typedef struct BDRVQuorumState {
>>       bool rewrite_corrupted;/* true if the driver must rewrite-on-read corrupted
>>                               * block if Quorum is reached.
>>                               */
>> +    unsigned long *index_bitmap;
>> +    int bsize;
>>
>>       QuorumReadPattern read_pattern;
>>   } BDRVQuorumState;
>> @@ -876,9 +879,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");
>
> Side note: Actually, we could work with 0 children, too. Quorum would
> then need to implement bdrv_is_inserted() and return false if there are
> no children.
>
> But that is something that can be implemented later on if the need arises.

Hi Max

Thanks for pointing it out.

>
>>           ret = -EINVAL;
>>           goto exit;
>>       }
>> @@ -927,6 +930,7 @@ static int quorum_open(BlockDriverState *bs, QDict *options, int flags,
>>       /* allocate the children array */
>>       s->children = g_new0(BdrvChild *, s->num_children);
>>       opened = g_new0(bool, s->num_children);
>> +    s->index_bitmap = bitmap_new(s->num_children);
>>
>>       for (i = 0; i < s->num_children; i++) {
>>           char indexstr[32];
>> @@ -942,6 +946,8 @@ static int quorum_open(BlockDriverState *bs, QDict *options, int flags,
>>
>>           opened[i] = true;
>>       }
>> +    bitmap_set(s->index_bitmap, 0, s->num_children);
>> +    s->bsize = s->num_children;
>>
>>       g_free(opened);
>>       goto exit;
>> @@ -998,6 +1004,115 @@ static void quorum_attach_aio_context(BlockDriverState *bs,
>>       }
>>   }
>>
>> +static int get_new_child_index(BDRVQuorumState *s)
>> +{
>> +    int index;
>> +
>> +    index = find_next_zero_bit(s->index_bitmap, s->bsize, 0);
>> +    if (index < s->bsize) {
>> +        return index;
>> +    }
>> +
>> +    if ((s->bsize % BITS_PER_LONG) == 0) {
>> +        s->index_bitmap = bitmap_zero_extend(s->index_bitmap, s->bsize,
>> +                                             s->bsize + 1);
>
> I think this function needs to be called unconditionally. Looking into
> its implementation, its call to g_realloc() will not do anything (and it
> will probably be pretty quick at that), but the following bitmap_clear()

Yes. If "BITS_TO_LONGS(new_nbits) == BITS_TO_LONGS(old_nbits)", 
g_realloc will do nothing.

> will only clear the bits from old_nbits (s->bsize) to new_nbits
> (s->bsize + 1).
>
> Thus, if you only call this function every 32nd/64th child, only that
> child's bit will be initialized to zero. All the rest is undefined.
>
> You probably didn't notice because bitmap_new() returns a
> zero-initialized bitmap, and thus you'd have to create around 64
> children (on an x64 machine) to notice

OOH! you're catching a *BIG* fish here. I'll remove the wrong "if" 
condition next version. *Thanks*

>
>> +    }
>> +
>> +    return s->bsize++;
>> +}
>> +
>> +static void remove_child_index(BDRVQuorumState *s, int index)
>> +{
>> +    int last_index;
>> +    long new_len;
>
> size_t would be the more appropriate type.

okay

>
>> +
>> +    assert(index < s->bsize);
>> +
>> +    clear_bit(index, s->index_bitmap);
>> +    if (index < s->bsize - 1) {
>> +        /*
>> +         * The last bit is always set, and we don't clear
>
> s/don't/didn't/

I'm going to remove "and we don't clear the last bit" here.

>
>> +         * the last bit.
>> +         */
>> +        return;
>> +    }
>> +
>> +    last_index = find_last_bit(s->index_bitmap, s->bsize);
>
> An assert(last_index < s->bsize); here wouldn't hurt.
>

okay.

> (last_index == s->bsize would be the case if no bit is set in
> s->index_bitmap anymore, which should be impossible.)
>
>> +    s->bsize = last_index + 1;
>> +    if (BITS_TO_LONGS(last_index + 1) == BITS_TO_LONGS(s->bsize)) {

I correct myself here, it should be "BITS_TO_LONGS(old_bsize) == 
BITS_TO_LONGS(s->bsize)".

>> +        return;
>> +    }
>> +
>> +    new_len = BITS_TO_LONGS(last_index + 1) * sizeof(unsigned long);
>
> s/last_index + 1/s->bsize/ looks better to me.

okay.

>
>> +    s->index_bitmap = g_realloc(s->index_bitmap, new_len);
>> +}
>> +
>> +static void quorum_add_child(BlockDriverState *bs, BlockDriverState *child_bs,
>> +                             Error **errp)
>> +{
>> +    BDRVQuorumState *s = bs->opaque;
>> +    BdrvChild *child;
>> +    char indexstr[32];
>> +    int index, ret;
>> +
>> +    index = get_new_child_index(s);
>> +    ret = snprintf(indexstr, 32, "children.%d", index);
>> +    if (ret < 0 || ret >= 32) {
>> +        error_setg(errp, "cannot generate child name");
>> +        return;
>> +    }
>> +
>> +    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, indexstr, &child_format);
>> +    s->children[s->num_children++] = child;
>> +    set_bit(index, s->index_bitmap);
>> +}
>> +
>> +static void quorum_del_child(BlockDriverState *bs, BlockDriverState *child_bs,
>> +                             Error **errp)
>> +{
>> +    BDRVQuorumState *s = bs->opaque;
>> +    BdrvChild *child;
>> +    int i, index;
>> +
>> +    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;
>> +    }
>> +
>> +    /* child->name is "children.%d" */
>
> Optional: assert(!strncmp(child->name, "children.", 9));
>
>> +    index = atoi(child->name + 9);
>
> Optional: Assert absence of an error:
>
> unsigned long index;
> char *endptr;
>
> index = strtoul(child->name + 9, &endptr, 10);
> assert(index >= 0 && !*endptr);

Really useful, but since we strictly named 'child->name' in 
quorum_add_child, let's just keep the orignal one.

Thanks
	-Xie
>
> Max
>
>> +
>> +    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);
>> +    remove_child_index(s, index);
>> +    bdrv_unref_child(bs, child);
>> +}
>> +
>>   static void quorum_refresh_filename(BlockDriverState *bs, QDict *options)
>>   {
>>       BDRVQuorumState *s = bs->opaque;
>> @@ -1053,6 +1168,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 ecde190..4b787d2 100644
>> --- a/include/block/block.h
>> +++ b/include/block/block.h
>> @@ -517,6 +517,10 @@ 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 char *child_name,
>> +                             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);
>>
>
>

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

[Changlong Xie]

On 03/06/2016 02:33 AM, Max Reitz wrote:
> On 16.02.2016 10:37, Changlong Xie wrote:
>> From: Wen Congyang <wency@cn.fujitsu.com>
>>
>> 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 <wency@cn.fujitsu.com>
>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
>> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
>> Signed-off-by: Changlong Xie <xiecl.fnst@cn.fujitsu.com>
>> ---
>>   blockdev.c           | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   qapi/block-core.json | 32 +++++++++++++++++++++++++++++++
>>   qmp-commands.hx      | 50 ++++++++++++++++++++++++++++++++++++++++++++++++
>>   3 files changed, 136 insertions(+)
>>
>> diff --git a/blockdev.c b/blockdev.c
>> index 1f73478..ca040b0 100644
>> --- a/blockdev.c
>> +++ b/blockdev.c
>> @@ -3983,6 +3983,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;
>> +}
>
> As I said for patch 1, making this function return a BdrvChild would be
> trivial...
>

Hi Max

okay.

>> +
>> +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 parameters child and node are in conflict");
>> +        } else {
>> +            error_setg(errp, "Either child or node must be specified");
>> +        }
>> +        return;
>> +    }
>> +
>> +    if (has_child) {
>> +        child_bs = bdrv_find_child(parent_bs, child);
>> +        if (!child_bs) {
>> +            error_setg(errp, "Node '%s' does not have child '%s'",
>> +                       parent, child);
>> +            return;
>> +        }
>> +        bdrv_del_child(parent_bs, child_bs, errp);
>
> ...and then we could pass the BdrvChild here.

ditto

>
> (It's your choice.)
>
>> +    }
>> +
>> +    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 33012b8..92eb7fe 100644
>> --- a/qapi/block-core.json
>> +++ b/qapi/block-core.json
>> @@ -2482,3 +2482,35 @@
>>   ##
>>   { '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 graph node. Currently only the
>> +# Quorum driver implements this feature to add or remove its child. This
>> +# is useful to fix a broken quorum child.
>> +#
>> +# If @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 @node is not, @child will be detached from @parent.
>> +#
>> +# @parent: the id or name of the parent node.
>> +#
>> +# @child: #optional the name of a child under the given parent node.
>> +#
>> +# @node: #optional the name of the node that will be added.
>> +#
>> +# Note: this command is experimental, and its API is not stable. It
>> +# does not support all kinds of operations, all kinds of children, nor
>> +# all block drivers.
>> +#
>> +# Warning: The data in a new quorum child MUST be consistent with that of
>> +# the rest of the array.
>> +#
>> +# 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 020e5ee..1c9a06f 100644
>> --- a/qmp-commands.hx
>> +++ b/qmp-commands.hx
>> @@ -4364,6 +4364,56 @@ 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 graph node. Currently only the
>> +Quorum driver implements this feature to add or remove its child. This
>> +is useful to fix a broken quorum child.
>
> The explanation of its behavior is missing here (the second paragraph in
> the qapi/block-core.json comment).
>

Ditto.

Thanks
	-Xie

> Max
>
>> +
>> +Arguments:
>> +- "parent": the id or name of the parent node (json-string)
>> +- "child": the name of a child under the given parent node (json-string, optional)
>> +- "node": the name of the node that 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.
>> +
>> +Warning: The data in a new quorum child MUST be consistent with that of
>> +the rest of the array.
>> +
>> +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,
>>
>
>

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

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 4409 bytes --]

On 07.03.2016 05:16, Changlong Xie wrote:
> On 03/06/2016 01:27 AM, Max Reitz wrote:
>> Sorry that I wasn't so pedantic last time; or maybe I should rather be
>> sorry that I'm so pedantic this time.
> 
> Hi Max
>     Welcome all your comments : )

Good :-)

>> On 16.02.2016 10:37, Changlong Xie wrote:
>>> From: Wen Congyang <wency@cn.fujitsu.com>
>>>
>>> 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>
>>> Signed-off-by: Changlong Xie <xiecl.fnst@cn.fujitsu.com>
>>> Reviewed-by: Eric Blake <eblake@redhat.com>
>>> Reviewed-by: Alberto Garcia <berto@igalia.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 efc3c43..08aa979 100644
>>> --- a/block.c
>>> +++ b/block.c

[...]

>>> +        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));
>>
>> Is there a special reason why you are using
>> bdrv_get_device_or_node_name() for the child here, while in
>> bdrv_add_child() you directly use the node name?
>>
> 
> Although we directly use the node name in bdrv_add_child(), but we still
> need treat bdrv_del_child() as a separate function, right? In up
> condition, we can't determine if child->bs supports BB or not, so i
> think bdrv_get_device_or_node_name(child->bs) is ok here.

I just realized that in order to invoke bdrv_add_child() one passes a
node name to x-blockdev-change, whereas for bdrv_del_child() name of the
child is passed (which is not a node name).

So it makes perfect sense to always emit the node name in
bdrv_add_child(), regardless of whether the BDS has a BB, because the
node name was the parameter that had been given to x-blockdev-change.

In contrast, the supposedly child node passed to bdrv_del_child() is not
identified by its node name, so it makes sense not to limit the output
to the node name but to print the BB's name if present.

So indeed, this is completely fine as it is in this patch.

Max

>> Neither seems wrong to me. A child is unlikely to have a BB of its own,
>> but if it does, printing its name instead of the node name may be
> 
> bdrv_get_device_or_node_name() can do that.
> 
> Thanks
>     -Xie
> 
>> appropriate. I'm just wondering about the difference between
>> bdrv_add_child() and bdrv_del_child().
>>
>> Max
>>
>>> +        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 1c4f4d8..ecde190 100644
>>> --- a/include/block/block.h
>>> +++ b/include/block/block.h
>>> @@ -582,4 +582,9 @@ void bdrv_drained_begin(BlockDriverState *bs);
>>>    */
>>>   void bdrv_drained_end(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 9ef823a..89ec138 100644
>>> --- a/include/block/block_int.h
>>> +++ b/include/block/block_int.h
>>> @@ -305,6 +305,11 @@ struct BlockDriver {
>>>        */
>>>       void (*bdrv_drain)(BlockDriverState *bs);
>>>
>>> +    void (*bdrv_add_child)(BlockDriverState *parent,
>>> BlockDriverState *child,
>>> +                           Error **errp);
>>> +    void (*bdrv_del_child)(BlockDriverState *parent,
>>> BlockDriverState *child,
>>> +                           Error **errp);
>>> +
>>>       QLIST_ENTRY(BlockDriver) list;
>>>   };
>>>
>>>
>>
>>
> 
> 



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

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

[Eric Blake]

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

On 03/05/2016 11:13 AM, Max Reitz wrote:

>> +    index = atoi(child->name + 9);
> 
> Optional: Assert absence of an error:
> 

Indeed, atoi() is worthless, because it cannot do error detection.

> unsigned long index;
> char *endptr;
> 
> index = strtoul(child->name + 9, &endptr, 10);
> assert(index >= 0 && !*endptr);

Still incorrect; you aren't handling errno properly for detecting all
errors.  Even better is to use qemu_strtoul(), which already handles
proper error detection.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


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

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

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 647 bytes --]

On 07.03.2016 17:02, Eric Blake wrote:
> On 03/05/2016 11:13 AM, Max Reitz wrote:
> 
>>> +    index = atoi(child->name + 9);
>>
>> Optional: Assert absence of an error:
>>
> 
> Indeed, atoi() is worthless, because it cannot do error detection.
> 
>> unsigned long index;
>> char *endptr;
>>
>> index = strtoul(child->name + 9, &endptr, 10);
>> assert(index >= 0 && !*endptr);
> 
> Still incorrect; you aren't handling errno properly for detecting all
> errors.  Even better is to use qemu_strtoul(), which already handles
> proper error detection.

Yeah, I keep forgetting that it returns ULONG_MAX on range error...

Max


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

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

[Changlong Xie]

On 03/08/2016 12:02 AM, Eric Blake wrote:
> On 03/05/2016 11:13 AM, Max Reitz wrote:
>
>>> +    index = atoi(child->name + 9);
>>
>> Optional: Assert absence of an error:
>>
>
> Indeed, atoi() is worthless, because it cannot do error detection.
>
>> unsigned long index;
>> char *endptr;
>>
>> index = strtoul(child->name + 9, &endptr, 10);
>> assert(index >= 0 && !*endptr);
>
> Still incorrect; you aren't handling errno properly for detecting all
> errors.  Even better is to use qemu_strtoul(), which already handles
> proper error detection.
>

Will fix this in next version, thanks for pointing it out.

Thanks
	-Xie

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

[Changlong Xie]

On 03/08/2016 12:02 AM, Max Reitz wrote:
> On 07.03.2016 17:02, Eric Blake wrote:
>> On 03/05/2016 11:13 AM, Max Reitz wrote:
>>
>>>> +    index = atoi(child->name + 9);
>>>
>>> Optional: Assert absence of an error:
>>>
>>
>> Indeed, atoi() is worthless, because it cannot do error detection.
>>
>>> unsigned long index;
>>> char *endptr;
>>>
>>> index = strtoul(child->name + 9, &endptr, 10);
>>> assert(index >= 0 && !*endptr);
>>
>> Still incorrect; you aren't handling errno properly for detecting all
>> errors.  Even better is to use qemu_strtoul(), which already handles
>> proper error detection.
>
> Yeah, I keep forgetting that it returns ULONG_MAX on range error...

Yes, we should limit the range to INT_MAX. How do you like the following 
codes, i just steal it from xen_host_pci_get_value().

int rc;
const char *endptr;
unsigned long value;

assert(!strncmp(child->name, "children.", 9));
rc = qemu_strtoul(child->name + 9, &endptr, 10, &value);
if (!rc) {
     assert(value <= INT_MAX);
     index = value;
} else {
     error_setg_errno(errp, -rc, "Failed to parse value '%s'",
                      child->name + 9);
     return;
}

Thanks
	-Xie

>
> Max
>

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

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 1923 bytes --]

On 08.03.2016 03:57, Changlong Xie wrote:
> On 03/08/2016 12:02 AM, Max Reitz wrote:
>> On 07.03.2016 17:02, Eric Blake wrote:
>>> On 03/05/2016 11:13 AM, Max Reitz wrote:
>>>
>>>>> +    index = atoi(child->name + 9);
>>>>
>>>> Optional: Assert absence of an error:
>>>>
>>>
>>> Indeed, atoi() is worthless, because it cannot do error detection.
>>>
>>>> unsigned long index;
>>>> char *endptr;
>>>>
>>>> index = strtoul(child->name + 9, &endptr, 10);
>>>> assert(index >= 0 && !*endptr);
>>>
>>> Still incorrect; you aren't handling errno properly for detecting all
>>> errors.  Even better is to use qemu_strtoul(), which already handles
>>> proper error detection.
>>
>> Yeah, I keep forgetting that it returns ULONG_MAX on range error...
> 
> Yes, we should limit the range to INT_MAX. How do you like the following
> codes, i just steal it from xen_host_pci_get_value().
> 
> int rc;
> const char *endptr;
> unsigned long value;
> 
> assert(!strncmp(child->name, "children.", 9));
> rc = qemu_strtoul(child->name + 9, &endptr, 10, &value);

Passing NULL instead of &endptr will make qemu_strtoul() check that the
string passed to it (child->name + 9) only consists of a number; which
should be true here, so you can do that (pass NULL instead of &endptr).

> if (!rc) {
>     assert(value <= INT_MAX);
>     index = value;
> } else {
>     error_setg_errno(errp, -rc, "Failed to parse value '%s'",
>                      child->name + 9);
>     return;
> }

You could simplify this as

assert(!rc && value <= INT_MAX);
index = value;

(It should be impossible for qemu_strtoul() to return an error here, so
an assert() is just as fine as a normal error.)

And you could get rid of the index = value assignment by making index an
unsigned long and replacing all instances of "value" by "index".

Max

> 
> Thanks
>     -Xie
> 
>>
>> Max
>>
> 
> 



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