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

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

[Changlong Xie]

ChangLog:
v10~v11:
1. Rebase to the newest codes
2. Address comment from Max
Don't use contractions in error messages,
p1: Remove R-Bs, and use "BdrvChild *child" in bdrv_del_child
p2: Fix error logic in get_new_child_index/remove_child_index, and prefect
child->name parsing
p3: Make bdrv_find_child return BdrvChild *, and add missing explanation

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                   |  57 +++++++++++++++++++--
 block/quorum.c            | 123 +++++++++++++++++++++++++++++++++++++++++++++-
 blockdev.c                |  55 +++++++++++++++++++++
 include/block/block.h     |   8 +++
 include/block/block_int.h |   5 ++
 qapi/block-core.json      |  32 ++++++++++++
 qmp-commands.hx           |  54 ++++++++++++++++++++
 7 files changed, 328 insertions(+), 6 deletions(-)

-- 
1.9.3

[Qemu-devel] [PATCH v11 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>
---
 block.c                   | 49 +++++++++++++++++++++++++++++++++++++++++++++++
 include/block/block.h     |  4 ++++
 include/block/block_int.h |  5 +++++
 3 files changed, 58 insertions(+)

diff --git a/block.c b/block.c
index ba24b8e..d48f441 100644
--- a/block.c
+++ b/block.c
@@ -4395,3 +4395,52 @@ 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 does not 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, BdrvChild *child, Error **errp)
+{
+    BdrvChild *tmp;
+
+    if (!parent_bs->drv || !parent_bs->drv->bdrv_del_child) {
+        error_setg(errp, "The node %s does not support removing a child",
+                   bdrv_get_device_or_node_name(parent_bs));
+        return;
+    }
+
+    QLIST_FOREACH(tmp, &parent_bs->children, next) {
+        if (tmp == child) {
+            break;
+        }
+    }
+
+    if (!tmp) {
+        error_setg(errp, "The node %s does not have child named %s",
+                   bdrv_get_device_or_node_name(parent_bs),
+                   bdrv_get_device_or_node_name(child->bs));
+        return;
+    }
+
+    parent_bs->drv->bdrv_del_child(parent_bs, child, errp);
+}
diff --git a/include/block/block.h b/include/block/block.h
index 1c4f4d8..7378e74 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -582,4 +582,8 @@ 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, BdrvChild *child, Error **errp);
+
 #endif
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 9ef823a..704efe5 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, BdrvChild *child,
+                           Error **errp);
+
     QLIST_ENTRY(BlockDriver) list;
 };
 
-- 
1.9.3

[Qemu-devel] [PATCH v11 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        | 123 +++++++++++++++++++++++++++++++++++++++++++++++++-
 include/block/block.h |   4 ++
 3 files changed, 129 insertions(+), 6 deletions(-)

diff --git a/block.c b/block.c
index d48f441..66d21af 100644
--- a/block.c
+++ b/block.c
@@ -1194,10 +1194,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 11cc60b..469e4a3 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;
@@ -877,9 +880,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;
     }
@@ -928,6 +931,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];
@@ -943,6 +947,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;
@@ -999,6 +1005,116 @@ 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;
+    }
+
+    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, old_bsize;
+    size_t new_len;
+
+    assert(index < s->bsize);
+
+    clear_bit(index, s->index_bitmap);
+    if (index < s->bsize - 1) {
+        /* The last bit is always set */
+        return;
+    }
+
+    /* Clear last bit */
+    old_bsize = s->bsize;
+    last_index = find_last_bit(s->index_bitmap, s->bsize);
+    assert(last_index < old_bsize);
+    s->bsize = last_index + 1;
+
+    if (BITS_TO_LONGS(old_bsize) == BITS_TO_LONGS(s->bsize)) {
+        return;
+    }
+
+    new_len = BITS_TO_LONGS(s->bsize) * 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, BdrvChild *child,
+                             Error **errp)
+{
+    BDRVQuorumState *s = bs->opaque;
+    int i, index, rc;
+    const char *endptr;
+    unsigned long value;
+
+    for (i = 0; i < s->num_children; i++) {
+        if (s->children[i] == child) {
+            break;
+        }
+    }
+
+    /* we have checked it in bdrv_del_child() */
+    assert(i < s->num_children);
+
+    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" */
+    assert(!strncmp(child->name, "children.", 9));
+    rc = qemu_strtoul(child->name + 9, &endptr, 10, &value);
+    assert(!rc && value < INT_MAX / sizeof(BdrvChild *));
+    index = value;
+
+    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;
@@ -1054,6 +1170,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 7378e74..8a3966d 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 v11 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           | 55 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 qapi/block-core.json | 32 ++++++++++++++++++++++++++++++
 qmp-commands.hx      | 54 +++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 141 insertions(+)

diff --git a/blockdev.c b/blockdev.c
index 0f20c65..435631e 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -4017,6 +4017,61 @@ out:
     aio_context_release(aio_context);
 }
 
