[Qemu-devel] [PATCH v3 07/12] block: Accept node-name arguments for block-commit

[Jeff Cody]

This modifies the block operation block-commit so that it will
accept node-name arguments for either 'top' or 'base' BDS.

The filename and node-name are mutually exclusive to each other;
i.e.:
    "top" and "top-node-name" are mutually exclusive (enforced)
    "base" and "base-node-name" are mutually exclusive (enforced)

The "device" argument now becomes optional as well, because with
a node-name we can identify the block device chain.  It is only
optional if a node-name is not specified.

The preferred and recommended method now of using block-commit
is to specify the BDS to operate on via their node-name arguments.

This also adds an explicit check that base_bs is in the chain of
top_bs, because if they are referenced by their unique node-name
arguments, the discovery process does not intrinsically verify
they are in the same chain.

Reviewed-by: Eric Blake <address@hidden>
Signed-off-by: Jeff Cody <address@hidden>
---
 blockdev.c       | 81 +++++++++++++++++++++++++++++++++++++++++++++++++-------
 qapi-schema.json | 38 +++++++++++++++++++-------
 qmp-commands.hx  | 33 +++++++++++++++++------
 3 files changed, 125 insertions(+), 27 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 8007fd9..4399cb5 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1910,14 +1910,17 @@ void qmp_block_stream(const char *device, bool has_base,
     trace_qmp_block_stream(bs, bs->job);
 }
 
