diff mbox

[v2,07/11] block: Accept node-name arguments for block-commit

Message ID 6a8ad9e44d39a5134fb66728ccdb035bae3a9500.1401200582.git.jcody@redhat.com
State New
Headers show

Commit Message

Jeff Cody May 27, 2014, 2:28 p.m. UTC 
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.

Signed-off-by: Jeff Cody <jcody@redhat.com>
---
 blockdev.c       | 83 +++++++++++++++++++++++++++++++++++++++++++++++++-------
 qapi-schema.json | 38 +++++++++++++++++++-------
 qmp-commands.hx  | 33 ++++++++++++++++------
 3 files changed, 126 insertions(+), 28 deletions(-)

Comments

Eric Blake May 27, 2014, 5:46 p.m. UTC | #1 
On 05/27/2014 08:28 AM, Jeff Cody wrote:
> 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.
> 
> Signed-off-by: Jeff Cody <jcody@redhat.com>
> ---
>  blockdev.c       | 83 +++++++++++++++++++++++++++++++++++++++++++++++++-------
>  qapi-schema.json | 38 +++++++++++++++++++-------
>  qmp-commands.hx  | 33 ++++++++++++++++------
>  3 files changed, 126 insertions(+), 28 deletions(-)
> 

> +    /* default top_bs is the active layer, if NULL */
> +    top_bs = top_bs ?: bs;
> +
> +    if (has_top && top) {
>          if (strcmp(bs->filename, top) != 0) {

Cool!  This part fixes the complaint I had in 6/11.  If you have a
reason to rebase, you could hoist this one-line if change into that
patch, while still keeping my R-b on both patches.

Everything else here looks sane (lots of conditions before starting, but
all of them made sense, and while there are five separate optional
arguments that can interplay, my read of your logic says you correctly
covered all the cases)

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

More precisely, it is also optional if node-name is used for base and
top/top-node-name is omitted, or if node-name is used for top and
base/base-node-name is omitted; but that's splitting hairs, so keep your
current wording.


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

We aren't consistent on whether to use a trailing '.'; if you have a
respin for other reasons, it's worth thinking about; but it's not a
cause for a respin by itself.

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

Patch

diff --git a/blockdev.c b/blockdev.c
index b37ace7..bb37670 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1911,14 +1911,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
@@ -1932,20 +1935,63 @@  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 (top) {
+    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) {
             top_bs = bdrv_find_backing_image(bs, top);
         }
@@ -1967,6 +2013,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..ca6f9c6 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,