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

[Qemu-devel] [Patch for-2.5 v2 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.

ChangLog:
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 (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 bdrv_add_child() and bdrv_del_child()
  qmp: add monitor command to add/remove a child
  hmp: add monitor command to add/remove a child

 block.c                   |  43 ++++++++++++++++++
 block/quorum.c            |  75 ++++++++++++++++++++++++++++++-
 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      |  57 ++++++++++++++++++++++-
 qapi/common.json          |  27 +++++++++++
 qmp-commands.hx           |  67 +++++++++++++++++++++++++++
 11 files changed, 416 insertions(+), 31 deletions(-)

-- 
2.4.3

[Qemu-devel] [Patch for-2.5 v2 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>
Reviewed-by: Alberto Garcia <berto@igalia.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 v2 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>
Reviewed-by: Alberto Garcia <berto@igalia.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 v2 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>
Reviewed-by: Alberto Garcia <berto@igalia.com>
---
 block.c                   | 43 +++++++++++++++++++++++++++++++++++++++++++
 include/block/block.h     |  4 ++++
 include/block/block_int.h |  5 +++++
 3 files changed, 52 insertions(+)

diff --git a/block.c b/block.c
index d088ee0..1746d99 100644
--- a/block.c
+++ b/block.c
@@ -4251,3 +4251,46 @@ BlockAcctStats *bdrv_get_stats(BlockDriverState *bs)
 {
     return &bs->stats;
 }
+
+/*
+ * Hot add/remove a BDS's child. So the user can take a child offline when
+ * it is broken and take a new child online
+ */
+void bdrv_add_child(BlockDriverState *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_device_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_device_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_device_or_node_name(child_bs),
+                   bdrv_get_device_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 v2 4/6] quorum: implement bdrv_add_child() and bdrv_del_child()

[Wen Congyang]

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

diff --git a/block/quorum.c b/block/quorum.c
index 2f6c45f..1305086 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.
                             */
@@ -874,9 +877,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;
     }
@@ -925,6 +928,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];
@@ -995,6 +999,70 @@ 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,
+            "The number of children cannot be lower than the vote threshold");
+        return;
+    }
+
+    if (s->num_children == 1) {
+        error_setg(errp, "Cannot remove the last child");
+        return;
+    }
+
+    bdrv_drain(bs);
+    /* We can safely 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;
@@ -1049,6 +1117,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 v2 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 | 40 ++++++++++++++++++++++++++
 qmp-commands.hx      | 67 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 186 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..50292b9 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2122,3 +2122,43 @@
 ##
 { 'command': 'block-set-write-threshold',
   'data': { 'node-name': 'str', 'write-threshold': 'uint64' } }
+
+##
+# @BlockdevOptionsChild
+#
+# Driver specific block device options for child block device.
+#
+# Since: 2.5
+##
+{ 'struct': 'BlockdevOptionsChild',
+  'data': { 'child': 'BlockdevOptions' } }
+
+##
+# @child-add
+#
+# Add a new child to the parent BDS. Currently only the Quorum driver
+# implements this feature. 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 the parent BDS. Currently only the Quorum driver
+# implements this feature. 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 v2 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 v2 6/6] hmp: add monitor command to add/remove a child

[Wen Congyang]

Ping...
Patch 1-4 have been reviewed by Alberto Garcia
Luiz Capitulino, Markus Armbruster, do you have time to review the monitor command implementation?

Thanks
Wen Congyang

On 08/11/2015 03:51 PM, 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: 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
> 

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

[Markus Armbruster]

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

> Ping...
> Patch 1-4 have been reviewed by Alberto Garcia
> Luiz Capitulino, Markus Armbruster, do you have time to review the
> monitor command implementation?

I will review at least the QMP parts of this series.  My review queue
has become painfully long...  I apologize for the delay, and have to ask
you for even more patience.

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

[Eric Blake]

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

On 08/11/2015 01:51 AM, 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>
> ---
>  qapi-schema.json | 27 ---------------------------
>  qapi/common.json | 27 +++++++++++++++++++++++++++
>  2 files changed, 27 insertions(+), 27 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

> +++ 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',

The fact that 'port' is sometimes textual, and sometimes an integer, is
pre-existing, so moving it does not affect the validity of this patch.
But I wonder if we should in the future add an 'alternate' type that
allows us to directly pass an integer for 'port', rather than always
having to pass a stringized integer.

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


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

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

[Eric Blake]

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

On 08/11/2015 01:51 AM, 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>
> Reviewed-by: Alberto Garcia <berto@igalia.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' ] }

Please keep the list alphabetical. Also, this is missing documentation
(see how BlockDeviceInfo has listed which releases have added which
formats; so it should add a listing of 'nbd' in 2.5).

>  
>  ##
>  # @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' } }

So does NBD really support a range of ports?  Or is it an error to
specify 'to'?  Perhaps what you should really be doing is making a
simpler base class for representing a single-port IP address, then
making InetSocketAddress a child class of the simpler one to add in
ranging, and make BlockdevOptionsNBD a child class of the simpler one to
add in an export name.

Also, InetSocketAddress appears to be limited to IPv4 and IPv6
addresses; but what about nbd+unix transport?  It feels like you need a
flat union with a discriminator that describes what address family (IP
vs. unix socket) and then further details based on that discriminator.

> +
> +##
>  # @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',

So while you are indeed literally taking the TODO suggestion, I'm not
sure that the literal interpretation is the best.

Remember, the current NBD source code accepts:

    if (is_unix) {
        /* nbd+unix:///export?socket=path */
        if (uri->server || uri->port || strcmp(qp->p[0].name, "socket")) {
            ret = -EINVAL;
            goto out;
        }
        qdict_put(options, "path", qstring_from_str(qp->p[0].value));
    } else {
        QString *host;
        /* nbd[+tcp]://host[:port]/export */

and we want to be able to represent all of those possibilities in QMP.

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


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

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

[Eric Blake]

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

On 08/11/2015 01:51 AM, Wen Congyang wrote:
> In some cases, we want to take a quorum child offline, and take
> another child online.
> 
> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> Reviewed-by: Alberto Garcia <berto@igalia.com>
> ---
>  block.c                   | 43 +++++++++++++++++++++++++++++++++++++++++++
>  include/block/block.h     |  4 ++++
>  include/block/block_int.h |  5 +++++
>  3 files changed, 52 insertions(+)
> 

> + * 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_device_or_node_name(bs));
> +        return;
> +    }
> +
> +    bs->drv->bdrv_add_child(bs, options, errp);

