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

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

[Changlong Xie]

ChangLog:
v13:
1. Rebase to the newest codes
2. Address commets from Betro and Max
p1. Add R-B, fix incorrect syntax
p2. Add missing "qemu/cutils.h" since 2.6, and rewrite quorum_add/del_child
p3. Remove unnecessary "id", add "since 2.7"
v11~v12:
1. Address comments from Max
p1. Add R-B
p2. Add R-B, remove unnecessary "endptr" "value"
p3. Add R-B
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            | 78 +++++++++++++++++++++++++++++++++++++++++++++--
 blockdev.c                | 55 +++++++++++++++++++++++++++++++++
 include/block/block.h     |  8 +++++
 include/block/block_int.h |  5 +++
 qapi/block-core.json      | 32 +++++++++++++++++++
 qmp-commands.hx           | 53 ++++++++++++++++++++++++++++++++
 7 files changed, 282 insertions(+), 6 deletions(-)

-- 
1.9.3

[Qemu-devel] [PATCH v13 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: Max Reitz <mreitz@redhat.com>
Reviewed-by: Alberto Garcia <berto@igalia.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 d4939b4..68cd3b2 100644
--- a/block.c
+++ b/block.c
@@ -3981,3 +3981,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 a 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 3a73137..694ca76 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -541,4 +541,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 10d8759..7e238a0 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -317,6 +317,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 v13 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        | 78 +++++++++++++++++++++++++++++++++++++++++++++++++--
 include/block/block.h |  4 +++
 3 files changed, 84 insertions(+), 6 deletions(-)

diff --git a/block.c b/block.c
index 68cd3b2..4bdc6b3 100644
--- a/block.c
+++ b/block.c
@@ -1176,10 +1176,10 @@ BdrvChild *bdrv_root_attach_child(BlockDriverState *child_bs,
     return child;
 }
 
-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 = bdrv_root_attach_child(child_bs, child_name, child_role);
     QLIST_INSERT_HEAD(&parent_bs->children, child, next);
diff --git a/block/quorum.c b/block/quorum.c
index da15465..2553f82 100644
--- a/block/quorum.c
+++ b/block/quorum.c
@@ -14,6 +14,7 @@
  */
 
 #include "qemu/osdep.h"
+#include "qemu/cutils.h"
 #include "block/block_int.h"
 #include "qapi/qmp/qbool.h"
 #include "qapi/qmp/qdict.h"
@@ -67,6 +68,9 @@ typedef struct QuorumVotes {
 typedef struct BDRVQuorumState {
     BdrvChild **children;  /* children BlockDriverStates */
     int num_children;      /* children count */
+    uint64_t last_index;   /* indicate the child role name of the last
+                            * element of children array
+                            */
     int threshold;         /* if less than threshold children reads gave the
                             * same result a quorum error occurs.
                             */
@@ -898,9 +902,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;
     }
@@ -964,6 +968,7 @@ static int quorum_open(BlockDriverState *bs, QDict *options, int flags,
 
         opened[i] = true;
     }
+    s->last_index = s->num_children;
 
     g_free(opened);
     goto exit;
@@ -1020,6 +1025,72 @@ static void quorum_attach_aio_context(BlockDriverState *bs,
     }
 }
 
+static void quorum_add_child(BlockDriverState *bs, BlockDriverState *child_bs,
+                             Error **errp)
+{
+    BDRVQuorumState *s = bs->opaque;
+    BdrvChild *child;
+    char indexstr[32];
+    int ret;
+
+    assert(s->num_children <= INT_MAX / sizeof(BdrvChild *) &&
+           s->last_index <= UINT64_MAX);
+    if (s->num_children == INT_MAX / sizeof(BdrvChild *) ||
+        s->last_index == UINT64_MAX) {
+        error_setg(errp, "Too many children");
+        return;
+    }
+
+    ret = snprintf(indexstr, 32, "children.%" PRIu64, s->last_index);
+    if (ret < 0 || ret >= 32) {
+        error_setg(errp, "cannot generate child name");
+        return;
+    }
+    s->last_index++;
+
+    bdrv_drain(bs);
+
+    /* We can safely add the child now */
+    bdrv_ref(child_bs);
+    child = bdrv_attach_child(bs, child_bs, indexstr, &child_format);
+    s->children = g_renew(BdrvChild *, s->children, s->num_children + 1);
+    s->children[s->num_children++] = child;
+}
+
+static void quorum_del_child(BlockDriverState *bs, BdrvChild *child,
+                             Error **errp)
+{
+    BDRVQuorumState *s = bs->opaque;
+    int i;
+
+    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));
+
+    bdrv_drain(bs);
+
+    /* We can safely remove this child now */
+    memmove(&s->children[i], &s->children[i + 1],
+            (s->num_children - i - 1) * sizeof(BdrvChild *));
+    s->children = g_renew(BdrvChild *, s->children, --s->num_children);
+    bdrv_unref_child(bs, child);
+}
+
 static void quorum_refresh_filename(BlockDriverState *bs, QDict *options)
 {
     BDRVQuorumState *s = bs->opaque;
@@ -1075,6 +1146,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 694ca76..52902cd 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -476,6 +476,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 v13 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>
Reviewed-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
---
 blockdev.c           | 55 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 qapi/block-core.json | 32 ++++++++++++++++++++++++++++++
 qmp-commands.hx      | 53 ++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 140 insertions(+)

diff --git a/blockdev.c b/blockdev.c
index f1f520a..d16d449 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -4092,6 +4092,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 1d09079..98a20d2 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2556,3 +2556,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.7
+##
+{ 'command': 'x-blockdev-change',
+  'data' : { 'parent': 'str',
+             '*child': 'str',
+             '*node': 'str' } }
diff --git a/qmp-commands.hx b/qmp-commands.hx
index de896a5..88f781a 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -4398,6 +4398,59 @@ 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",
+                                "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 v13 2/3] quorum: implement bdrv_add_child() and bdrv_del_child()

[Changlong Xie]

ping...

On 04/13/2016 04:33 PM, 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        | 78 +++++++++++++++++++++++++++++++++++++++++++++++++--
>   include/block/block.h |  4 +++
>   3 files changed, 84 insertions(+), 6 deletions(-)
>
> diff --git a/block.c b/block.c
> index 68cd3b2..4bdc6b3 100644
> --- a/block.c
> +++ b/block.c
> @@ -1176,10 +1176,10 @@ BdrvChild *bdrv_root_attach_child(BlockDriverState *child_bs,
>       return child;
>   }
>
> -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 = bdrv_root_attach_child(child_bs, child_name, child_role);
>       QLIST_INSERT_HEAD(&parent_bs->children, child, next);
> diff --git a/block/quorum.c b/block/quorum.c
> index da15465..2553f82 100644
> --- a/block/quorum.c
> +++ b/block/quorum.c
> @@ -14,6 +14,7 @@
>    */
>
>   #include "qemu/osdep.h"
> +#include "qemu/cutils.h"
>   #include "block/block_int.h"
>   #include "qapi/qmp/qbool.h"
>   #include "qapi/qmp/qdict.h"
> @@ -67,6 +68,9 @@ typedef struct QuorumVotes {
>   typedef struct BDRVQuorumState {
>       BdrvChild **children;  /* children BlockDriverStates */
>       int num_children;      /* children count */
> +    uint64_t last_index;   /* indicate the child role name of the last
> +                            * element of children array
> +                            */
>       int threshold;         /* if less than threshold children reads gave the
>                               * same result a quorum error occurs.
>                               */
> @@ -898,9 +902,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;
>       }
> @@ -964,6 +968,7 @@ static int quorum_open(BlockDriverState *bs, QDict *options, int flags,
>
>           opened[i] = true;
>       }
> +    s->last_index = s->num_children;
>
>       g_free(opened);
>       goto exit;
> @@ -1020,6 +1025,72 @@ static void quorum_attach_aio_context(BlockDriverState *bs,
>       }
>   }
>
> +static void quorum_add_child(BlockDriverState *bs, BlockDriverState *child_bs,
> +                             Error **errp)
> +{
> +    BDRVQuorumState *s = bs->opaque;
> +    BdrvChild *child;
> +    char indexstr[32];
> +    int ret;
> +
> +    assert(s->num_children <= INT_MAX / sizeof(BdrvChild *) &&
> +           s->last_index <= UINT64_MAX);
> +    if (s->num_children == INT_MAX / sizeof(BdrvChild *) ||
> +        s->last_index == UINT64_MAX) {
> +        error_setg(errp, "Too many children");
> +        return;
> +    }
> +
> +    ret = snprintf(indexstr, 32, "children.%" PRIu64, s->last_index);
> +    if (ret < 0 || ret >= 32) {
> +        error_setg(errp, "cannot generate child name");
> +        return;
> +    }
> +    s->last_index++;
> +
> +    bdrv_drain(bs);
> +
> +    /* We can safely add the child now */
> +    bdrv_ref(child_bs);
> +    child = bdrv_attach_child(bs, child_bs, indexstr, &child_format);
> +    s->children = g_renew(BdrvChild *, s->children, s->num_children + 1);
> +    s->children[s->num_children++] = child;
> +}
> +
> +static void quorum_del_child(BlockDriverState *bs, BdrvChild *child,
> +                             Error **errp)
> +{
> +    BDRVQuorumState *s = bs->opaque;
> +    int i;
> +
> +    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));
> +
> +    bdrv_drain(bs);
> +
> +    /* We can safely remove this child now */
> +    memmove(&s->children[i], &s->children[i + 1],
> +            (s->num_children - i - 1) * sizeof(BdrvChild *));
> +    s->children = g_renew(BdrvChild *, s->children, --s->num_children);
> +    bdrv_unref_child(bs, child);
> +}
> +
>   static void quorum_refresh_filename(BlockDriverState *bs, QDict *options)
>   {
>       BDRVQuorumState *s = bs->opaque;
> @@ -1075,6 +1146,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 694ca76..52902cd 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -476,6 +476,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 v13 2/3] quorum: implement bdrv_add_child() and bdrv_del_child()

