[Qemu-devel] [PATCH for-2.5 0/6] qapi: child add/delete support

[Qemu-devel] [PATCH for-2.5 0/6] qapi: child add/delete support

[Wen Congyang]

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

Wen Congyang (6):
  QAPI: move InetSocketAddress to qapi/common.json
  support nbd driver in blockdev-add
  Add new block driver interface to add/delete a BDS's child
  quorum: implement block driver interfaces add/delete a BDS's child
  qmp: add monitor command to add/remove a child
  hmp: add monitor command to add/remove a child

 block.c                   |  52 +++++++++++++++++++++
 block/quorum.c            |  74 +++++++++++++++++++++++++++++-
 blockdev.c                | 112 ++++++++++++++++++++++++++++++++++++++++++++++
 hmp-commands.hx           |  28 ++++++++++++
 include/block/block.h     |   4 ++
 include/block/block_int.h |   5 +++
 include/sysemu/blockdev.h |   2 +
 qapi-schema.json          |  27 -----------
 qapi/block-core.json      |  49 +++++++++++++++++++-
 qapi/common.json          |  27 +++++++++++
 qmp-commands.hx           |  67 +++++++++++++++++++++++++++
 11 files changed, 416 insertions(+), 31 deletions(-)

-- 
2.4.3

[Qemu-devel] [PATCH for-2.5 1/6] QAPI: move InetSocketAddress to qapi/common.json

[Wen Congyang]

It will be used by BlockdevOptionsNBD.

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>
---
 qapi-schema.json | 27 ---------------------------
 qapi/common.json | 27 +++++++++++++++++++++++++++
 2 files changed, 27 insertions(+), 27 deletions(-)

diff --git a/qapi-schema.json b/qapi-schema.json
index 4342a08..0ccb002 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -2537,33 +2537,6 @@
     'opts': 'NetClientOptions' } }
 
 ##
-# @InetSocketAddress
-#
-# Captures a socket address or address range in the Internet namespace.
-#
-# @host: host part of the address
-#
-# @port: port part of the address, or lowest port if @to is present
-#
-# @to: highest port to try
-#
-# @ipv4: whether to accept IPv4 addresses, default try both IPv4 and IPv6
-#        #optional
-#
-# @ipv6: whether to accept IPv6 addresses, default try both IPv4 and IPv6
-#        #optional
-#
-# Since 1.3
-##
-{ 'struct': 'InetSocketAddress',
-  'data': {
-    'host': 'str',
-    'port': 'str',
-    '*to': 'uint16',
-    '*ipv4': 'bool',
-    '*ipv6': 'bool' } }
-
-##
 # @UnixSocketAddress
 #
 # Captures a socket address in the local ("Unix socket") namespace.
diff --git a/qapi/common.json b/qapi/common.json
index bad56bf..11d039a 100644
--- a/qapi/common.json
+++ b/qapi/common.json
@@ -114,3 +114,30 @@
 ##
 { 'enum': 'OnOffAuto',
   'data': [ 'auto', 'on', 'off' ] }
+
+##
+# @InetSocketAddress
+#
+# Captures a socket address or address range in the Internet namespace.
+#
+# @host: host part of the address
+#
+# @port: port part of the address, or lowest port if @to is present
+#
+# @to: highest port to try
+#
+# @ipv4: whether to accept IPv4 addresses, default try both IPv4 and IPv6
+#        #optional
+#
+# @ipv6: whether to accept IPv6 addresses, default try both IPv4 and IPv6
+#        #optional
+#
+# Since 1.3
+##
+{ 'struct': 'InetSocketAddress',
+  'data': {
+    'host': 'str',
+    'port': 'str',
+    '*to': 'uint16',
+    '*ipv4': 'bool',
+    '*ipv6': 'bool' } }
-- 
2.4.3

[Qemu-devel] [PATCH for-2.5 2/6] support nbd driver in blockdev-add