Should this also check that bs is not already a child of something?  Or
a bit looser, we may want to allow a BDS to be a child of multiple trees
(a common shared backing file), but we still definitely don't want to
allow nonsensical loops such as trying to make a BDS be hot-added as its
own child.

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


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

Re: [Qemu-devel] [Patch for-2.5 v2 4/6] quorum: implement bdrv_add_child() and bdrv_del_child()

[Eric Blake]

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

On 08/11/2015 01:51 AM, 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>
> Reviewed-by: Alberto Garcia <berto@igalia.com>
> ---
>  block/quorum.c | 75 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 73 insertions(+), 2 deletions(-)
> 
> diff --git a/block/quorum.c b/block/quorum.c
> index 2f6c45f..1305086 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.

s/will/grows/


> @@ -995,6 +999,70 @@ 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;

why not use ++?

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

The previous patch already assert()ed that the child was present; can't
this one just 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");
> +        return;

Might be nice to include the numeric value of that threshold in the
error message.

> +    }
> +
> +    if (s->num_children == 1) {
> +        error_setg(errp, "Cannot remove the last child");
> +        return;
> +    }

Isn't this dead code, as the vote threshold always has to be at least 1,
so the previous 'if' already rejected an attempt to go lower than the
threshold?

> +
> +    bdrv_drain(bs);
> +    /* We can safely remove this child now */
> +    memmove(&s->bs[i], &s->bs[i+1], (s->num_children - i - 1) * sizeof(void *));

Spaces around '+'.

> +    s->num_children--;
> +    s->bs[s->num_children] = NULL;
> +    bdrv_unref(child_bs);
> +}
> +
>  static void quorum_refresh_filename(BlockDriverState *bs)
>  {
>      BDRVQuorumState *s = bs->opaque;
> @@ -1049,6 +1117,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,
>  };
> 

Overall seems reasonable.

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


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

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

[Eric Blake]

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

On 08/11/2015 01:51 AM, 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>
> ---
>  blockdev.c           | 79 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  qapi/block-core.json | 40 ++++++++++++++++++++++++++
>  qmp-commands.hx      | 67 ++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 186 insertions(+)
> 

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

s/cann't/can't/

If they can't be used, then why not write the qapi so that they can't
even be provided?  On the other hand, why can't they be used?  Can't you
specify some of these options separately for different quorum children
when first creating a quorum, in which case you'd want to be able to do
likewise when adding a new member to the quorum?