[Max Reitz]

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

On 13.04.2016 10:33, 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        | 78 +++++++++++++++++++++++++++++++++++++++++++++++++--
>  include/block/block.h |  4 +++
>  3 files changed, 84 insertions(+), 6 deletions(-)

Design-wise: Nice change. It's a bit strange now to have gaps in the
child naming, but this strategy is very simple to implement and the
order of the children used for FIFO is the same as the numerical order
of the indices in the child names, so I'm happy.

> diff --git a/block.c b/block.c
> index 68cd3b2..4bdc6b3 100644
> --- a/block.c
> +++ b/block.c
> @@ -1176,10 +1176,10 @@ BdrvChild *bdrv_root_attach_child(BlockDriverState *child_bs,
>      return child;
>  }
>  
> -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 = bdrv_root_attach_child(child_bs, child_name, child_role);
>      QLIST_INSERT_HEAD(&parent_bs->children, child, next);
> diff --git a/block/quorum.c b/block/quorum.c
> index da15465..2553f82 100644
> --- a/block/quorum.c
> +++ b/block/quorum.c
> @@ -14,6 +14,7 @@
>   */
>  
>  #include "qemu/osdep.h"
> +#include "qemu/cutils.h"
>  #include "block/block_int.h"
>  #include "qapi/qmp/qbool.h"
>  #include "qapi/qmp/qdict.h"
> @@ -67,6 +68,9 @@ typedef struct QuorumVotes {
>  typedef struct BDRVQuorumState {
>      BdrvChild **children;  /* children BlockDriverStates */
>      int num_children;      /* children count */
> +    uint64_t last_index;   /* indicate the child role name of the last
> +                            * element of children array

The name (and the comment) is a bit misleading, as it's not the index of
the last child but one after that, i.e. the index of the next child
should it be added.

So maybe this variable should be called "next_child_index" or something
similar, and the comment should reflect that.

> +                            */
>      int threshold;         /* if less than threshold children reads gave the
>                              * same result a quorum error occurs.
>                              */
> @@ -898,9 +902,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;
>      }
> @@ -964,6 +968,7 @@ static int quorum_open(BlockDriverState *bs, QDict *options, int flags,
>  
>          opened[i] = true;
>      }
> +    s->last_index = s->num_children;
>  
>      g_free(opened);
>      goto exit;
> @@ -1020,6 +1025,72 @@ static void quorum_attach_aio_context(BlockDriverState *bs,
>      }
>  }
>  
> +static void quorum_add_child(BlockDriverState *bs, BlockDriverState *child_bs,
> +                             Error **errp)
> +{
> +    BDRVQuorumState *s = bs->opaque;
> +    BdrvChild *child;
> +    char indexstr[32];
> +    int ret;
> +
> +    assert(s->num_children <= INT_MAX / sizeof(BdrvChild *) &&
> +           s->last_index <= UINT64_MAX);
> +    if (s->num_children == INT_MAX / sizeof(BdrvChild *) ||
> +        s->last_index == UINT64_MAX) {
> +        error_setg(errp, "Too many children");
> +        return;
> +    }
> +
> +    ret = snprintf(indexstr, 32, "children.%" PRIu64, s->last_index);
> +    if (ret < 0 || ret >= 32) {
> +        error_setg(errp, "cannot generate child name");
> +        return;
> +    }
> +    s->last_index++;
> +
> +    bdrv_drain(bs);

We have bdrv_drained_begin() and bdrv_drained_end() now. Perhaps we
should make use of them and call bdrv_drained_begin() here...

> +
> +    /* We can safely add the child now */
> +    bdrv_ref(child_bs);
> +    child = bdrv_attach_child(bs, child_bs, indexstr, &child_format);
> +    s->children = g_renew(BdrvChild *, s->children, s->num_children + 1);
> +    s->children[s->num_children++] = child;

...and bdrv_drained_end() here.

> +}
> +
> +static void quorum_del_child(BlockDriverState *bs, BdrvChild *child,
> +                             Error **errp)
> +{
> +    BDRVQuorumState *s = bs->opaque;
> +    int i;
> +
> +    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));