+static BdrvChild *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;
+        }
+    }
+
+    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, *new_bs = NULL;
+    BdrvChild *p_child;
+
+    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) {
+        p_child = bdrv_find_child(parent_bs, child);
+        if (!p_child) {
+            error_setg(errp, "Node '%s' does not have child '%s'",
+                       parent, child);
+            return;
+        }
+        bdrv_del_child(parent_bs, p_child, 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 9bf1b22..bc3fd0b 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2548,3 +2548,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 b629673..2a55135 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -4398,6 +4398,60 @@ 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.
+
+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.
+
+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 v11 1/3] Add new block driver interface to add/delete a BDS's child

[Max Reitz]

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

On 09.03.2016 04:51, 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>
> ---
>  block.c                   | 49 +++++++++++++++++++++++++++++++++++++++++++++++
>  include/block/block.h     |  4 ++++
>  include/block/block_int.h |  5 +++++
>  3 files changed, 58 insertions(+)

Reviewed-by: Max Reitz <mreitz@redhat.com>


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

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

[Max Reitz]

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

On 09.03.2016 04:51, 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        | 123 +++++++++++++++++++++++++++++++++++++++++++++++++-
>  include/block/block.h |   4 ++
>  3 files changed, 129 insertions(+), 6 deletions(-)
> 
> diff --git a/block.c b/block.c
> index d48f441..66d21af 100644
> --- a/block.c
> +++ b/block.c
> @@ -1194,10 +1194,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 11cc60b..469e4a3 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;
> @@ -877,9 +880,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;
>      }
> @@ -928,6 +931,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];
> @@ -943,6 +947,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;
> @@ -999,6 +1005,116 @@ 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;
> +    }
> +
> +    s->index_bitmap = bitmap_zero_extend(s->index_bitmap, s->bsize,
> +                                         s->bsize + 1);

If s->bsize == INT_MAX, then this will overflow to INT_MIN (probably).
This negative value will then be converted to a smaller negative value
by BITS_TO_LONGS() * sizeof(long) in bitmap_zero_extend(), and this
negative value will then be implicitly casted to a size_t value for the
g_realloc() call. On both 32 and 64 bit systems, allocating this will
probably fail due to insufficient memory which will then crash qemu.

One way to prevent this: Prevent the overflow in this function by
failing if s->bsize == INT_MAX before bitmap_zero_extend() is called.

Another way: Do not limit the number of children in quorum_add_child()
(and additionally in quorum_open()) to INT_MAX, but to something more
sane like 256 or 1024 or 65536 if you want to go really high (I can't
imagine anyone using more than 32 children). That way, s->bsize can
never grow to be INT_MAX in the first place.

In any case, qemu will probably crash long before this overflows because
trying to create 2G BDSs will definitely break something. This is why
I'd prefer the second approach (limiting the number of children to a
sane amount), and this is also why I don't actually care about this
overflow here:

In my opinion you don't need to change anything here. A follow-up patch
can take care of limiting the number of quorum children to a sane amount.

> +    return s->bsize++;
> +}
> +
> +static void remove_child_index(BDRVQuorumState *s, int index)
> +{
> +    int last_index, old_bsize;
> +    size_t new_len;
> +
> +    assert(index < s->bsize);
> +
> +    clear_bit(index, s->index_bitmap);
> +    if (index < s->bsize - 1) {
> +        /* The last bit is always set */
> +        return;
> +    }
> +
> +    /* Clear last bit */
> +    old_bsize = s->bsize;
> +    last_index = find_last_bit(s->index_bitmap, s->bsize);
> +    assert(last_index < old_bsize);
> +    s->bsize = last_index + 1;
> +
> +    if (BITS_TO_LONGS(old_bsize) == BITS_TO_LONGS(s->bsize)) {
> +        return;
> +    }
> +
> +    new_len = BITS_TO_LONGS(s->bsize) * 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, BdrvChild *child,
> +                             Error **errp)
> +{
> +    BDRVQuorumState *s = bs->opaque;
> +    int i, index, rc;
> +    const char *endptr;
> +    unsigned long value;
> +
> +    for (i = 0; i < s->num_children; i++) {
> +        if (s->children[i] == child) {
> +            break;
> +        }
> +    }
> +
> +    /* we have checked it in bdrv_del_child() */
> +    assert(i < s->num_children);
> +
> +    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" */
> +    assert(!strncmp(child->name, "children.", 9));
> +    rc = qemu_strtoul(child->name + 9, &endptr, 10, &value);

Should be NULL instead of &endptr. With that fixed (and the endptr
declaration removed):

Reviewed-by: Max Reitz <mreitz@redhat.com>

> +    assert(!rc && value < INT_MAX / sizeof(BdrvChild *));
> +    index = value;

Optional: Make index an unsigned long, replace all instances of "value"
by "index", and then you can drop this assignment.

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;
> @@ -1054,6 +1170,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 7378e74..8a3966d 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 v11 3/3] qmp: add monitor command to add/remove a child