> +++ b/qapi/block-core.json
> @@ -2122,3 +2122,43 @@
>  ##
>  { 'command': 'block-set-write-threshold',
>    'data': { 'node-name': 'str', 'write-threshold': 'uint64' } }
> +
> +##
> +# @BlockdevOptionsChild
> +#
> +# Driver specific block device options for child block device.
> +#
> +# Since: 2.5
> +##
> +{ 'struct': 'BlockdevOptionsChild',
> +  'data': { 'child': 'BlockdevOptions' } }

Do you need this struct?  It causes extra nesting on the wire...

> +
> +##
> +# @child-add
> +#
> +# Add a new child to the parent BDS. Currently only the Quorum driver
> +# implements this feature. 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' } }

...Consider if you just did:

{ 'command': 'child-add',
  'data': { 'device', 'str', 'child': 'BlockdevOptions' } }

> 
> +
> +##
> +# @child-del
> +#
> +# Remove a child from the parent BDS. Currently only the Quorum driver
> +# implements this feature. This is useful to fix a broken quorum child.

Might also be worth mentioning that you can't remove a child if it would
bring the quorum below its threshold.

> +++ b/qmp-commands.hx

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

Maybe we should name it 'x-child-add' for now, so that we aren't baking
ourselves into a corner.

> +
> +Arguments:
> +
> +- "device": the quorum's id or node name
> +- "options": the new child options
> +
> +Example:
> +
> +-> { "execute": "child-add",
> +    "arguments": {
> +        "device": "disk1",
> +        "options" : {
> +            "child": {

...the simper command idea above would reduce one layer of {} nesting here.

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


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

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

[Wen Congyang]

On 09/01/2015 01:40 AM, Eric Blake wrote:
> On 08/11/2015 01:51 AM, Wen Congyang wrote:
>> In some cases, we want to take a quorum child offline, and take
>> another child online.
>>
>> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
>> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
>> Reviewed-by: Alberto Garcia <berto@igalia.com>
>> ---
>>  block.c                   | 43 +++++++++++++++++++++++++++++++++++++++++++
>>  include/block/block.h     |  4 ++++
>>  include/block/block_int.h |  5 +++++
>>  3 files changed, 52 insertions(+)
>>
> 
>> + * 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_device_or_node_name(bs));
>> +        return;
>> +    }
>> +
>> +    bs->drv->bdrv_add_child(bs, options, errp);
> 
> Should this also check that bs is not already a child of something?  Or
> a bit looser, we may want to allow a BDS to be a child of multiple trees
> (a common shared backing file), but we still definitely don't want to
> allow nonsensical loops such as trying to make a BDS be hot-added as its
> own child.
> 

hot-added BDS is a new BDS, but it is OK to check it here. I will update it
in the next version.

Thanks
Wen Congyang

Re: [Qemu-devel] [Patch for-2.5 v2 4/6] quorum: implement bdrv_add_child() and bdrv_del_child()

[Wen Congyang]

On 09/01/2015 02:53 AM, Eric Blake wrote:
> On 08/11/2015 01:51 AM, 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>
>> Reviewed-by: Alberto Garcia <berto@igalia.com>
>> ---
>>  block/quorum.c | 75 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
>>  1 file changed, 73 insertions(+), 2 deletions(-)
>>
>> diff --git a/block/quorum.c b/block/quorum.c
>> index 2f6c45f..1305086 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.
> 
> s/will/grows/
> 
> 
>> @@ -995,6 +999,70 @@ 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;
> 
> why not use ++?
> 
>> +    }
>> +
>> +    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;
>> +    }
> 
> The previous patch already assert()ed that the child was present; can't
> this one just 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");
>> +        return;
> 
> Might be nice to include the numeric value of that threshold in the
> error message.
> 
>> +    }
>> +
>> +    if (s->num_children == 1) {
>> +        error_setg(errp, "Cannot remove the last child");
>> +        return;
>> +    }
> 
> Isn't this dead code, as the vote threshold always has to be at least 1,
> so the previous 'if' already rejected an attempt to go lower than the
> threshold?

Yes, the vote threshold has to be at least 1. But current codes have a bug, and
vote threshold can be 0. The patch is queued in Max's tree. I will remove
it in the later version.

All other comments will be addressed in the next version.

Thanks
Wen Congyang