[Wen Congyang]

Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
Signed-off-by: Gonglei <arei.gonglei@huawei.com>
---
 qapi/block-core.json | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 7b2efb8..3ed8114 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1383,7 +1383,7 @@
             'dmg', 'file', 'ftp', 'ftps', 'host_cdrom', 'host_device',
             'host_floppy', 'http', 'https', 'null-aio', 'null-co', 'parallels',
             'qcow', 'qcow2', 'qed', 'quorum', 'raw', 'tftp', 'vdi', 'vhdx',
-            'vmdk', 'vpc', 'vvfat' ] }
+            'vmdk', 'vpc', 'vvfat', 'nbd' ] }
 
 ##
 # @BlockdevOptionsBase
@@ -1789,6 +1789,19 @@
             '*read-pattern': 'QuorumReadPattern' } }
 
 ##
+# @BlockdevOptionsNBD
+#
+# Driver specific block device options for NBD
+#
+# @export: #options the NBD export name
+#
+# Since: 2.5
+##
+{ 'struct': 'BlockdevOptionsNBD',
+  'base': 'InetSocketAddress',
+  'data': { '*export': 'str' } }
+
+##
 # @BlockdevOptions
 #
 # Options for creating a block device.
@@ -1815,7 +1828,7 @@
       'http':       'BlockdevOptionsFile',
       'https':      'BlockdevOptionsFile',
 # TODO iscsi: Wait for structured options
-# TODO nbd: Should take InetSocketAddress for 'host'?
+      'nbd':        'BlockdevOptionsNBD',
 # TODO nfs: Wait for structured options
       'null-aio':   'BlockdevOptionsNull',
       'null-co':    'BlockdevOptionsNull',
-- 
2.4.3

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

[Wen Congyang]

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

Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
Signed-off-by: Gonglei <arei.gonglei@huawei.com>
---
 block.c                   | 52 +++++++++++++++++++++++++++++++++++++++++++++++
 include/block/block.h     |  4 ++++
 include/block/block_int.h |  5 +++++
 3 files changed, 61 insertions(+)

diff --git a/block.c b/block.c
index d088ee0..934a32d 100644
--- a/block.c
+++ b/block.c
@@ -4251,3 +4251,55 @@ BlockAcctStats *bdrv_get_stats(BlockDriverState *bs)
 {
     return &bs->stats;
 }
+
+static const char *bdrv_get_id_or_node_name(BlockDriverState *bs)
+{
+    if (bs->blk) {
+        return blk_name(bs->blk);
+    }
+
+    return bs->node_name ? bs->node_name : "";
+}
+
+/*
+ * 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 *bs, QDict *options, Error **errp)
+{
+
+    if (!bs->drv || !bs->drv->bdrv_add_child) {
+        error_setg(errp, "The BDS %s doesn't support adding a child",
+                   bdrv_get_id_or_node_name(bs));
+        return;
+    }
+
+    bs->drv->bdrv_add_child(bs, options, errp);
+}
+
+void bdrv_del_child(BlockDriverState *bs, BlockDriverState *child_bs,
+                    Error **errp)
+{
+    BdrvChild *child;
+
+    if (!bs->drv || !bs->drv->bdrv_del_child) {
+        error_setg(errp, "The BDS %s doesn't support removing a child",
+                   bdrv_get_id_or_node_name(bs));
+        return;
+    }
+
+    QLIST_FOREACH(child, &bs->children, next) {
+        if (child->bs == child_bs) {
+            break;
+        }
+    }
+
+    if (!child) {
+        error_setg(errp, "The BDS %s is not the BDS %s's child",
+                   bdrv_get_id_or_node_name(child_bs),
+                   bdrv_get_id_or_node_name(bs));
+        return;
+    }
+
+    bs->drv->bdrv_del_child(bs, child_bs, errp);
+}
diff --git a/include/block/block.h b/include/block/block.h
index 37916f7..4a03fb6 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -616,4 +616,8 @@ void bdrv_flush_io_queue(BlockDriverState *bs);
 
 BlockAcctStats *bdrv_get_stats(BlockDriverState *bs);
 
+void bdrv_add_child(BlockDriverState *bs, QDict *options, Error **errp);
+void bdrv_del_child(BlockDriverState *bs, BlockDriverState *child,
+                    Error **errp);
+
 #endif
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 14ad4c3..b6f2905 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -288,6 +288,11 @@ struct BlockDriver {
      */
     int (*bdrv_probe_geometry)(BlockDriverState *bs, HDGeometry *geo);
 