This could be removed now, too. I was asking for these assertions (of
which in this version only this one is left) because we were using the
child name to infer the index in the child bitmap before.

Now we're not using the child name here at all, so we can just drop this
assertion.

> +
> +    bdrv_drain(bs);

As above, bdrv_drained_begin() here...

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

...and bdrv_drained_end() here may be better than a plain bdrv_drain().

> +}
> +
>  static void quorum_refresh_filename(BlockDriverState *bs, QDict *options)
>  {
>      BDRVQuorumState *s = bs->opaque;
> @@ -1075,6 +1146,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 694ca76..52902cd 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -476,6 +476,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);
> 

None of the things I pointed out above is critical, but I'd still rather
see them fixed before giving my R-b.

Max


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

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

[Changlong Xie]

On 05/06/2016 11:20 PM, Max Reitz wrote:
> On 13.04.2016 10:33, 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        | 78 +++++++++++++++++++++++++++++++++++++++++++++++++--
>>   include/block/block.h |  4 +++
>>   3 files changed, 84 insertions(+), 6 deletions(-)
>
> Design-wise: Nice change. It's a bit strange now to have gaps in the
> child naming, but this strategy is very simple to implement and the
> order of the children used for FIFO is the same as the numerical order
> of the indices in the child names, so I'm happy.