> 
>> +
>> +    bdrv_drain(bs);
>> +    /* We can safely remove this child now */
>> +    memmove(&s->bs[i], &s->bs[i+1], (s->num_children - i - 1) * sizeof(void *));
> 
> Spaces around '+'.
> 
>> +    s->num_children--;
>> +    s->bs[s->num_children] = NULL;
>> +    bdrv_unref(child_bs);
>> +}
>> +
>>  static void quorum_refresh_filename(BlockDriverState *bs)
>>  {
>>      BDRVQuorumState *s = bs->opaque;
>> @@ -1049,6 +1117,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,
>>  };
>>
> 
> Overall seems reasonable.
> 

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

[Wen Congyang]

On 09/01/2015 03:04 AM, Eric Blake wrote:
> On 08/11/2015 01:51 AM, 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>
>> ---
>>  blockdev.c           | 79 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  qapi/block-core.json | 40 ++++++++++++++++++++++++++
>>  qmp-commands.hx      | 67 ++++++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 186 insertions(+)
>>
> 
>> +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");
> 
> s/cann't/can't/
> 
> If they can't be used, then why not write the qapi so that they can't
> even be provided?  On the other hand, why can't they be used?  Can't you
> specify some of these options separately for different quorum children
> when first creating a quorum, in which case you'd want to be able to do
> likewise when adding a new member to the quorum?

IIUC, it is just top BDS's option, and it is not be used by the hot-added
child. The hot-added child will use its parent flags.

> 
>> +++ b/qapi/block-core.json
>> @@ -2122,3 +2122,43 @@
>>  ##
>>  { 'command': 'block-set-write-threshold',
>>    'data': { 'node-name': 'str', 'write-threshold': 'uint64' } }
>> +
>> +##
>> +# @BlockdevOptionsChild
>> +#
>> +# Driver specific block device options for child block device.
>> +#
>> +# Since: 2.5
>> +##
>> +{ 'struct': 'BlockdevOptionsChild',
>> +  'data': { 'child': 'BlockdevOptions' } }
> 
> Do you need this struct?  It causes extra nesting on the wire...
> 
>> +
>> +##
>> +# @child-add
>> +#
>> +# Add a new child to the parent BDS. Currently only the Quorum driver
>> +# implements this feature. 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' } }
> 
> ...Consider if you just did:
> 
> { 'command': 'child-add',
>   'data': { 'device', 'str', 'child': 'BlockdevOptions' } }

OK, I will try it.

> 
>>
>> +
>> +##
>> +# @child-del
>> +#
>> +# Remove a child from the parent BDS. Currently only the Quorum driver
>> +# implements this feature. This is useful to fix a broken quorum child.
> 
> Might also be worth mentioning that you can't remove a child if it would
> bring the quorum below its threshold.
> 
>> +++ b/qmp-commands.hx
> 
>> +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.
> 
> Maybe we should name it 'x-child-add' for now, so that we aren't baking
> ourselves into a corner.

Do you mean the command name should be x-child-add? It is OK.