+    void (*bdrv_add_child)(BlockDriverState *bs, QDict *options,
+                           Error **errp);
+    void (*bdrv_del_child)(BlockDriverState *bs, BlockDriverState *child,
+                           Error **errp);
+
     QLIST_ENTRY(BlockDriver) list;
 };
 
-- 
2.4.3

[Qemu-devel] [PATCH for-2.5 4/6] quorum: implement block driver interfaces add/delete a BDS's child

[Wen Congyang]

Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
Signed-off-by: Gonglei <arei.gonglei@huawei.com>
Cc: Alberto Garcia <berto@igalia.com>
---
 block/quorum.c | 74 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 72 insertions(+), 2 deletions(-)

diff --git a/block/quorum.c b/block/quorum.c
index 4e66221..1c09265 100644
--- a/block/quorum.c
+++ b/block/quorum.c
@@ -66,6 +66,9 @@ typedef struct QuorumVotes {
 typedef struct BDRVQuorumState {
     BlockDriverState **bs; /* children BlockDriverStates */
     int num_children;      /* children count */
+    int max_children;      /* The maximum children count, we need to reallocate
+                            * bs if num_children will larger than maximum.
+                            */
     int threshold;         /* if less than threshold children reads gave the
                             * same result a quorum error occurs.
                             */
@@ -880,9 +883,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;
     }
@@ -931,6 +934,7 @@ static int quorum_open(BlockDriverState *bs, QDict *options, int flags,
     /* allocate the children BlockDriverState array */
     s->bs = g_new0(BlockDriverState *, s->num_children);
     opened = g_new0(bool, s->num_children);
+    s->max_children = s->num_children;
 
     for (i = 0; i < s->num_children; i++) {
         char indexstr[32];
@@ -1001,6 +1005,69 @@ static void quorum_attach_aio_context(BlockDriverState *bs,
     }
 }
 
+static void quorum_add_child(BlockDriverState *bs, QDict *options, Error **errp)
+{
+    BDRVQuorumState *s = bs->opaque;
+    int ret;
+    Error *local_err = NULL;
+
+    bdrv_drain(bs);
+
+    if (s->num_children == s->max_children) {
+        if (s->max_children >= INT_MAX) {
+            error_setg(errp, "Too many children");
+            return;
+        }
+
+        s->bs = g_renew(BlockDriverState *, s->bs, s->max_children + 1);
+        s->bs[s->num_children] = NULL;
+        s->max_children += 1;
+    }
+
+    ret = bdrv_open_image(&s->bs[s->num_children], NULL, options, "child", bs,
+                          &child_format, false, &local_err);
+    if (ret < 0) {
+        error_propagate(errp, local_err);
+        return;
+    }
+    s->num_children++;
+}
+
+static void quorum_del_child(BlockDriverState *bs, BlockDriverState *child_bs,
+                             Error **errp)
+{
+    BDRVQuorumState *s = bs->opaque;
+    int i;
+
+    for (i = 0; i < s->num_children; i++) {
+        if (s->bs[i] == child_bs) {
+            break;
+        }
+    }
+
+    if (i == s->num_children) {
+        error_setg(errp, "Invalid child");
+        return;
+    }
+
+    if (s->num_children <= s->threshold) {
+        error_setg(errp, "Cannot remove any more child");
+        return;
+    }
+
+    if (s->num_children == 1) {
+        error_setg(errp, "Cannot remove the last child");
+        return;
+    }
+
+    bdrv_drain(bs);
+    /* We can safe remove this child now */
+    memmove(&s->bs[i], &s->bs[i+1], (s->num_children - i - 1) * sizeof(void *));
+    s->num_children--;
+    s->bs[s->num_children] = NULL;
+    bdrv_unref(child_bs);
+}
+
 static void quorum_refresh_filename(BlockDriverState *bs)
 {
     BDRVQuorumState *s = bs->opaque;
@@ -1055,6 +1122,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,
 };
-- 
2.4.3

[Qemu-devel] [PATCH for-2.5 5/6] qmp: add monitor command to add/remove a child

[Wen Congyang]

Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
Signed-off-by: Gonglei <arei.gonglei@huawei.com>
---
 blockdev.c           | 79 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 qapi/block-core.json | 32 +++++++++++++++++++++
 qmp-commands.hx      | 67 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 178 insertions(+)

diff --git a/blockdev.c b/blockdev.c
index 62a4586..df40e92 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2186,6 +2186,23 @@ void hmp_drive_del(Monitor *mon, const QDict *qdict)
     aio_context_release(aio_context);
 }
 
+static void do_child_add(const char *device, QDict *opts, Error **errp)
+{
+    BlockDriverState *bs;
+    Error *local_err = NULL;
+
+    bs = bdrv_lookup_bs(device, device, &local_err);
+    if (!bs) {
+        error_propagate(errp, local_err);
+        return;
+    }
+
+    bdrv_add_child(bs, opts, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+    }
+}
+
 void qmp_block_resize(bool has_device, const char *device,
                       bool has_node_name, const char *node_name,
                       int64_t size, Error **errp)
@@ -3096,6 +3113,68 @@ fail:
     qmp_output_visitor_cleanup(ov);
 }
 