I think this is the simplest approach too : )

>
>> diff --git a/block.c b/block.c
>> index 68cd3b2..4bdc6b3 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -1176,10 +1176,10 @@ BdrvChild *bdrv_root_attach_child(BlockDriverState *child_bs,
>>       return child;
>>   }
>>
>> -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 = bdrv_root_attach_child(child_bs, child_name, child_role);
>>       QLIST_INSERT_HEAD(&parent_bs->children, child, next);
>> diff --git a/block/quorum.c b/block/quorum.c
>> index da15465..2553f82 100644
>> --- a/block/quorum.c
>> +++ b/block/quorum.c
>> @@ -14,6 +14,7 @@
>>    */
>>
>>   #include "qemu/osdep.h"
>> +#include "qemu/cutils.h"
>>   #include "block/block_int.h"
>>   #include "qapi/qmp/qbool.h"
>>   #include "qapi/qmp/qdict.h"
>> @@ -67,6 +68,9 @@ typedef struct QuorumVotes {
>>   typedef struct BDRVQuorumState {
>>       BdrvChild **children;  /* children BlockDriverStates */
>>       int num_children;      /* children count */
>> +    uint64_t last_index;   /* indicate the child role name of the last
>> +                            * element of children array
>
> The name (and the comment) is a bit misleading, as it's not the index of
> the last child but one after that, i.e. the index of the next child
> should it be added.
>
> So maybe this variable should be called "next_child_index" or something
> similar, and the comment should reflect that.
>

Will fix.

>> +                            */
>>       int threshold;         /* if less than threshold children reads gave the
>>                               * same result a quorum error occurs.
>>                               */
>> @@ -898,9 +902,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;
>>       }
>> @@ -964,6 +968,7 @@ static int quorum_open(BlockDriverState *bs, QDict *options, int flags,
>>
>>           opened[i] = true;
>>       }
>> +    s->last_index = s->num_children;
>>
>>       g_free(opened);
>>       goto exit;
>> @@ -1020,6 +1025,72 @@ static void quorum_attach_aio_context(BlockDriverState *bs,
>>       }
>>   }
>>
>> +static void quorum_add_child(BlockDriverState *bs, BlockDriverState *child_bs,
>> +                             Error **errp)
>> +{
>> +    BDRVQuorumState *s = bs->opaque;
>> +    BdrvChild *child;
>> +    char indexstr[32];
>> +    int ret;
>> +
>> +    assert(s->num_children <= INT_MAX / sizeof(BdrvChild *) &&
>> +           s->last_index <= UINT64_MAX);
>> +    if (s->num_children == INT_MAX / sizeof(BdrvChild *) ||
>> +        s->last_index == UINT64_MAX) {
>> +        error_setg(errp, "Too many children");
>> +        return;
>> +    }
>> +
>> +    ret = snprintf(indexstr, 32, "children.%" PRIu64, s->last_index);
>> +    if (ret < 0 || ret >= 32) {
>> +        error_setg(errp, "cannot generate child name");
>> +        return;
>> +    }
>> +    s->last_index++;
>> +
>> +    bdrv_drain(bs);
>
> We have bdrv_drained_begin() and bdrv_drained_end() now. Perhaps we
> should make use of them and call bdrv_drained_begin() here...

Ditto

>
>> +
>> +    /* We can safely add the child now */
>> +    bdrv_ref(child_bs);
>> +    child = bdrv_attach_child(bs, child_bs, indexstr, &child_format);
>> +    s->children = g_renew(BdrvChild *, s->children, s->num_children + 1);
>> +    s->children[s->num_children++] = child;
>
> ...and bdrv_drained_end() here.

Ditto

>
>> +}
>> +
>> +static void quorum_del_child(BlockDriverState *bs, BdrvChild *child,
>> +                             Error **errp)
>> +{
>> +    BDRVQuorumState *s = bs->opaque;
>> +    int i;
>> +
>> +    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));
>
> This could be removed now, too. I was asking for these assertions (of
> which in this version only this one is left) because we were using the
> child name to infer the index in the child bitmap before.
>
> Now we're not using the child name here at all, so we can just drop this
> assertion.