[Max Reitz]

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

On 09.03.2016 04:51, 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           | 55 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  qapi/block-core.json | 32 ++++++++++++++++++++++++++++++
>  qmp-commands.hx      | 54 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 141 insertions(+)

Reviewed-by: Max Reitz <mreitz@redhat.com>


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

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

[Changlong Xie]

On 03/10/2016 02:11 AM, Max Reitz wrote:
> On 09.03.2016 04:51, 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        | 123 +++++++++++++++++++++++++++++++++++++++++++++++++-
>>   include/block/block.h |   4 ++
>>   3 files changed, 129 insertions(+), 6 deletions(-)
>>
>> diff --git a/block.c b/block.c
>> index d48f441..66d21af 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -1194,10 +1194,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 11cc60b..469e4a3 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;
>> @@ -877,9 +880,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;
>>       }
>> @@ -928,6 +931,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];
>> @@ -943,6 +947,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;
>> @@ -999,6 +1005,116 @@ 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;
>> +    }
>> +
>> +    s->index_bitmap = bitmap_zero_extend(s->index_bitmap, s->bsize,
>> +                                         s->bsize + 1);
>
> If s->bsize == INT_MAX, then this will overflow to INT_MIN (probably).
> This negative value will then be converted to a smaller negative value
> by BITS_TO_LONGS() * sizeof(long) in bitmap_zero_extend(), and this
> negative value will then be implicitly casted to a size_t value for the
> g_realloc() call. On both 32 and 64 bit systems, allocating this will
> probably fail due to insufficient memory which will then crash qemu.
>

It's a problem.

> One way to prevent this: Prevent the overflow in this function by
> failing if s->bsize == INT_MAX before bitmap_zero_extend() is called.
>
> Another way: Do not limit the number of children in quorum_add_child()
> (and additionally in quorum_open()) to INT_MAX, but to something more
> sane like 256 or 1024 or 65536 if you want to go really high (I can't
> imagine anyone using more than 32 children). That way, s->bsize can
> never grow to be INT_MAX in the first place.
>
> In any case, qemu will probably crash long before this overflows because
> trying to create 2G BDSs will definitely break something. This is why
> I'd prefer the second approach (limiting the number of children to a

Me too

> sane amount), and this is also why I don't actually care about this
> overflow here:
>
> In my opinion you don't need to change anything here. A follow-up patch
> can take care of limiting the number of quorum children to a sane amount.

Okay

>
>> +    return s->bsize++;
>> +}
>> +
>> +static void remove_child_index(BDRVQuorumState *s, int index)
>> +{
>> +    int last_index, old_bsize;
>> +    size_t new_len;
>> +
>> +    assert(index < s->bsize);
>> +
>> +    clear_bit(index, s->index_bitmap);
>> +    if (index < s->bsize - 1) {
>> +        /* The last bit is always set */
>> +        return;
>> +    }
>> +
>> +    /* Clear last bit */
>> +    old_bsize = s->bsize;
>> +    last_index = find_last_bit(s->index_bitmap, s->bsize);
>> +    assert(last_index < old_bsize);
>> +    s->bsize = last_index + 1;
>> +
>> +    if (BITS_TO_LONGS(old_bsize) == BITS_TO_LONGS(s->bsize)) {
>> +        return;
>> +    }
>> +
>> +    new_len = BITS_TO_LONGS(s->bsize) * 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, BdrvChild *child,
>> +                             Error **errp)
>> +{
>> +    BDRVQuorumState *s = bs->opaque;
>> +    int i, index, rc;
>> +    const char *endptr;
>> +    unsigned long value;
>> +
>> +    for (i = 0; i < s->num_children; i++) {
>> +        if (s->children[i] == child) {
>> +            break;
>> +        }
>> +    }
>> +
>> +    /* we have checked it in bdrv_del_child() */
>> +    assert(i < s->num_children);
>> +
>> +    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" */
>> +    assert(!strncmp(child->name, "children.", 9));
>> +    rc = qemu_strtoul(child->name + 9, &endptr, 10, &value);
>
> Should be NULL instead of &endptr. With that fixed (and the endptr
> declaration removed):
>

Will fix in next version.

> Reviewed-by: Max Reitz <mreitz@redhat.com>
>
>> +    assert(!rc && value < INT_MAX / sizeof(BdrvChild *));
>> +    index = value;
>
> Optional: Make index an unsigned long, replace all instances of "value"
> by "index", and then you can drop this assignment.

Yes, you're right.

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;
>> @@ -1054,6 +1170,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 7378e74..8a3966d 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);
>>
>
>