+void qmp_child_add(const char *device, BlockdevOptionsChild *options,
+                   Error **errp)
+{
+    QmpOutputVisitor *ov = qmp_output_visitor_new();
+    QObject *obj;
+    QDict *qdict;
+    Error *local_err = NULL;
+
+    if (options->child->has_id || options->child->has_discard ||
+        options->child->has_cache || options->child->has_aio ||
+        options->child->has_rerror || options->child->has_werror ||
+        options->child->has_read_only || options->child->has_detect_zeroes) {
+        error_setg(errp, "id, discard, cache, aio, rerror, werror, readonly"
+                   " and detect_zeroes cann't be used for child-add");
+        goto fail;
+    }
+
+    visit_type_BlockdevOptionsChild(qmp_output_get_visitor(ov),
+                                    &options, NULL, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        goto fail;
+    }
+
+    obj = qmp_output_get_qobject(ov);
+    qdict = qobject_to_qdict(obj);
+
+    qdict_flatten(qdict);
+
+    do_child_add(device, qdict, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        goto fail;
+    }
+
+fail:
+    qmp_output_visitor_cleanup(ov);
+}
+
+void qmp_child_del(const char *parent, const char *child, Error **errp)
+{
+    BlockDriverState *parent_bs, *child_bs;
+    Error *local_err = NULL;
+
+    parent_bs = bdrv_lookup_bs(parent, parent, &local_err);
+    if (!parent_bs) {
+        error_propagate(errp, local_err);
+        return;
+    }
+
+    child_bs = bdrv_lookup_bs(child, child, &local_err);
+    if (!child_bs) {
+        error_propagate(errp, local_err);
+        return;
+    }
+
+    bdrv_del_child(parent_bs, child_bs, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+    }
+}
+
 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 3ed8114..4d82944 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2122,3 +2122,35 @@
 ##
 { 'command': 'block-set-write-threshold',
   'data': { 'node-name': 'str', 'write-threshold': 'uint64' } }