Will remove it in next version.

>
>> +
>> +    bdrv_drain(bs);
>
> As above, bdrv_drained_begin() here...

Will fix it.

>
>> +
>> +    /* We can safely remove this child now */
>> +    memmove(&s->children[i], &s->children[i + 1],
>> +            (s->num_children - i - 1) * sizeof(BdrvChild *));
>> +    s->children = g_renew(BdrvChild *, s->children, --s->num_children);
>> +    bdrv_unref_child(bs, child);
>
> ...and bdrv_drained_end() here may be better than a plain bdrv_drain().
>

Ditto.

>> +}
>> +
>>   static void quorum_refresh_filename(BlockDriverState *bs, QDict *options)
>>   {
>>       BDRVQuorumState *s = bs->opaque;
>> @@ -1075,6 +1146,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 694ca76..52902cd 100644
>> --- a/include/block/block.h
>> +++ b/include/block/block.h
>> @@ -476,6 +476,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);
>>
>
> None of the things I pointed out above is critical, but I'd still rather
> see them fixed before giving my R-b.

I'm glab to hear that, will send out next version.

Thanks
	-Xie
>
> Max
>

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

[Alberto Garcia]

On Wed 13 Apr 2016 10:33:08 AM CEST, Changlong Xie wrote:

Sorry for the late reply!

The patch looks good, I have some additional comments on top of what Max
Wrote, nothing serious though :)