-void qmp_block_commit(const char *device,
+void qmp_block_commit(bool has_device, const char *device,
                       bool has_base, const char *base,
+                      bool has_base_node_name, const char *base_node_name,
                       bool has_top, const char *top,
+                      bool has_top_node_name, const char *top_node_name,
                       bool has_speed, int64_t speed,
                       Error **errp)
 {
-    BlockDriverState *bs;
-    BlockDriverState *base_bs, *top_bs;
+    BlockDriverState *bs = NULL;
+    BlockDriverState *base_bs = NULL;
+    BlockDriverState *top_bs = NULL;
     Error *local_err = NULL;
     /* This will be part of the QMP command, if/when the
      * BlockdevOnError change for blkmirror makes it in
@@ -1931,18 +1934,61 @@ void qmp_block_commit(const char *device,
     /* drain all i/o before commits */
     bdrv_drain_all();
 
-    bs = bdrv_find(device);
-    if (!bs) {
-        error_set(errp, QERR_DEVICE_NOT_FOUND, device);
+    /* argument combination validation */
+    if (has_base && has_base_node_name) {
+        error_setg(errp, "'base' and 'base-node-name' are mutually exclusive");
+        return;
+    }
+    if (has_top && has_top_node_name) {
+        error_setg(errp, "'top' and 'top-node-name' are mutually exclusive");
         return;
     }
 
-    if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_COMMIT, errp)) {
+    if ((has_base || has_top) && !has_device) {
+        error_setg(errp, "'device' required if 'top' or 'base' specified");
+        return;
+    }
+
+    if (!has_top  && !has_top_node_name  &&
+        !has_base && !has_base_node_name && !has_device) {
+        error_setg(errp, "'device' required if no node-name specified");
         return;
     }
 
-    /* default top_bs is the active layer */
-    top_bs = bs;
+    if (has_device) {
+        /* device lookups */
+        bs = bdrv_find(device);
+        if (!bs) {
+            error_set(errp, QERR_DEVICE_NOT_FOUND, device);
+            return;
+        }
+    }
+
+    if (has_top_node_name) {
+        top_bs = bdrv_lookup_bs(NULL, top_node_name, &local_err);
+        if (local_err) {
+            error_propagate(errp, local_err);
+            return;
+        }
+        bs = bs ?: bdrv_find_active(top_bs);
+    }
+
+    if (has_base_node_name) {
+        base_bs = bdrv_lookup_bs(NULL, base_node_name, &local_err);
+        if (local_err) {
+            error_propagate(errp, local_err);
+            return;
+        }
+        bs = bs ?: bdrv_find_active(base_bs);
+    }
+
+    if (!bs) {
+        error_setg(errp, "Could not find active layer");
+        return;
+    }
+
+    /* default top_bs is the active layer, if NULL */
+    top_bs = top_bs ?: bs;
 
     if (has_top && top) {
         if (strcmp(bs->filename, top) != 0) {
@@ -1966,6 +2012,23 @@ void qmp_block_commit(const char *device,
         return;
     }
 
+    /* Verify that 'base' is in the same chain as 'top' */
+    if (!bdrv_chain_contains(top_bs, base_bs)) {
+        error_setg(errp, "'base' and 'top' are not in the same chain");
+        return;
+    }
+
+    /* This should technically be caught in commit_start, but
+     * check here for validation completeness */
+    if (!bdrv_chain_contains(bs, top_bs)) {
+        error_setg(errp, "'%s' and 'top' are not in the same chain", device);
+        return;
+    }
+
+    if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_COMMIT, errp)) {
+        return;
+    }
+
     if (top_bs == bs) {
         commit_active_start(bs, base_bs, speed, on_error, block_job_cb,
                             bs, &local_err);
diff --git a/qapi-schema.json b/qapi-schema.json
index 97cf053..2858155 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -2097,14 +2097,30 @@
 # Live commit of data from overlay image nodes into backing nodes - i.e.,
 # writes data between 'top' and 'base' into 'base'.
 #
-# @device:  the name of the device
+# @device:        #optional The name of the device.  Optional only if
+#                           node-names are used for both base and top.
 #
-# @base:   #optional The file name of the backing image to write data into.
-#                    If not specified, this is the deepest backing image
+# For 'base', either @base or @base-node-name may be set but not both. If
+# neither is specified, this is the deepest backing image.
 #
-# @top:    #optional The file name of the backing image within the image chain,
-#                    which contains the topmost data to be committed down. If
-#                    not specified, this is the active layer.
+# @base:          #optional The file name of the backing image to write data
+#                           into.
+#
+# @base-node-name #optional The name of the block driver state node of the
+#                           backing image to write data into.
+#                           (Since 2.1)
+#
+# For 'top', either @top or @top-node-name must be set but not both. If
+# neither is specified, this is the active layer.
+#
+# @top:           #optional The file name of the backing image within the image
+#                           chain, which contains the topmost data to be
+#                           committed down.
+#
+# @top-node-name: #optional The block driver state node name of the backing
+#                           image within the image chain, which contains the
+#                           topmost data to be committed down.
+#                           (Since 2.1)
 #
 #                    If top == base, that is an error.
 #                    If top == active, the job will not be completed by itself,
@@ -2123,17 +2139,19 @@
 #
 # Returns: Nothing on success
 #          If commit or stream is already active on this device, DeviceInUse
-#          If @device does not exist, DeviceNotFound
+#          If @device does not exist or cannot be determined, DeviceNotFound
 #          If image commit is not supported by this device, NotSupported
-#          If @base or @top is invalid, a generic error is returned
+#          If @base, @top, @base-node-name, @top-node-name invalid, 
GenericError
 #          If @speed is invalid, InvalidParameter
+#          If both @base and @base-node-name are specified, GenericError
+#          If both @top and @top-node-name are specified, GenericError
 #
 # Since: 1.3
 #
 ##
 { 'command': 'block-commit',
-  'data': { 'device': 'str', '*base': 'str', '*top': 'str',
-            '*speed': 'int' } }
+  'data': { '*device': 'str', '*base': 'str', '*base-node-name': 'str',
+            '*top': 'str', '*top-node-name': 'str', '*speed': 'int' } }
 
 ##
 # @drive-backup
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 6b67d2f..e0b6916 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -985,7 +985,7 @@ EQMP
 
     {
         .name       = "block-commit",
-        .args_type  = "device:B,base:s?,top:s?,speed:o?",
+        .args_type  = 
"device:B?,base:s?,base-node-name:s?,top:s?,top-node-name:s?,speed:o?",
         .mhandler.cmd_new = qmp_marshal_input_block_commit,
     },
 
@@ -998,13 +998,30 @@ data between 'top' and 'base' into 'base'.
 
 Arguments:
 
-- "device": The device's ID, must be unique (json-string)
-- "base": The file name of the backing image to write data into.
-          If not specified, this is the deepest backing image
-          (json-string, optional)
-- "top":  The file name of the backing image within the image chain,
-          which contains the topmost data to be committed down. If
-          not specified, this is the active layer. (json-string, optional)
+- "device":         The device's ID, must be unique (json-string, optional)
+                    Optional only if node-names are used to identify
+                    "top" and "base"
+
+For 'base', either base or base-node-name may be set but not both. If
+neither is specified, this is the deepest backing image
+
+- "base":           The file name of the backing image to write data into.
+                    (json-string, optional)
+- "base-node-name": The name of the block driver state node of the
+                    backing image to write data into.
+                    (json-string, optional) (Since 2.1)
+
+For 'top', either top or top-node-name must be set but not both. If
+neither is specified, this is the active layer
+
+- "top":            The file name of the backing image within the image chain,
+                    which contains the topmost data to be committed down.
+                    (json-string, optional)
+
+- "top-node-name":  The block driver state node name of the backing
+                    image within the image chain, which contains the
+                    topmost data to be committed down.
+                    (json-string, optional) (Since 2.1)
 
           If top == base, that is an error.
           If top == active, the job will not be completed by itself,
-- 
1.8.3.1