> 
>> +
>> +Arguments:
>> +
>> +- "device": the quorum's id or node name
>> +- "options": the new child options
>> +
>> +Example:
>> +
>> +-> { "execute": "child-add",
>> +    "arguments": {
>> +        "device": "disk1",
>> +        "options" : {
>> +            "child": {
> 
> ...the simper command idea above would reduce one layer of {} nesting here.
> 

Yes, I will try it.

Thanks
Wen Congyang

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

[Wen Congyang]

On 09/01/2015 01:40 AM, Eric Blake wrote:
> On 08/11/2015 01:51 AM, Wen Congyang wrote:
>> In some cases, we want to take a quorum child offline, and take
>> another child online.
>>
>> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
>> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
>> Reviewed-by: Alberto Garcia <berto@igalia.com>
>> ---
>>  block.c                   | 43 +++++++++++++++++++++++++++++++++++++++++++
>>  include/block/block.h     |  4 ++++
>>  include/block/block_int.h |  5 +++++
>>  3 files changed, 52 insertions(+)
>>
> 
>> + * 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_device_or_node_name(bs));
>> +        return;
>> +    }
>> +
>> +    bs->drv->bdrv_add_child(bs, options, errp);
> 
> Should this also check that bs is not already a child of something?  Or
> a bit looser, we may want to allow a BDS to be a child of multiple trees
> (a common shared backing file), but we still definitely don't want to
> allow nonsensical loops such as trying to make a BDS be hot-added as its
> own child.
> 

bs is parent, and the child is options, and will be opened by the parent.
So there is no need to check it.

Thanks
Wen Congyang

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

[Wen Congyang]

On 09/01/2015 03:04 AM, Eric Blake wrote:
> On 08/11/2015 01:51 AM, 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>
>> ---
>>  blockdev.c           | 79 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  qapi/block-core.json | 40 ++++++++++++++++++++++++++
>>  qmp-commands.hx      | 67 ++++++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 186 insertions(+)
>>
> 
>> +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");
> 
> s/cann't/can't/
> 
> If they can't be used, then why not write the qapi so that they can't
> even be provided?  On the other hand, why can't they be used?  Can't you
> specify some of these options separately for different quorum children
> when first creating a quorum, in which case you'd want to be able to do
> likewise when adding a new member to the quorum?
> 
>> +++ b/qapi/block-core.json
>> @@ -2122,3 +2122,43 @@
>>  ##
>>  { 'command': 'block-set-write-threshold',
>>    'data': { 'node-name': 'str', 'write-threshold': 'uint64' } }
>> +
>> +##
>> +# @BlockdevOptionsChild
>> +#
>> +# Driver specific block device options for child block device.
>> +#
>> +# Since: 2.5
>> +##
>> +{ 'struct': 'BlockdevOptionsChild',
>> +  'data': { 'child': 'BlockdevOptions' } }
> 
> Do you need this struct?  It causes extra nesting on the wire...
> 
>> +
>> +##
>> +# @child-add
>> +#
>> +# Add a new child to the parent BDS. Currently only the Quorum driver
>> +# implements this feature. 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' } }
> 
> ...Consider if you just did:
> 
> { 'command': 'child-add',
>   'data': { 'device', 'str', 'child': 'BlockdevOptions' } }
> 
>>
>> +
>> +##
>> +# @child-del
>> +#
>> +# Remove a child from the parent BDS. Currently only the Quorum driver
>> +# implements this feature. This is useful to fix a broken quorum child.
> 
> Might also be worth mentioning that you can't remove a child if it would
> bring the quorum below its threshold.
> 
>> +++ b/qmp-commands.hx
> 
>> +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.
> 
> Maybe we should name it 'x-child-add' for now, so that we aren't baking
> ourselves into a corner.
> 
>> +
>> +Arguments:
>> +
>> +- "device": the quorum's id or node name
>> +- "options": the new child options
>> +
>> +Example:
>> +
>> +-> { "execute": "child-add",
>> +    "arguments": {
>> +        "device": "disk1",
>> +        "options" : {
>> +            "child": {
> 
> ...the simper command idea above would reduce one layer of {} nesting here.
> 

When we open a child BDS, the options for child BDS must have the same prefix, such as
file.xxx, x-image.xxx, ... For hot-added child BDS, the prefix is "child“. If we don't
use "options" here, the prefix "child" will be eaten in qmp_marshal_input_x_child_add().
I am investigating how to solve it. Any suggestion is welcome.

Thanks
Wen Congyang

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

[Wen Congyang]

On 09/01/2015 01:19 AM, Eric Blake wrote:
> On 08/11/2015 01:51 AM, 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>
>> Reviewed-by: Alberto Garcia <berto@igalia.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' ] }
> 
> Please keep the list alphabetical. Also, this is missing documentation
> (see how BlockDeviceInfo has listed which releases have added which
> formats; so it should add a listing of 'nbd' in 2.5).
> 
>>  
>>  ##
>>  # @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' } }
> 
> So does NBD really support a range of ports?  Or is it an error to
> specify 'to'?  Perhaps what you should really be doing is making a
> simpler base class for representing a single-port IP address, then
> making InetSocketAddress a child class of the simpler one to add in
> ranging, and make BlockdevOptionsNBD a child class of the simpler one to
> add in an export name.

NBD doesn;t support a range of ports. I will update it in the next version.

> 
> Also, InetSocketAddress appears to be limited to IPv4 and IPv6
> addresses; but what about nbd+unix transport?  It feels like you need a
> flat union with a discriminator that describes what address family (IP
> vs. unix socket) and then further details based on that discriminator.

Hmm, It is SocketAddress, but it contains fd, which nbd doesn't support.
Another problem is that: NBD doesn't support type. It check the key path
and host:

    if (qdict_haskey(options, "path") == qdict_haskey(options, "host")) {
        if (qdict_haskey(options, "path")) {
            error_setg(errp, "path and host may not be used at the same time.");
        } else {
            error_setg(errp, "one of path and host must be specified.");
        }
        return;
    }

    s->client.is_unix = qdict_haskey(options, "path");

Update NBD to support it?

Thanks
Wen Congyang

> 
>> +
>> +##
>>  # @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',
> 
> So while you are indeed literally taking the TODO suggestion, I'm not
> sure that the literal interpretation is the best.
> 
> Remember, the current NBD source code accepts:
> 
>     if (is_unix) {
>         /* nbd+unix:///export?socket=path */
>         if (uri->server || uri->port || strcmp(qp->p[0].name, "socket")) {
>             ret = -EINVAL;
>             goto out;
>         }
>         qdict_put(options, "path", qstring_from_str(qp->p[0].value));
>     } else {
>         QString *host;
>         /* nbd[+tcp]://host[:port]/export */
> 
> and we want to be able to represent all of those possibilities in QMP.
> 

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

[Eric Blake]

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

On 08/31/2015 06:44 PM, 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_device_or_node_name(bs));
>>> +        return;
>>> +    }
>>> +
>>> +    bs->drv->bdrv_add_child(bs, options, errp);
>>
>> Should this also check that bs is not already a child of something?  Or
>> a bit looser, we may want to allow a BDS to be a child of multiple trees
>> (a common shared backing file), but we still definitely don't want to
>> allow nonsensical loops such as trying to make a BDS be hot-added as its
>> own child.
>>
> 
> hot-added BDS is a new BDS, but it is OK to check it here. I will update it
> in the next version.

Design-wise, I think we really want to have the add-child operation be
handed a pre-opened BDS, rather than the options dictionary to open the
BDS itself.  That is, we should use the existing blockdev-add (and
enhance it to support everything) to open the BDS, and then this command
should just attach that BDS as the new child (which is why it IS
important that we validate that the new BDS being added doesn't create
an invalid loop).

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


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

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

[Eric Blake]

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

On 08/31/2015 06:55 PM, Wen Congyang wrote:

>>> +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.
>>
>> Maybe we should name it 'x-child-add' for now, so that we aren't baking
>> ourselves into a corner.
> 
> Do you mean the command name should be x-child-add? It is OK.

Use of the 'x-' prefix means a command is experimental and may change or
be withdrawn.  It gives us a way to test if an interface is useful
without committing to that interface long term.  We've still got time
before 2.5 to get blockdev-add working everywhere, in which case I think
we are better off using blockdev-add to create a new unattached BDS and
then have this command pass the node name to be made the new child,
rather than all the options for opening the child from scratch.

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


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

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

[Wen Congyang]

On 09/01/2015 11:34 PM, Eric Blake wrote:
> On 08/31/2015 06:55 PM, Wen Congyang wrote:
> 
>>>> +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.
>>>
>>> Maybe we should name it 'x-child-add' for now, so that we aren't baking
>>> ourselves into a corner.
>>
>> Do you mean the command name should be x-child-add? It is OK.
> 
> Use of the 'x-' prefix means a command is experimental and may change or
> be withdrawn.  It gives us a way to test if an interface is useful
> without committing to that interface long term.  We've still got time
> before 2.5 to get blockdev-add working everywhere, in which case I think
> we are better off using blockdev-add to create a new unattached BDS and
> then have this command pass the node name to be made the new child,
> rather than all the options for opening the child from scratch.
> 

Good idea. The unattached BDS created by the command blockdev-add always
have BB. So it may be used by the device created by the command device_add
later. So I think we should have an API to check it. What about the following
patches?
http://lists.nongnu.org/archive/html/qemu-devel/2015-07/msg01591.html
http://lists.nongnu.org/archive/html/qemu-devel/2015-07/msg01590.html

Thanks
Wen Congyang

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

[Eric Blake]

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

On 09/01/2015 07:25 PM, Wen Congyang wrote:
> On 09/01/2015 11:34 PM, Eric Blake wrote:
>> On 08/31/2015 06:55 PM, Wen Congyang wrote:
>>
>>>>> +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.
>>>>
>>>> Maybe we should name it 'x-child-add' for now, so that we aren't baking
>>>> ourselves into a corner.
>>>
>>> Do you mean the command name should be x-child-add? It is OK.
>>
>> Use of the 'x-' prefix means a command is experimental and may change or
>> be withdrawn.  It gives us a way to test if an interface is useful
>> without committing to that interface long term.  We've still got time
>> before 2.5 to get blockdev-add working everywhere, in which case I think
>> we are better off using blockdev-add to create a new unattached BDS and
>> then have this command pass the node name to be made the new child,
>> rather than all the options for opening the child from scratch.
>>
> 
> Good idea. The unattached BDS created by the command blockdev-add always
> have BB. So it may be used by the device created by the command device_add
> later.

We recently discussed (although the current documentation of
BlockdevOptionsBase sounds like it hasn't been merged yet) the idea of
enhancing blockdev-add to control whether a BB is created alongside a BDS.

Remember, blockdev-add can be used to create a chain.  When originally
implemented, we documented that 'id' must be present on the top of the
chain, and absent everywhere else, and that 'node-name' was optional
everywhere.  The 'id' became associated with the BB at the top level,
and the optional node-names allow you to then manipulate other BDS.  But
the proposal at hand is that we would allow 'id' to be optional at the
top level (at which point 'node-name' is required, because we have to
have SOME way to refer to the BDS); and when that happens, we end up
creating a BDS that is not associated with any BB yet.  Similarly, it is
possible to create a BB that does not yet have a BDS yet (think an empty
cdrom drive); so it then becomes possible to associate a BDS to a BB as
a separate step.

[/me goes searching]
Ah - we are waiting for Max's patches to land:
https://lists.gnu.org/archive/html/qemu-devel/2015-07/msg04201.html
in particular patch 02/38:
https://lists.gnu.org/archive/html/qemu-devel/2015-07/msg04204.html

Once we have that, then we can indeed create a BDS without a BB, and it
is then that you can plug your unconnected BDS into a quorum.  So I'd
recommend helping review Max's series, and base your actions on top of
his (I really do think it is a better approach to separate BDS creation
from chain manipulation, and your add/remove a child is about chain
manipulation and does not need to be creating BDS).

> So I think we should have an API to check it. What about the following
> patches?
> http://lists.nongnu.org/archive/html/qemu-devel/2015-07/msg01591.html
> http://lists.nongnu.org/archive/html/qemu-devel/2015-07/msg01590.html

I haven't looked at them yet, but really like Max's approach for
separating BDS from BB by treating BB without BDS as an empty media
case, and BDS without a BB as something that can then be manipulated to
be associated with another BDS or BB.

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


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

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

[Wen Congyang]

On 09/02/2015 11:00 PM, Eric Blake wrote:
> On 09/01/2015 07:25 PM, Wen Congyang wrote:
>> On 09/01/2015 11:34 PM, Eric Blake wrote:
>>> On 08/31/2015 06:55 PM, Wen Congyang wrote:
>>>
>>>>>> +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.
>>>>>
>>>>> Maybe we should name it 'x-child-add' for now, so that we aren't baking
>>>>> ourselves into a corner.
>>>>
>>>> Do you mean the command name should be x-child-add? It is OK.
>>>
>>> Use of the 'x-' prefix means a command is experimental and may change or
>>> be withdrawn.  It gives us a way to test if an interface is useful
>>> without committing to that interface long term.  We've still got time
>>> before 2.5 to get blockdev-add working everywhere, in which case I think
>>> we are better off using blockdev-add to create a new unattached BDS and
>>> then have this command pass the node name to be made the new child,
>>> rather than all the options for opening the child from scratch.
>>>
>>
>> Good idea. The unattached BDS created by the command blockdev-add always
>> have BB. So it may be used by the device created by the command device_add
>> later.
> 
> We recently discussed (although the current documentation of
> BlockdevOptionsBase sounds like it hasn't been merged yet) the idea of
> enhancing blockdev-add to control whether a BB is created alongside a BDS.
> 
> Remember, blockdev-add can be used to create a chain.  When originally
> implemented, we documented that 'id' must be present on the top of the
> chain, and absent everywhere else, and that 'node-name' was optional
> everywhere.  The 'id' became associated with the BB at the top level,
> and the optional node-names allow you to then manipulate other BDS.  But
> the proposal at hand is that we would allow 'id' to be optional at the
> top level (at which point 'node-name' is required, because we have to
> have SOME way to refer to the BDS); and when that happens, we end up
> creating a BDS that is not associated with any BB yet.  Similarly, it is
> possible to create a BB that does not yet have a BDS yet (think an empty
> cdrom drive); so it then becomes possible to associate a BDS to a BB as
> a separate step.
> 
> [/me goes searching]
> Ah - we are waiting for Max's patches to land:
> https://lists.gnu.org/archive/html/qemu-devel/2015-07/msg04201.html
> in particular patch 02/38:
> https://lists.gnu.org/archive/html/qemu-devel/2015-07/msg04204.html
> 
> Once we have that, then we can indeed create a BDS without a BB, and it
> is then that you can plug your unconnected BDS into a quorum.  So I'd
> recommend helping review Max's series, and base your actions on top of
> his (I really do think it is a better approach to separate BDS creation
> from chain manipulation, and your add/remove a child is about chain
> manipulation and does not need to be creating BDS).
> 
>> So I think we should have an API to check it. What about the following
>> patches?
>> http://lists.nongnu.org/archive/html/qemu-devel/2015-07/msg01591.html
>> http://lists.nongnu.org/archive/html/qemu-devel/2015-07/msg01590.html
> 
> I haven't looked at them yet, but really like Max's approach for
> separating BDS from BB by treating BB without BDS as an empty media
> case, and BDS without a BB as something that can then be manipulated to
> be associated with another BDS or BB.

Do you mean we can create a top BDS without BB by the command line -drive
in the future?

Thanks
Wen Congyang

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

[Wen Congyang]

On 09/01/2015 11:30 PM, Eric Blake wrote:
> On 08/31/2015 06:44 PM, 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_device_or_node_name(bs));
>>>> +        return;
>>>> +    }
>>>> +
>>>> +    bs->drv->bdrv_add_child(bs, options, errp);
>>>
>>> Should this also check that bs is not already a child of something?  Or
>>> a bit looser, we may want to allow a BDS to be a child of multiple trees
>>> (a common shared backing file), but we still definitely don't want to
>>> allow nonsensical loops such as trying to make a BDS be hot-added as its
>>> own child.
>>>
>>
>> hot-added BDS is a new BDS, but it is OK to check it here. I will update it
>> in the next version.
> 
> Design-wise, I think we really want to have the add-child operation be
> handed a pre-opened BDS, rather than the options dictionary to open the
> BDS itself.  That is, we should use the existing blockdev-add (and
> enhance it to support everything) to open the BDS, and then this command
> should just attach that BDS as the new child (which is why it IS
> important that we validate that the new BDS being added doesn't create
> an invalid loop).
> 

How to check it? The parent BDS can get all children. But the child doesn't
know if it is some BDS's child.

Thanks
Wen Congyang

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

[Eric Blake]

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

On 09/08/2015 03:10 AM, Wen Congyang wrote:

>> Design-wise, I think we really want to have the add-child operation be
>> handed a pre-opened BDS, rather than the options dictionary to open the
>> BDS itself.  That is, we should use the existing blockdev-add (and
>> enhance it to support everything) to open the BDS, and then this command
>> should just attach that BDS as the new child (which is why it IS
>> important that we validate that the new BDS being added doesn't create
>> an invalid loop).
>>
> 
> How to check it? The parent BDS can get all children. But the child doesn't
> know if it is some BDS's child.

If I'm not mistaken, a child DOES know what its parent(s) are, once we
have Max's series for NULL BDS representing a BB without media.

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


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

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

[Eric Blake]

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

On 09/06/2015 09:55 PM, Wen Congyang wrote:
>> I haven't looked at them yet, but really like Max's approach for
>> separating BDS from BB by treating BB without BDS as an empty media
>> case, and BDS without a BB as something that can then be manipulated to
>> be associated with another BDS or BB.
> 
> Do you mean we can create a top BDS without BB by the command line -drive
> in the future?

Not quite. -drive means create a BB. But it should be possible to create
a BDS without a BB from the command line, to match what blockdev-add can
do in QMP.

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


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

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

[Wen Congyang]

On 09/08/2015 11:52 PM, Eric Blake wrote:
> On 09/08/2015 03:10 AM, Wen Congyang wrote:
> 
>>> Design-wise, I think we really want to have the add-child operation be
>>> handed a pre-opened BDS, rather than the options dictionary to open the
>>> BDS itself.  That is, we should use the existing blockdev-add (and
>>> enhance it to support everything) to open the BDS, and then this command
>>> should just attach that BDS as the new child (which is why it IS
>>> important that we validate that the new BDS being added doesn't create
>>> an invalid loop).
>>>
>>
>> How to check it? The parent BDS can get all children. But the child doesn't
>> know if it is some BDS's child.
> 
> If I'm not mistaken, a child DOES know what its parent(s) are, once we
> have Max's series for NULL BDS representing a BB without media.
> 

Which patch? I don't find it.

Thanks
Wen Congyang