> @@ -67,6 +68,9 @@ typedef struct QuorumVotes {
>  typedef struct BDRVQuorumState {
>      BdrvChild **children;  /* children BlockDriverStates */
>      int num_children;      /* children count */
> +    uint64_t last_index;   /* indicate the child role name of the last
> +                            * element of children array
> +                            */

I think you can use a regular 'unsigned' here, it's simpler and more
efficient. We're not going to have 2^32 Quorum children, are we? :)

> +static void quorum_add_child(BlockDriverState *bs, BlockDriverState *child_bs,
> +                             Error **errp)
> +{
> +    BDRVQuorumState *s = bs->opaque;
> +    BdrvChild *child;
> +    char indexstr[32];
> +    int ret;
> +
> +    assert(s->num_children <= INT_MAX / sizeof(BdrvChild *) &&
> +           s->last_index <= UINT64_MAX);

That last condition has no practical effect. last_index is a uint64_t so
s->last_index <= UINT64_MAX is always going to be true.

> +    s->children = g_renew(BdrvChild *, s->children, s->num_children + 1);
> +    s->children[s->num_children++] = child;

Slightly simpler way:

       s->children = g_renew(BdrvChild *, s->children, ++s->num_children);
       s->children[s->num_children] = child;

But this is not very important, so you can leave it as it is now if you
prefer.

> +    /* child->name is "children.%d" */
> +    assert(!strncmp(child->name, "children.", 9));

I actually don't think there's anything wrong with this assertion, but
if you decide to keep it you can use g_str_has_prefix() instead, which
is a bit easier and more readable.

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

Question: do we want to decrement last_index if 'i' is the last
children? Something like:

if (i == s->num_children - 1) {
   s->last_index--;
} else {
   memmove(...)
}
s->children = g_renew(...)

Berto

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

[Max Reitz]

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

On 09.05.2016 17:52, Alberto Garcia wrote:
> On Wed 13 Apr 2016 10:33:08 AM CEST, Changlong Xie wrote:

[...]

>> +    /* We can safely remove this child now */
>> +    memmove(&s->children[i], &s->children[i + 1],
>> +            (s->num_children - i - 1) * sizeof(BdrvChild *));
>> +    s->children = g_renew(BdrvChild *, s->children, --s->num_children);
>> +    bdrv_unref_child(bs, child);
> 
> Question: do we want to decrement last_index if 'i' is the last
> children? Something like:

I think it's better to keep it consistent. It probably wouldn't hurt to
do this, but I don't see any real benefit and on the other hand it looks
a bit strange (to the user) to special-case replacement of the last child.

Max

> if (i == s->num_children - 1) {
>    s->last_index--;
> } else {
>    memmove(...)
> }
> s->children = g_renew(...)
> 
> Berto
> 



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

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

[Changlong Xie]

On 05/09/2016 11:52 PM, Alberto Garcia wrote:
> On Wed 13 Apr 2016 10:33:08 AM CEST, Changlong Xie wrote:
>
> Sorry for the late reply!
>

Never mind : )

> The patch looks good, I have some additional comments on top of what Max
> Wrote, nothing serious though :)
>
>> @@ -67,6 +68,9 @@ typedef struct QuorumVotes {
>>   typedef struct BDRVQuorumState {
>>       BdrvChild **children;  /* children BlockDriverStates */
>>       int num_children;      /* children count */
>> +    uint64_t last_index;   /* indicate the child role name of the last
>> +                            * element of children array
>> +                            */
>
> I think you can use a regular 'unsigned' here, it's simpler and more
> efficient. We're not going to have 2^32 Quorum children, are we? :)
>

Actually, i tried to use 'unsinged' here in my first version. But 
thinking of if someone did crazy child add/delete test(add 10 children 
per second), it will overflow in about 2^32/(60*60*24*365*10) = 13 
years, so i choiced uint64_t(2^64 is big enough) here.
Now, i argree with you, it's overwrought. Will use 'unsigned' in next 
version.

>> +static void quorum_add_child(BlockDriverState *bs, BlockDriverState *child_bs,
>> +                             Error **errp)
>> +{
>> +    BDRVQuorumState *s = bs->opaque;
>> +    BdrvChild *child;
>> +    char indexstr[32];
>> +    int ret;
>> +
>> +    assert(s->num_children <= INT_MAX / sizeof(BdrvChild *) &&
>> +           s->last_index <= UINT64_MAX);
>
> That last condition has no practical effect. last_index is a uint64_t so
> s->last_index <= UINT64_MAX is always going to be true.

Yes, it's redundant code.

>
>> +    s->children = g_renew(BdrvChild *, s->children, s->num_children + 1);
>> +    s->children[s->num_children++] = child;
>
> Slightly simpler way:
>
>         s->children = g_renew(BdrvChild *, s->children, ++s->num_children);
>         s->children[s->num_children] = child;

Overflow arrays, should (s->num_children - 1) here. I'll keep my 
original one.

>
> But this is not very important, so you can leave it as it is now if you
> prefer.
>
>> +    /* child->name is "children.%d" */
>> +    assert(!strncmp(child->name, "children.", 9));
>
> I actually don't think there's anything wrong with this assertion, but
> if you decide to keep it you can use g_str_has_prefix() instead, which
> is a bit easier and more readable.
>

Just as Max said, it's extra check, and will remove it.

>> +    /* We can safely remove this child now */
>> +    memmove(&s->children[i], &s->children[i + 1],
>> +            (s->num_children - i - 1) * sizeof(BdrvChild *));
>> +    s->children = g_renew(BdrvChild *, s->children, --s->num_children);
>> +    bdrv_unref_child(bs, child);
>
> Question: do we want to decrement last_index if 'i' is the last
> children? Something like:
>

I agree with Max, it seems no benifit(although will save number 
resources if (i == s->num_children - 1)) here.

Thanks
	-Xie

> if (i == s->num_children - 1) {
>     s->last_index--;
> } else {
>     memmove(...)
> }
> s->children = g_renew(...)
>
> Berto
>
>
> .
>

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

[Kevin Wolf]

Am 09.05.2016 um 17:52 hat Alberto Garcia geschrieben:
> On Wed 13 Apr 2016 10:33:08 AM CEST, Changlong Xie wrote:
> 
> Sorry for the late reply!
> 
> The patch looks good, I have some additional comments on top of what Max
> Wrote, nothing serious though :)
> 
> > @@ -67,6 +68,9 @@ typedef struct QuorumVotes {
> >  typedef struct BDRVQuorumState {
> >      BdrvChild **children;  /* children BlockDriverStates */
> >      int num_children;      /* children count */
> > +    uint64_t last_index;   /* indicate the child role name of the last
> > +                            * element of children array
> > +                            */
> 
> I think you can use a regular 'unsigned' here, it's simpler and more
> efficient. We're not going to have 2^32 Quorum children, are we? :)
> 
> > +static void quorum_add_child(BlockDriverState *bs, BlockDriverState *child_bs,
> > +                             Error **errp)
> > +{
> > +    BDRVQuorumState *s = bs->opaque;
> > +    BdrvChild *child;
> > +    char indexstr[32];
> > +    int ret;
> > +
> > +    assert(s->num_children <= INT_MAX / sizeof(BdrvChild *) &&
> > +           s->last_index <= UINT64_MAX);
> 
> That last condition has no practical effect. last_index is a uint64_t so
> s->last_index <= UINT64_MAX is always going to be true.
> 
> > +    s->children = g_renew(BdrvChild *, s->children, s->num_children + 1);
> > +    s->children[s->num_children++] = child;
> 
> Slightly simpler way:
> 
>        s->children = g_renew(BdrvChild *, s->children, ++s->num_children);
>        s->children[s->num_children] = child;

Without having checked the context, this code is not equivalent. You
need to access s->children[s->num_children - 1] now in the second line.

> But this is not very important, so you can leave it as it is now if you
> prefer.
> 
> > +    /* child->name is "children.%d" */
> > +    assert(!strncmp(child->name, "children.", 9));
> 
> I actually don't think there's anything wrong with this assertion, but
> if you decide to keep it you can use g_str_has_prefix() instead, which
> is a bit easier and more readable.

There's also good old strstart() from cutils.c.

Kevin

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

[Alberto Garcia]

On Tue 10 May 2016 10:39:21 AM CEST, Kevin Wolf wrote:
>>        s->children = g_renew(BdrvChild *, s->children, ++s->num_children);
>>        s->children[s->num_children] = child;
>
> Without having checked the context, this code is not equivalent. You
> need to access s->children[s->num_children - 1] now in the second
> line.

Yeah, you are both right, sorry for the mistake!

Berto