+
+{
+  'struct': 'BlockdevOptionsChild',
+  'data': { 'child': 'BlockdevOptions'} }
+
+##
+# @child-add
+#
+# Add a new child to quorum. This is useful to fix a broken quorum child.
+#
+# @device: graph node name or id which the child will be added to.
+#
+# @options: the options to create child BDS.
+#
+# Since: 2.5
+##
+{ 'command': 'child-add',
+  'data' : { 'device': 'str', 'options': 'BlockdevOptionsChild' } }
+
+##
+# @child-del
+#
+# Remove a child from quorum. This is useful to fix a broken quorum child.
+#
+# @parent: graph node name or id from which the child will removed.
+#
+# @child: graph node name that will be removed.
+#
+# Since: 2.5
+##
+{ 'command': 'child-del',
+  'data' : { 'parent': 'str', 'child': 'str' } }
diff --git a/qmp-commands.hx b/qmp-commands.hx
index ba630b1..79a1146 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -3872,6 +3872,73 @@ Example (2):
 EQMP
 
     {
+        .name       = "child-add",
+        .args_type  = "device:B,options:q",
+        .mhandler.cmd_new = qmp_marshal_input_child_add,
+    },
+
+SQMP
+child-add
+------------
+
+Add a child to a quorum node.
+
+This command is still a work in progress. It doesn't support all
+block drivers. Stay away from it unless you want it to help with
+its development.
+
+Arguments:
+
+- "device": the quorum's id or node name
+- "options": the new child options
+
+Example:
+
+-> { "execute": "child-add",
+    "arguments": {
+        "device": "disk1",
+        "options" : {
+            "child": {
+                "driver": "qcow2",
+                "file": {
+                    "driver": "file",
+                    "filename": "test.qcow2"
+                },
+                "node-name": "new_node"
+            }
+        }
+    }
+<- { "return": {} }
+
+EQMP
+
+    {
+        .name        = "child-del",
+        .args_type   = "parent:B,child:B",
+        .mhandler.cmd_new = qmp_marshal_input_child_del,
+    },
+
+SQMP
+child-del
+------------
+
+Delete a child from a quorum node. It can be used to remove a broken
+quorum child.
+
+Arguments:
+
+- "parent": the quorum's id or node name
+- "child": the child node-name which will be removed
+
+Example:
+
+-> { "execute": "child-del",
+    "arguments": { "parent": "disk1", "child": "new_node" } }
+<- { "return": {} }
+
+EQMP
+
+    {
         .name       = "query-named-block-nodes",
         .args_type  = "",
         .mhandler.cmd_new = qmp_marshal_input_query_named_block_nodes,
-- 
2.4.3

[Qemu-devel] [PATCH for-2.5 6/6] hmp: add monitor command to add/remove a child

[Wen Congyang]

Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
Signed-off-by: Gonglei <arei.gonglei@huawei.com>
Cc: Luiz Capitulino <lcapitulino@redhat.com>
---
 blockdev.c                | 33 +++++++++++++++++++++++++++++++++
 hmp-commands.hx           | 28 ++++++++++++++++++++++++++++
 include/sysemu/blockdev.h |  2 ++
 3 files changed, 63 insertions(+)

diff --git a/blockdev.c b/blockdev.c
index df40e92..4d9dfd9 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2203,6 +2203,39 @@ static void do_child_add(const char *device, QDict *opts, Error **errp)
     }
 }
 
+void hmp_child_add(Monitor *mon, const QDict *qdict)
+{
+    const char *id = qdict_get_str(qdict, "id");
+    const char *optstr = qdict_get_str(qdict, "opts");
+    QemuOpts *opts;
+    QDict *bs_opts = qdict_new();
+    Error *local_err = NULL;
+
+    opts = drive_def(optstr);
+    if (!opts) {
+        /* We have reported error in drive_def */
+        return;
+    }
+
+    bs_opts = qemu_opts_to_qdict(opts, bs_opts);
+    do_child_add(id, bs_opts, &local_err);
+    if (local_err) {
+        error_report_err(local_err);
+    }
+}
+
+void hmp_child_del(Monitor *mon, const QDict *qdict)
+{
+    const char *id = qdict_get_str(qdict, "id");
+    const char *child_id = qdict_get_str(qdict, "child");
+    Error *local_err = NULL;
+
+    qmp_child_del(id, child_id, &local_err);
+    if (local_err) {
+        error_report_err(local_err);
+    }
+}
+
 void qmp_block_resize(bool has_device, const char *device,
                       bool has_node_name, const char *node_name,
                       int64_t size, Error **errp)
diff --git a/hmp-commands.hx b/hmp-commands.hx
index d3b7932..1d5b392 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -193,6 +193,34 @@ actions (drive options rerror, werror).
 ETEXI
 
     {
+        .name       = "child_add",
+        .args_type  = "id:B,opts:s",
+        .params     = "device child.file=file",
+        .help       = "add a child to a BDS",
+        .mhandler.cmd = hmp_child_add,
+    },
+
+STEXI
+@item child_add @var{device} @var{options}
+@findex child_add
+Add a child to the block device.
+ETEXI
+
+    {
+        .name       = "child_del",
+        .args_type  = "id:B,child:B",
+        .params     = "parent child",
+        .help       = "remove a child from a BDS",
+        .mhandler.cmd = hmp_child_del,
+    },
+
+STEXI
+@item child_del @var{parent device} @var{child device}
+@findex child_del
+Remove a child from the parent device.
+ETEXI
+
+    {
         .name       = "change",
         .args_type  = "device:B,target:F,arg:s?",
         .params     = "device filename [format]",
diff --git a/include/sysemu/blockdev.h b/include/sysemu/blockdev.h
index 3104150..594bfab 100644
--- a/include/sysemu/blockdev.h
+++ b/include/sysemu/blockdev.h
@@ -67,4 +67,6 @@ void qmp_change_blockdev(const char *device, const char *filename,
                          const char *format, Error **errp);
 void hmp_commit(Monitor *mon, const QDict *qdict);
 void hmp_drive_del(Monitor *mon, const QDict *qdict);
+void hmp_child_add(Monitor *mon, const QDict *qdict);
+void hmp_child_del(Monitor *mon, const QDict *qdict);
 #endif
-- 
2.4.3

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

[Alberto Garcia]

On Fri 31 Jul 2015 11:19:12 AM CEST, Wen Congyang wrote:

> +/*
> + * 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 *bs, QDict *options, Error **errp)
> +{
> +
> +    if (!bs->drv || !bs->drv->bdrv_add_child) {
> +        error_setg(errp, "The BDS %s doesn't support adding a child",
> +                   bdrv_get_id_or_node_name(bs));
> +        return;
> +    }
> +
> +    bs->drv->bdrv_add_child(bs, options, errp);
> +}
> +
> +void bdrv_del_child(BlockDriverState *bs, BlockDriverState *child_bs,
> +                    Error **errp)
> +{
> +    BdrvChild *child;
> +
> +    if (!bs->drv || !bs->drv->bdrv_del_child) {
> +        error_setg(errp, "The BDS %s doesn't support removing a child",
> +                   bdrv_get_id_or_node_name(bs));
> +        return;
> +    }
> +
> +    QLIST_FOREACH(child, &bs->children, next) {
> +        if (child->bs == child_bs) {
> +            break;
> +        }
> +    }
> +
> +    if (!child) {
> +        error_setg(errp, "The BDS %s is not the BDS %s's child",
> +                   bdrv_get_id_or_node_name(child_bs),
> +                   bdrv_get_id_or_node_name(bs));
> +        return;
> +    }
> +
> +    bs->drv->bdrv_del_child(bs, child_bs, errp);
> +}

I see that bdrv_del_child() checks that the child is in bs->children,
but who takes care of putting it there in the first place?

Berto

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

[Wen Congyang]

On 08/06/2015 10:33 PM, Alberto Garcia wrote:
> On Fri 31 Jul 2015 11:19:12 AM CEST, Wen Congyang wrote:
> 
>> +/*
>> + * 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 *bs, QDict *options, Error **errp)
>> +{
>> +
>> +    if (!bs->drv || !bs->drv->bdrv_add_child) {
>> +        error_setg(errp, "The BDS %s doesn't support adding a child",
>> +                   bdrv_get_id_or_node_name(bs));
>> +        return;
>> +    }
>> +
>> +    bs->drv->bdrv_add_child(bs, options, errp);
>> +}
>> +
>> +void bdrv_del_child(BlockDriverState *bs, BlockDriverState *child_bs,
>> +                    Error **errp)
>> +{
>> +    BdrvChild *child;
>> +
>> +    if (!bs->drv || !bs->drv->bdrv_del_child) {
>> +        error_setg(errp, "The BDS %s doesn't support removing a child",
>> +                   bdrv_get_id_or_node_name(bs));
>> +        return;
>> +    }
>> +
>> +    QLIST_FOREACH(child, &bs->children, next) {
>> +        if (child->bs == child_bs) {
>> +            break;
>> +        }
>> +    }
>> +
>> +    if (!child) {
>> +        error_setg(errp, "The BDS %s is not the BDS %s's child",
>> +                   bdrv_get_id_or_node_name(child_bs),
>> +                   bdrv_get_id_or_node_name(bs));
>> +        return;
>> +    }
>> +
>> +    bs->drv->bdrv_del_child(bs, child_bs, errp);
>> +}
> 
> I see that bdrv_del_child() checks that the child is in bs->children,
> but who takes care of putting it there in the first place?

The child is opened by the function bdrv_open_image().
bdrv_open_image()
    bdrv_open_child()
        bdrv_attach_child()

Thanks
Wen Congyang

> 
> Berto
> .
> 

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

[Alberto Garcia]

On Fri 07 Aug 2015 03:03:07 AM CEST, Wen Congyang wrote:

>>> +void bdrv_add_child(BlockDriverState *bs, QDict *options, Error **errp)
>>> +{
>>> +
>>> +    if (!bs->drv || !bs->drv->bdrv_add_child) {
>>> +        error_setg(errp, "The BDS %s doesn't support adding a child",
>>> +                   bdrv_get_id_or_node_name(bs));
>>> +        return;
>>> +    }
>>> +
>>> +    bs->drv->bdrv_add_child(bs, options, errp);
>>> +}
>>> +
>>> +void bdrv_del_child(BlockDriverState *bs, BlockDriverState *child_bs,
>>> +                    Error **errp)
>>> +{
>>> +    BdrvChild *child;
>>> +
>>> +    if (!bs->drv || !bs->drv->bdrv_del_child) {
>>> +        error_setg(errp, "The BDS %s doesn't support removing a child",
>>> +                   bdrv_get_id_or_node_name(bs));
>>> +        return;
>>> +    }
>>> +
>>> +    QLIST_FOREACH(child, &bs->children, next) {
>>> +        if (child->bs == child_bs) {
>>> +            break;
>>> +        }
>>> +    }
>>> +
>>> +    if (!child) {
>>> +        error_setg(errp, "The BDS %s is not the BDS %s's child",
>>> +                   bdrv_get_id_or_node_name(child_bs),
>>> +                   bdrv_get_id_or_node_name(bs));
>>> +        return;
>>> +    }
>>> +
>>> +    bs->drv->bdrv_del_child(bs, child_bs, errp);
>>> +}
>> 
>> I see that bdrv_del_child() checks that the child is in bs->children,
>> but who takes care of putting it there in the first place?
>
> The child is opened by the function bdrv_open_image().
> bdrv_open_image()
>     bdrv_open_child()
>         bdrv_attach_child()

I see, it's drv->bdrv_add_child() who takes care of opening the image,
this only provides the BlockdevOptions.

Thanks!

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

Berto

Re: [Qemu-devel] [PATCH for-2.5 4/6] quorum: implement block driver interfaces add/delete a BDS's child

[Alberto Garcia]

On Fri 31 Jul 2015 11:19:13 AM CEST, Wen Congyang wrote:
> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> Cc: Alberto Garcia <berto@igalia.com>
> ---
>  block/quorum.c | 74 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 72 insertions(+), 2 deletions(-)

The description is confusing, I suggest something like

"quorum: implement bdrv_add_child() and bdrv_del_child()"

> +    if (s->num_children <= s->threshold) {
> +        error_setg(errp, "Cannot remove any more child");
> +        return;
> +    }

See also my comments about this error message in one of my earlier
e-mails.

Otherwise it looks good to me, I saw that you added the bdrv_unref()
line, thanks!

Berto

Re: [Qemu-devel] [PATCH for-2.5 2/6] support nbd driver in blockdev-add

[Alberto Garcia]

On Fri 31 Jul 2015 11:19:11 AM CEST, Wen Congyang wrote:
> 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>
> ---
>  qapi/block-core.json | 17 +++++++++++++++--
>  1 file changed, 15 insertions(+), 2 deletions(-)

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

Berto

Re: [Qemu-devel] [PATCH for-2.5 1/6] QAPI: move InetSocketAddress to qapi/common.json

[Alberto Garcia]

On Fri 31 Jul 2015 11:19:10 AM CEST, Wen Congyang wrote:
> It will be used by BlockdevOptionsNBD.
>
> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
> Signed-off-by: Gonglei <arei.gonglei@huawei.com>

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

Berto

Re: [Qemu-devel] [PATCH for-2.5 5/6] qmp: add monitor command to add/remove a child

[Alberto Garcia]

On Fri 31 Jul 2015 11:19:14 AM CEST, Wen Congyang wrote:
> +##
> +# @child-add
> +#
> +# Add a new child to quorum. This is useful to fix a broken quorum
> child.

But the idea is that this can be eventually used by other drivers, isn't
it?

Berto

Re: [Qemu-devel] [PATCH for-2.5 5/6] qmp: add monitor command to add/remove a child

[Wen Congyang]

On 08/07/2015 09:12 PM, Alberto Garcia wrote:
> On Fri 31 Jul 2015 11:19:14 AM CEST, Wen Congyang wrote:
>> +##
>> +# @child-add
>> +#
>> +# Add a new child to quorum. This is useful to fix a broken quorum
>> child.
> 
> But the idea is that this can be eventually used by other drivers, isn't
> it?

Yes, but is is only used for quorum now. I don't find any other driver needs this feature.

Thanks
Wen Congyang

> 
> Berto
> .
> 

Re: [Qemu-devel] [PATCH for-2.5 5/6] qmp: add monitor command to add/remove a child

[Alberto Garcia]

On Mon 10 Aug 2015 03:00:32 AM CEST, Wen Congyang <wency@cn.fujitsu.com> wrote:

>>> +# @child-add
>>> +#
>>> +# Add a new child to quorum. This is useful to fix a broken quorum
>>> child.
>> 
>> But the idea is that this can be eventually used by other drivers, isn't
>> it?
>
> Yes, but is is only used for quorum now. I don't find any other driver
> needs this feature.

Maybe it's clearer if the documentation describes the general case and
then adds something like "Currently only the Quorum driver implements
this feature".

Then again I don't have a very strong opinion, if I'm the only one who
thinks like this you can leave it as it is now.

Berto

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

[Alberto Garcia]

On Fri 31 Jul 2015 11:19:12 AM CEST, Wen Congyang wrote:

> +static const char *bdrv_get_id_or_node_name(BlockDriverState *bs)
> +{
> +    if (bs->blk) {
> +        return blk_name(bs->blk);
> +    }
> +
> +    return bs->node_name ? bs->node_name : "";
> +}

You don't need to add this function, you can use
bdrv_get_device_or_node_name() instead.

Berto

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

[Wen Congyang]

On 08/10/2015 04:19 PM, Alberto Garcia wrote:
> On Fri 31 Jul 2015 11:19:12 AM CEST, Wen Congyang wrote:
> 
>> +static const char *bdrv_get_id_or_node_name(BlockDriverState *bs)
>> +{
>> +    if (bs->blk) {
>> +        return blk_name(bs->blk);
>> +    }
>> +
>> +    return bs->node_name ? bs->node_name : "";
>> +}
> 
> You don't need to add this function, you can use
> bdrv_get_device_or_node_name() instead.

Yes, I will update it in the next version.

Thanks
Wen Congyang

> 
> Berto
> .
>