[Qemu-devel] [PATCH v4 00/10] Modify block jobs to use node-names

[Qemu-devel] [PATCH v4 00/10] Modify block jobs to use node-names

[Jeff Cody]

This is v4 of "block: Modify block-commit to use node-names".

Changes from v3->v4:

* Rebased on master
* Dropped overlay pointers, Eric's concerns are correct
* Require "device" for all arguments, in light of the above,
  so we can find the active layer in all cases.
* Simplify more operations!
* Dropped Eric's Reviewed-by: on patches 3,5,6
  Added Eric's Reviewed-by: on patches 8,9


Changes from v2->v3:

* Add Eric's reviewed-by
* Addressed Eric's review comments
* Dropped HMP changes
* Added helper function for setting the overlay, and
  set the overlay in bdrv_append()
* Use bs->backing_file instead of bs->backing_hd->filename in block_stream 

Using node-names instead of filenames for block job operations
over QMP is a superior method of identifying the block driver
images to operate on, as it removes all pathname ambiguity.

This series modifies block-commit and block-stream to use node-names,
and creates a new QAPI command to allow stand-alone backing file
changes on an image file.

So that node-names can be used as desired for all block job
operations, this series also auto-generates node-names for every
BDS.  User-specified node-names will override any autogenerated

Jeff Cody (10):
  block: Auto-generate node_names for each BDS entry
  block: add helper function to determine if a BDS is in a chain
  block: simplify bdrv_find_base() and bdrv_find_overlay()
  block: make 'top' argument to block-commit optional
  block: Accept node-name arguments for block-commit
  block: extend block-commit to accept a string for the backing file
  block: add ability for block-stream to use node-name
  block: add backing-file option to block-stream
  block: Add QMP documentation for block-stream
  block: add QAPI command to allow live backing file change

 block.c                   |  80 ++++++++--------
 block/commit.c            |   9 +-
 block/stream.c            |  11 +--
 blockdev.c                | 228 ++++++++++++++++++++++++++++++++++++++++++----
 hmp.c                     |   1 +
 include/block/block.h     |   4 +-
 include/block/block_int.h |   3 +-
 qapi-schema.json          | 145 ++++++++++++++++++++++++++---
 qmp-commands.hx           | 181 ++++++++++++++++++++++++++++++++++--
 tests/qemu-iotests/040    |  28 ++++--
 10 files changed, 592 insertions(+), 98 deletions(-)

-- 
1.9.3

[Qemu-devel] [PATCH v4 01/10] block: Auto-generate node_names for each BDS entry

[Jeff Cody]

Currently, node_name is only filled in when done so explicitly by the
user.  If no node_name is specified, then the node name field is not
populated.

If node_names are automatically generated when not specified, that means
that all block job operations can be done by reference to the unique
node_name field.  This eliminates ambiguity in resolving filenames
(relative filenames, or file descriptors, symlinks, mounts, etc..) that
qemu currently needs to deal with.

If a node name is specified, then it will not be automatically
generated for that BDS entry.

If it is automatically generated, it will be prefaced with "__qemu##",
followed by 8 characters of a unique number, followed by 8 random
ASCII characters in the range of 'A-Z'.  Some sample generated node-name
strings:
    __qemu##00000000IAIYNXXR
    __qemu##00000002METXTRBQ
    __qemu##00000001FMBORDWG

The prefix is to aid in identifying it as a qemu-generated name, the
numeric portion is to guarantee uniqueness in a given qemu session, and
the random characters are to further avoid any accidental collisions
with user-specified node-names.

Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Jeff Cody <jcody@redhat.com>
---
 block.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/block.c b/block.c
index 310ea89..9882d3f 100644
--- a/block.c
+++ b/block.c
@@ -842,12 +842,26 @@ static int bdrv_open_flags(BlockDriverState *bs, int flags)
     return open_flags;
 }
 
+#define GEN_NODE_NAME_PREFIX    "__qemu##"
+#define GEN_NODE_NAME_MAX_LEN   (sizeof(GEN_NODE_NAME_PREFIX) + 8 + 8)
 static void bdrv_assign_node_name(BlockDriverState *bs,
                                   const char *node_name,
                                   Error **errp)
 {
+    char gen_node_name[GEN_NODE_NAME_MAX_LEN];
+    static uint32_t counter; /* simple counter to guarantee uniqueness */
+
+    /* if node_name is NULL, auto-generate a node name */
     if (!node_name) {
-        return;
+        int len;
+        snprintf(gen_node_name, GEN_NODE_NAME_MAX_LEN,
+                 "%s%08x", GEN_NODE_NAME_PREFIX, counter++);
+        len = strlen(gen_node_name);
+        while (len < GEN_NODE_NAME_MAX_LEN - 1) {
+            gen_node_name[len++] = g_random_int_range('A', 'Z');
+        }
+        gen_node_name[GEN_NODE_NAME_MAX_LEN - 1] = '\0';
+        node_name = gen_node_name;
     }
 
     /* empty string node name is invalid */
-- 
1.9.3

[Qemu-devel] [PATCH v4 02/10] block: add helper function to determine if a BDS is in a chain

[Jeff Cody]

This is a small helper function, to determine if 'base' is in the
chain of BlockDriverState 'top'.  It returns true if it is in the chain,
and false otherwise.

If either argument is NULL, it will also return false.

Reviewed-by: Benoit Canet <benoit@irqsave.net>
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Jeff Cody <jcody@redhat.com>
---
 block.c               | 11 +++++++++++
 include/block/block.h |  1 +
 2 files changed, 12 insertions(+)

diff --git a/block.c b/block.c
index 9882d3f..ad3fbc1 100644
--- a/block.c
+++ b/block.c
@@ -3812,6 +3812,17 @@ BlockDriverState *bdrv_lookup_bs(const char *device,
     return NULL;
 }
 
+/* If 'base' is in the same chain as 'top', return true. Otherwise,
+ * return false.  If either argument is NULL, return false. */
+bool bdrv_chain_contains(BlockDriverState *top, BlockDriverState *base)
+{
+    while (top && top != base) {
+        top = top->backing_hd;
+    }
+
+    return top != NULL;
+}
+
 BlockDriverState *bdrv_next(BlockDriverState *bs)
 {
     if (!bs) {
diff --git a/include/block/block.h b/include/block/block.h
index faee3aa..4dc68be 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -404,6 +404,7 @@ BlockDeviceInfoList *bdrv_named_nodes_list(void);
 BlockDriverState *bdrv_lookup_bs(const char *device,
                                  const char *node_name,
                                  Error **errp);
+bool bdrv_chain_contains(BlockDriverState *top, BlockDriverState *base);
 BlockDriverState *bdrv_next(BlockDriverState *bs);
 void bdrv_iterate(void (*it)(void *opaque, BlockDriverState *bs),
                   void *opaque);
-- 
1.9.3

[Qemu-devel] [PATCH v4 03/10] block: simplify bdrv_find_base() and bdrv_find_overlay()

[Jeff Cody]

This simplifies the function bdrv_find_overlay().  With this change,
bdrv_find_base() is just a subset of usage of bdrv_find_overlay(),
so this also take advantage of that.

Signed-off-by: Jeff Cody <jcody@redhat.com>
---
 block.c | 45 ++++++++++-----------------------------------
 1 file changed, 10 insertions(+), 35 deletions(-)

diff --git a/block.c b/block.c
index ad3fbc1..6b8fbe5 100644
--- a/block.c
+++ b/block.c
@@ -2561,32 +2561,23 @@ int bdrv_change_backing_file(BlockDriverState *bs,
  *
  * Returns NULL if bs is not found in active's image chain,
  * or if active == bs.
+ *
+ * Returns the bottommost base image if bs == NULL.
  */
 BlockDriverState *bdrv_find_overlay(BlockDriverState *active,
                                     BlockDriverState *bs)
 {
-    BlockDriverState *overlay = NULL;
-    BlockDriverState *intermediate;
-
-    assert(active != NULL);
-    assert(bs != NULL);
-
-    /* if bs is the same as active, then by definition it has no overlay
-     */
-    if (active == bs) {
-        return NULL;
+    while (active && bs != active->backing_hd) {
+        active = active->backing_hd;
     }
 
-    intermediate = active;
-    while (intermediate->backing_hd) {
-        if (intermediate->backing_hd == bs) {
-            overlay = intermediate;
-            break;
-        }
-        intermediate = intermediate->backing_hd;
-    }
+    return active;
+}
 
-    return overlay;
+/* Given a BDS, searches for the base layer. */
+BlockDriverState *bdrv_find_base(BlockDriverState *bs)
+{
+    return bdrv_find_overlay(bs, NULL);
 }
 
 typedef struct BlkIntermediateStates {
@@ -4359,22 +4350,6 @@ int bdrv_get_backing_file_depth(BlockDriverState *bs)
     return 1 + bdrv_get_backing_file_depth(bs->backing_hd);
 }
 
-BlockDriverState *bdrv_find_base(BlockDriverState *bs)
-{
-    BlockDriverState *curr_bs = NULL;
-
-    if (!bs) {
-        return NULL;
-    }
-
-    curr_bs = bs;
-
-    while (curr_bs->backing_hd) {
-        curr_bs = curr_bs->backing_hd;
-    }
-    return curr_bs;
-}
-
 /**************************************************************/
 /* async I/Os */
 
-- 
1.9.3

[Qemu-devel] [PATCH v4 04/10] block: make 'top' argument to block-commit optional

[Jeff Cody]

Now that active layer block-commit is supported, the 'top' argument
no longer needs to be mandatory.

Change it to optional, with the default being the active layer in the
device chain.

Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Benoit Canet <benoit@irqsave.net>
Signed-off-by: Jeff Cody <jcody@redhat.com>
---
 blockdev.c             |  5 +++--
 qapi-schema.json       |  7 ++++---
 qmp-commands.hx        |  5 +++--
 tests/qemu-iotests/040 | 28 ++++++++++++++++++----------
 4 files changed, 28 insertions(+), 17 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 9b5261b..58b526a 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1914,7 +1914,8 @@ void qmp_block_stream(const char *device, bool has_base,
 }
 
 void qmp_block_commit(const char *device,
-                      bool has_base, const char *base, const char *top,
+                      bool has_base, const char *base,
+                      bool has_top, const char *top,
                       bool has_speed, int64_t speed,
                       Error **errp)
 {
@@ -1946,7 +1947,7 @@ void qmp_block_commit(const char *device,
     /* default top_bs is the active layer */
     top_bs = bs;
 
-    if (top) {
+    if (has_top && top) {
         if (strcmp(bs->filename, top) != 0) {
             top_bs = bdrv_find_backing_image(bs, top);
         }
diff --git a/qapi-schema.json b/qapi-schema.json
index 7bc33ea..97cf053 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -2102,8 +2102,9 @@
 # @base:   #optional The file name of the backing image to write data into.
 #                    If not specified, this is the deepest backing image
 #
-# @top:              The file name of the backing image within the image chain,
-#                    which contains the topmost data to be committed down.
+# @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.
 #
 #                    If top == base, that is an error.
 #                    If top == active, the job will not be completed by itself,
@@ -2131,7 +2132,7 @@
 #
 ##
 { 'command': 'block-commit',
-  'data': { 'device': 'str', '*base': 'str', 'top': 'str',
+  'data': { 'device': 'str', '*base': 'str', '*top': 'str',
             '*speed': 'int' } }
 
 ##
diff --git a/qmp-commands.hx b/qmp-commands.hx
index d8aa4ed..6b67d2f 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?,top:s?,speed:o?",
         .mhandler.cmd_new = qmp_marshal_input_block_commit,
     },
 
@@ -1003,7 +1003,8 @@ Arguments:
           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.
+          which contains the topmost data to be committed down. If
+          not specified, this is the active layer. (json-string, optional)
 
           If top == base, that is an error.
           If top == active, the job will not be completed by itself,
diff --git a/tests/qemu-iotests/040 b/tests/qemu-iotests/040
index 734b6a6..803b0c7 100755
--- a/tests/qemu-iotests/040
+++ b/tests/qemu-iotests/040
@@ -35,11 +35,7 @@ test_img = os.path.join(iotests.test_dir, 'test.img')
 class ImageCommitTestCase(iotests.QMPTestCase):
     '''Abstract base class for image commit test cases'''
 
-    def run_commit_test(self, top, base):
-        self.assert_no_active_block_jobs()
-        result = self.vm.qmp('block-commit', device='drive0', top=top, base=base)
-        self.assert_qmp(result, 'return', {})
-
+    def wait_for_complete(self):
         completed = False
         while not completed:
             for event in self.vm.get_qmp_events(wait=True):
@@ -58,6 +54,18 @@ class ImageCommitTestCase(iotests.QMPTestCase):
         self.assert_no_active_block_jobs()
         self.vm.shutdown()
 
+    def run_commit_test(self, top, base):
+        self.assert_no_active_block_jobs()
+        result = self.vm.qmp('block-commit', device='drive0', top=top, base=base)
+        self.assert_qmp(result, 'return', {})
+        self.wait_for_complete()
+
+    def run_default_commit_test(self):
+        self.assert_no_active_block_jobs()
+        result = self.vm.qmp('block-commit', device='drive0')
+        self.assert_qmp(result, 'return', {})
+        self.wait_for_complete()
+
 class TestSingleDrive(ImageCommitTestCase):
     image_len = 1 * 1024 * 1024
     test_len = 1 * 1024 * 256
@@ -109,17 +117,17 @@ class TestSingleDrive(ImageCommitTestCase):
         self.assertEqual(-1, qemu_io('-c', 'read -P 0xab 0 524288', backing_img).find("verification failed"))
         self.assertEqual(-1, qemu_io('-c', 'read -P 0xef 524288 524288', backing_img).find("verification failed"))
 
+    def test_top_is_default_active(self):
+        self.run_default_commit_test()
+        self.assertEqual(-1, qemu_io('-c', 'read -P 0xab 0 524288', backing_img).find("verification failed"))
+        self.assertEqual(-1, qemu_io('-c', 'read -P 0xef 524288 524288', backing_img).find("verification failed"))
+
     def test_top_and_base_reversed(self):
         self.assert_no_active_block_jobs()
         result = self.vm.qmp('block-commit', device='drive0', top='%s' % backing_img, base='%s' % mid_img)
         self.assert_qmp(result, 'error/class', 'GenericError')
         self.assert_qmp(result, 'error/desc', 'Base \'%s\' not found' % mid_img)
 
-    def test_top_omitted(self):
-        self.assert_no_active_block_jobs()
-        result = self.vm.qmp('block-commit', device='drive0')
-        self.assert_qmp(result, 'error/class', 'GenericError')
-        self.assert_qmp(result, 'error/desc', "Parameter 'top' is missing")
 
 class TestRelativePaths(ImageCommitTestCase):
     image_len = 1 * 1024 * 1024
-- 
1.9.3

[Qemu-devel] [PATCH v4 05/10] 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.

Signed-off-by: Jeff Cody <jcody@redhat.com>
---
 blockdev.c       | 55 +++++++++++++++++++++++++++++++++++++++++++++++--------
 qapi-schema.json | 37 +++++++++++++++++++++++++++----------
 qmp-commands.hx  | 31 +++++++++++++++++++++++--------
 3 files changed, 97 insertions(+), 26 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 58b526a..50d4890 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1915,12 +1915,15 @@ void qmp_block_stream(const char *device, bool has_base,
 
 void qmp_block_commit(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
@@ -1934,20 +1937,33 @@ 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)) {
+    /* device lookups */
+    bs = bdrv_find(device);
+    if (!bs) {
+        error_set(errp, QERR_DEVICE_NOT_FOUND, device);
         return;
     }
 
     /* default top_bs is the active layer */
     top_bs = bs;
 
-    if (has_top && top) {
+    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;
+        }
+    } else if (has_top && top) {
         if (strcmp(bs->filename, top) != 0) {
             top_bs = bdrv_find_backing_image(bs, top);
         }
@@ -1958,7 +1974,13 @@ void qmp_block_commit(const char *device,
         return;
     }
 
-    if (has_base && base) {
+    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;
+        }
+    } else if (has_base && base) {
         base_bs = bdrv_find_backing_image(top_bs, base);
     } else {
         base_bs = bdrv_find_base(top_bs);
@@ -1969,6 +1991,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..67a627f 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -2097,14 +2097,29 @@
 # 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:                 The name of the device.
 #
-# @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 +2138,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..c61f4cb 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,28 @@ 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)
+
+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.9.3

[Qemu-devel] [PATCH v4 06/10] block: extend block-commit to accept a string for the backing file

[Jeff Cody]

On some image chains, QEMU may not always be able to resolve the
filenames properly, when updating the backing file of an image
after a block commit.

For instance, certain relative pathnames may fail, or drives may
have been specified originally by file descriptor (e.g. /dev/fd/???),
or a relative protocol pathname may have been used.

In these instances, QEMU may lack the information to be able to make
the correct choice, but the user or management layer most likely does
have that knowledge.

With this extension to the block-commit api, the user is able to change
the backing file of the overlay image as part of the block-commit
operation.

This allows the change to be 'safe', in the sense that if the attempt
to write the overlay image metadata fails, then the block-commit
operation returns failure, without disrupting the guest.

If the commit top is the active layer, then specifying the backing
file string will be treated as an error (there is no overlay image
to modify in that case).

If a backing file string is not specified in the command, the backing
file string to use is determined in the same manner as it was
previously.

Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Jeff Cody <jcody@redhat.com>
---
 block.c                   |  8 ++++++--
 block/commit.c            |  9 ++++++---
 blockdev.c                |  8 +++++++-
 include/block/block.h     |  3 ++-
 include/block/block_int.h |  3 ++-
 qapi-schema.json          | 23 +++++++++++++++++++++--
 qmp-commands.hx           | 19 ++++++++++++++++++-
 7 files changed, 62 insertions(+), 11 deletions(-)

diff --git a/block.c b/block.c
index 6b8fbe5..ccff942 100644
--- a/block.c
+++ b/block.c
@@ -2608,12 +2608,15 @@ typedef struct BlkIntermediateStates {
  *
  * base <- active
  *
+ * If backing_file_str is non-NULL, it will be used when modifying top's
+ * overlay image metadata.
+ *
  * Error conditions:
  *  if active == top, that is considered an error
  *
  */
 int bdrv_drop_intermediate(BlockDriverState *active, BlockDriverState *top,
-                           BlockDriverState *base)
+                           BlockDriverState *base, const char *backing_file_str)
 {
     BlockDriverState *intermediate;
     BlockDriverState *base_bs = NULL;
@@ -2665,7 +2668,8 @@ int bdrv_drop_intermediate(BlockDriverState *active, BlockDriverState *top,
     }
 
     /* success - we can delete the intermediate states, and link top->base */
-    ret = bdrv_change_backing_file(new_top_bs, base_bs->filename,
+    backing_file_str = backing_file_str ? backing_file_str : base_bs->filename;
+    ret = bdrv_change_backing_file(new_top_bs, backing_file_str,
                                    base_bs->drv ? base_bs->drv->format_name : "");
     if (ret) {
         goto exit;
diff --git a/block/commit.c b/block/commit.c
index 5c09f44..91517d3 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -37,6 +37,7 @@ typedef struct CommitBlockJob {
     BlockdevOnError on_error;
     int base_flags;
     int orig_overlay_flags;
+    char *backing_file_str;
 } CommitBlockJob;
 
 static int coroutine_fn commit_populate(BlockDriverState *bs,
@@ -141,7 +142,7 @@ wait:
 
     if (!block_job_is_cancelled(&s->common) && sector_num == end) {
         /* success */
-        ret = bdrv_drop_intermediate(active, top, base);
+        ret = bdrv_drop_intermediate(active, top, base, s->backing_file_str);
     }
 
 exit_free_buf:
@@ -158,7 +159,7 @@ exit_restore_reopen:
     if (overlay_bs && s->orig_overlay_flags != bdrv_get_flags(overlay_bs)) {
         bdrv_reopen(overlay_bs, s->orig_overlay_flags, NULL);
     }
-
+    g_free(s->backing_file_str);
     block_job_completed(&s->common, ret);
 }
 
@@ -182,7 +183,7 @@ static const BlockJobDriver commit_job_driver = {
 void commit_start(BlockDriverState *bs, BlockDriverState *base,
                   BlockDriverState *top, int64_t speed,
                   BlockdevOnError on_error, BlockDriverCompletionFunc *cb,
-                  void *opaque, Error **errp)
+                  void *opaque, const char *backing_file_str, Error **errp)
 {
     CommitBlockJob *s;
     BlockReopenQueue *reopen_queue = NULL;
@@ -244,6 +245,8 @@ void commit_start(BlockDriverState *bs, BlockDriverState *base,
     s->base_flags          = orig_base_flags;
     s->orig_overlay_flags  = orig_overlay_flags;
 
+    s->backing_file_str = g_strdup(backing_file_str);
+
     s->on_error = on_error;
     s->common.co = qemu_coroutine_create(commit_run);
 
diff --git a/blockdev.c b/blockdev.c
index 50d4890..7a97b7f 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1918,6 +1918,7 @@ void qmp_block_commit(const char *device,
                       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_backing_file, const char *backing_file,
                       bool has_speed, int64_t speed,
                       Error **errp)
 {
@@ -2009,11 +2010,16 @@ void qmp_block_commit(const char *device,
     }
 
     if (top_bs == bs) {
+        if (has_backing_file) {
+            error_setg(errp, "'backing-file' specified,"
+                             " but 'top' is the active layer");
+            return;
+        }
         commit_active_start(bs, base_bs, speed, on_error, block_job_cb,
                             bs, &local_err);
     } else {
         commit_start(bs, base_bs, top_bs, speed, on_error, block_job_cb, bs,
-                    &local_err);
+                     has_backing_file ? backing_file : NULL, &local_err);
     }
     if (local_err != NULL) {
         error_propagate(errp, local_err);
diff --git a/include/block/block.h b/include/block/block.h
index 4dc68be..c07a85f 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -289,7 +289,8 @@ int bdrv_change_backing_file(BlockDriverState *bs,
     const char *backing_file, const char *backing_fmt);
 void bdrv_register(BlockDriver *bdrv);
 int bdrv_drop_intermediate(BlockDriverState *active, BlockDriverState *top,
-                           BlockDriverState *base);
+                           BlockDriverState *base,
+                           const char *backing_file_str);
 BlockDriverState *bdrv_find_overlay(BlockDriverState *active,
                                     BlockDriverState *bs);
 BlockDriverState *bdrv_find_base(BlockDriverState *bs);
diff --git a/include/block/block_int.h b/include/block/block_int.h
index f2e753f..87c2e02 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -436,13 +436,14 @@ void stream_start(BlockDriverState *bs, BlockDriverState *base,
  * @on_error: The action to take upon error.
  * @cb: Completion function for the job.
  * @opaque: Opaque pointer value passed to @cb.
+ * @backing_file_str: String to use as the backing file in @top's overlay
  * @errp: Error object.
  *
  */
 void commit_start(BlockDriverState *bs, BlockDriverState *base,
                  BlockDriverState *top, int64_t speed,
                  BlockdevOnError on_error, BlockDriverCompletionFunc *cb,
-                 void *opaque, Error **errp);
+                 void *opaque, const char *backing_file_str, Error **errp);
 /**
  * commit_active_start:
  * @bs: Active block device to be committed.
diff --git a/qapi-schema.json b/qapi-schema.json
index 67a627f..b449aec 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -2121,6 +2121,23 @@
 #                           topmost data to be committed down.
 #                           (Since 2.1)
 #
+# @backing-file:  #optional The backing file string to write into the overlay
+#                           image of 'top'.  If 'top' is the active layer,
+#                           specifying a backing file string is an error. This
+#                           filename is not validated.
+#
+#                           If a pathname string is such that it cannot be
+#                           resolved by QEMU, that means that subsequent QMP or
+#                           HMP commands must use node-names for the image in
+#                           question, as filename lookup methods will fail.
+#
+#                           If not specified, QEMU will automatically determine
+#                           the backing file string to use, or error out if
+#                           there is no obvious choice. Care should be taken
+#                           when specifying the string, to specify a valid
+#                           filename or protocol.
+#                           (Since 2.1)
+#
 #                    If top == base, that is an error.
 #                    If top == active, the job will not be completed by itself,
 #                    user needs to complete the job with the block-job-complete
@@ -2133,7 +2150,6 @@
 #                    size of the smaller top, you can safely truncate it
 #                    yourself once the commit operation successfully completes.
 #
-#
 # @speed:  #optional the maximum speed, in bytes per second
 #
 # Returns: Nothing on success
@@ -2144,13 +2160,16 @@
 #          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
+#          If @top or @top-node-name is the active layer, and @backing-file is
+#             specified, GenericError
 #
 # Since: 1.3
 #
 ##
 { 'command': 'block-commit',
   'data': { 'device': 'str', '*base': 'str', '*base-node-name': 'str',
-            '*top': 'str', '*top-node-name': 'str', '*speed': 'int' } }
+            '*top': 'str', '*top-node-name': 'str', '*backing-file': 'str',
+            '*speed': 'int' } }
 
 ##
 # @drive-backup
diff --git a/qmp-commands.hx b/qmp-commands.hx
index c61f4cb..1014b68 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -985,7 +985,7 @@ EQMP
 
     {
         .name       = "block-commit",
-        .args_type  = "device:B,base:s?,base-node-name:s?,top:s?,top-node-name:s?,speed:o?",
+        .args_type  = "device:B,base:s?,base-node-name:s?,top:s?,top-node-name:s?,backing-file:s?,speed:o?",
         .mhandler.cmd_new = qmp_marshal_input_block_commit,
     },
 
@@ -1021,6 +1021,23 @@ neither is specified, this is the active layer
                     topmost data to be committed down.
                     (json-string, optional) (Since 2.1)
 
+- backing-file:     The backing file string to write into the overlay
+                    image of 'top'.  If 'top' is the active layer,
+                    specifying a backing file string is an error. This
+                    filename is not validated.
+
+                    If a pathname string is such that it cannot be
+                    resolved by QEMU, that means that subsequent QMP or
+                    HMP commands must use node-names for the image in
+                    question, as filename lookup methods will fail.
+
+                    If not specified, QEMU will automatically determine
+                    the backing file string to use, or error out if
+                    there is no obvious choice. Care should be taken
+                    when specifying the string, to specify a valid
+                    filename or protocol.
+                    (json-string, optional) (Since 2.1)
+
           If top == base, that is an error.
           If top == active, the job will not be completed by itself,
           user needs to complete the job with the block-job-complete
-- 
1.9.3

[Qemu-devel] [PATCH v4 07/10] block: add ability for block-stream to use node-name

[Jeff Cody]

This adds the ability for block-stream to use node-name arguments
for base, to specify the backing image to stream from.

Both 'base' and 'base-node-name' are optional, but mutually exclusive.
Either can be specified, but not both together.

The argument for "device" is now optional as well, so long as
base-node-name is specified.  From the node-name, we can determine
the full device chain.

Signed-off-by: Jeff Cody <jcody@redhat.com>
---
 blockdev.c       | 48 +++++++++++++++++++++++++++++++++++++++---------
 hmp.c            |  2 +-
 qapi-schema.json | 16 ++++++++++++----
 qmp-commands.hx  |  2 +-
 4 files changed, 53 insertions(+), 15 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 7a97b7f..16b44f7 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1872,38 +1872,68 @@ static void block_job_cb(void *opaque, int ret)
     bdrv_put_ref_bh_schedule(bs);
 }
 
-void qmp_block_stream(const char *device, bool has_base,
-                      const char *base, bool has_speed, int64_t speed,
+void qmp_block_stream(const char *device,
+                      bool has_base, const char *base,
+                      bool has_base_node_name, const char *base_node_name,
+                      bool has_speed, int64_t speed,
                       bool has_on_error, BlockdevOnError on_error,
                       Error **errp)
 {
-    BlockDriverState *bs;
+    BlockDriverState *bs = NULL;
     BlockDriverState *base_bs = NULL;
+    BlockDriverState *tmp_bs;
     Error *local_err = NULL;
+    const char *base_name = NULL;
 
     if (!has_on_error) {
         on_error = BLOCKDEV_ON_ERROR_REPORT;
     }
 
+    if (has_base && has_base_node_name) {
+        error_setg(errp, "'base' and 'base-node-name' are mutually exclusive");
+        return;
+    }
+
     bs = bdrv_find(device);
     if (!bs) {
         error_set(errp, QERR_DEVICE_NOT_FOUND, device);
         return;
     }
 
+    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;
+        }
+        tmp_bs = bdrv_find_overlay(bs, base_bs);
+        if (tmp_bs) {
+            base_name = tmp_bs->backing_file;
+        }
+    }
+
     if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_STREAM, errp)) {
         return;
     }
 
-    if (base) {
+    if (has_base) {
         base_bs = bdrv_find_backing_image(bs, base);
-        if (base_bs == NULL) {
-            error_set(errp, QERR_BASE_NOT_FOUND, base);
-            return;
-        }
+        base_name = base;
+    }
+
+    if (base_bs == NULL && (has_base || has_base_node_name)) {
+        error_set(errp, QERR_BASE_NOT_FOUND, base);
+        return;
+    }
+
+    /* Verify that 'base' is in the same chain as 'bs', if 'base' was
+     * specified */
+    if (base_bs && !bdrv_chain_contains(bs, base_bs)) {
+        error_setg(errp, "'device' and 'base' are not in the same chain");
+        return;
     }
 
-    stream_start(bs, base_bs, base, has_speed ? speed : 0,
+    stream_start(bs, base_bs, base_name, has_speed ? speed : 0,
                  on_error, block_job_cb, bs, &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
diff --git a/hmp.c b/hmp.c
index ccc35d4..bb934df 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1173,7 +1173,7 @@ void hmp_block_stream(Monitor *mon, const QDict *qdict)
     int64_t speed = qdict_get_try_int(qdict, "speed", 0);
 
     qmp_block_stream(device, base != NULL, base,
-                     qdict_haskey(qdict, "speed"), speed,
+                     false, NULL, qdict_haskey(qdict, "speed"), speed,
                      true, BLOCKDEV_ON_ERROR_REPORT, &error);
 
     hmp_handle_error(mon, &error);
diff --git a/qapi-schema.json b/qapi-schema.json
index b449aec..2cdd96e 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -2599,9 +2599,17 @@
 # On successful completion the image file is updated to drop the backing file
 # and the BLOCK_JOB_COMPLETED event is emitted.
 #
-# @device: the device name
+# @device:                  The device name.
+#
+# For 'base', either @base or @base-node-name may be set but not both. If
+# neither is specified, the entire chain will be streamed into the active image,
+# and the chain will consist of a single image (the current active layer) with
+# no backing file.
 #
-# @base:   #optional the common backing file name
+# @base:           #optional the common backing file name
+#
+# @base-node-name: #optional the block driver state node name of the
+#                            common backing file.  (Since 2.1)
 #
 # @speed:  #optional the maximum speed, in bytes per second
 #
@@ -2615,8 +2623,8 @@
 # Since: 1.1
 ##
 { 'command': 'block-stream',
-  'data': { 'device': 'str', '*base': 'str', '*speed': 'int',
-            '*on-error': 'BlockdevOnError' } }
+  'data': { 'device': 'str', '*base': 'str', '*base-node-name': 'str',
+            '*speed': 'int', '*on-error': 'BlockdevOnError' } }
 
 ##
 # @block-job-set-speed:
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 1014b68..9ddc39a 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -979,7 +979,7 @@ EQMP
 
     {
         .name       = "block-stream",
-        .args_type  = "device:B,base:s?,speed:o?,on-error:s?",
+        .args_type  = "device:B,base:s?,base-node-name:s?,speed:o?,on-error:s?",
         .mhandler.cmd_new = qmp_marshal_input_block_stream,
     },
 
-- 
1.9.3

[Qemu-devel] [PATCH v4 08/10] block: add backing-file option to block-stream

[Jeff Cody]

On some image chains, QEMU may not always be able to resolve the
filenames properly, when updating the backing file of an image
after a block job.

For instance, certain relative pathnames may fail, or drives may
have been specified originally by file descriptor (e.g. /dev/fd/???),
or a relative protocol pathname may have been used.

In these instances, QEMU may lack the information to be able to make
the correct choice, but the user or management layer most likely does
have that knowledge.

With this extension to the block-stream api, the user is able to change
the backing file of the active layer as part of the block-stream
operation.

This allows the change to be 'safe', in the sense that if the attempt
to write the active image metadata fails, then the block-stream
operation returns failure, without disrupting the guest.

If a backing file string is not specified in the command, the backing
file string to use is determined in the same manner as it was
previously.

Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Jeff Cody <jcody@redhat.com>
---
 block/stream.c   | 11 +++++------
 blockdev.c       | 12 ++++++++++++
 hmp.c            |  3 ++-
 qapi-schema.json | 18 +++++++++++++++++-
 qmp-commands.hx  |  2 +-
 5 files changed, 37 insertions(+), 9 deletions(-)

diff --git a/block/stream.c b/block/stream.c
index 91d18a2..1c7f0a0 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -32,7 +32,7 @@ typedef struct StreamBlockJob {
     RateLimit limit;
     BlockDriverState *base;
     BlockdevOnError on_error;
-    char backing_file_id[1024];
+    char *backing_file_str;
 } StreamBlockJob;
 
 static int coroutine_fn stream_populate(BlockDriverState *bs,
@@ -186,7 +186,7 @@ wait:
     if (!block_job_is_cancelled(&s->common) && sector_num == end && ret == 0) {
         const char *base_id = NULL, *base_fmt = NULL;
         if (base) {
-            base_id = s->backing_file_id;
+            base_id = s->backing_file_str;
             if (base->drv) {
                 base_fmt = base->drv->format_name;
             }
@@ -196,6 +196,7 @@ wait:
     }
 
     qemu_vfree(buf);
+    g_free(s->backing_file_str);
     block_job_completed(&s->common, ret);
 }
 
@@ -217,7 +218,7 @@ static const BlockJobDriver stream_job_driver = {
 };
 
 void stream_start(BlockDriverState *bs, BlockDriverState *base,
-                  const char *base_id, int64_t speed,
+                  const char *backing_file_str, int64_t speed,
                   BlockdevOnError on_error,
                   BlockDriverCompletionFunc *cb,
                   void *opaque, Error **errp)
@@ -237,9 +238,7 @@ void stream_start(BlockDriverState *bs, BlockDriverState *base,
     }
 
     s->base = base;
-    if (base_id) {
-        pstrcpy(s->backing_file_id, sizeof(s->backing_file_id), base_id);
-    }
+    s->backing_file_str = g_strdup(backing_file_str);
 
     s->on_error = on_error;
     s->common.co = qemu_coroutine_create(stream_run);
diff --git a/blockdev.c b/blockdev.c
index 16b44f7..23a76eb 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1875,6 +1875,7 @@ static void block_job_cb(void *opaque, int ret)
 void qmp_block_stream(const char *device,
                       bool has_base, const char *base,
                       bool has_base_node_name, const char *base_node_name,
+                      bool has_backing_file, const char *backing_file,
                       bool has_speed, int64_t speed,
                       bool has_on_error, BlockdevOnError on_error,
                       Error **errp)
@@ -1926,6 +1927,9 @@ void qmp_block_stream(const char *device,
         return;
     }
 
+    /* backing_file string overrides base bs filename */
+    base_name = has_backing_file ? backing_file : base_name;
+
     /* Verify that 'base' is in the same chain as 'bs', if 'base' was
      * specified */
     if (base_bs && !bdrv_chain_contains(bs, base_bs)) {
@@ -1933,6 +1937,14 @@ void qmp_block_stream(const char *device,
         return;
     }
 
+    /* if we are streaming the entire chain, the result will have no backing
+     * file, and specifying one is therefore an error */
+    if (base_bs == NULL && has_backing_file) {
+        error_setg(errp, "backing file specified, but streaming the "
+                         "entire chain");
+        return;
+    }
+
     stream_start(bs, base_bs, base_name, has_speed ? speed : 0,
                  on_error, block_job_cb, bs, &local_err);
     if (local_err) {
diff --git a/hmp.c b/hmp.c
index bb934df..644e854 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1173,7 +1173,8 @@ void hmp_block_stream(Monitor *mon, const QDict *qdict)
     int64_t speed = qdict_get_try_int(qdict, "speed", 0);
 
     qmp_block_stream(device, base != NULL, base,
-                     false, NULL, qdict_haskey(qdict, "speed"), speed,
+                     false, NULL, false, NULL,
+                     qdict_haskey(qdict, "speed"), speed,
                      true, BLOCKDEV_ON_ERROR_REPORT, &error);
 
     hmp_handle_error(mon, &error);
diff --git a/qapi-schema.json b/qapi-schema.json
index 2cdd96e..3400561 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -2611,6 +2611,21 @@
 # @base-node-name: #optional the block driver state node name of the
 #                            common backing file.  (Since 2.1)
 #
+# @backing-file: #optional The backing file string to write into the active
+#                          layer. This filename is not validated.
+#
+#                          If a pathname string is such that it cannot be
+#                          resolved by QEMU, that means that subsequent QMP or
+#                          HMP commands must use node-names for the image in
+#                          question, as filename lookup methods will fail.
+#
+#                          If not specified, QEMU will automatically determine
+#                          the backing file string to use, or error out if there
+#                          is no obvious choice.  Care should be taken when
+#                          specifying the string, to specify a valid filename or
+#                          protocol.
+#                          (Since 2.1)
+#
 # @speed:  #optional the maximum speed, in bytes per second
 #
 # @on-error: #optional the action to take on an error (default report).
@@ -2624,7 +2639,8 @@
 ##
 { 'command': 'block-stream',
   'data': { 'device': 'str', '*base': 'str', '*base-node-name': 'str',
-            '*speed': 'int', '*on-error': 'BlockdevOnError' } }
+            '*backing-file': 'str', '*speed': 'int',
+            '*on-error': 'BlockdevOnError' } }
 
 ##
 # @block-job-set-speed:
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 9ddc39a..b41af0f 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -979,7 +979,7 @@ EQMP
 
     {
         .name       = "block-stream",
-        .args_type  = "device:B,base:s?,base-node-name:s?,speed:o?,on-error:s?",
+        .args_type  = "device:B,base:s?,base-node-name:s?,speed:o?,backing-file:s?,on-error:s?",
         .mhandler.cmd_new = qmp_marshal_input_block_stream,
     },
 
-- 
1.9.3

[Qemu-devel] [PATCH v4 09/10] block: Add QMP documentation for block-stream

[Jeff Cody]

The QMP command 'block-stream' was missing QMP documentation.  Add
that documentation.

Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Jeff Cody <jcody@redhat.com>
---
 qmp-commands.hx | 58 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 58 insertions(+)

diff --git a/qmp-commands.hx b/qmp-commands.hx
index b41af0f..69d29ae 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -983,6 +983,64 @@ EQMP
         .mhandler.cmd_new = qmp_marshal_input_block_stream,
     },
 
+SQMP
+block-stream
+------------
+
+Copy data from a backing file into a block device.
+
+The block streaming operation is performed in the background until the entire
+backing file has been copied.  This command returns immediately once streaming
+has started.  The status of ongoing block streaming operations can be checked
+with query-block-jobs.  The operation can be stopped before it has completed
+using the block-job-cancel command.
+
+If a base file is specified then sectors are not copied from that base file and
+its backing chain.  When streaming completes the image file will have the base
+file as its backing file.  This can be used to stream a subset of the backing
+file chain instead of flattening the entire image.
+
+On successful completion the image file is updated to drop the backing file
+and the BLOCK_JOB_COMPLETED event is emitted.
+
+- "device":         The device name.
+                    (json-string)
+
+For base, either 'base' or 'base-node-name' may be set but not both. If
+neither is specified, the entire chain will be streamed into the active image,
+and the chain will consist of a single image (the current active layer) with
+no backing file.
+
+- "base":           The common backing file name.
+                    (json-string, optional)
+
+- "base-node-name": The block driver state node name of the common backing file.
+                    (json-string, optional) (Since 2.1)
+
+- "backing-file":   The backing file string to write into the active layer.
+                    This filename is not validated.
+
+                    If a pathname string is such that it cannot be resolved by
+                    QEMU, that means that subsequent QMP or HMP commands must
+                    use node-names for the image in question, as filename
+                    lookup methods will fail.
+
+                    If not specified, QEMU will automatically determine the
+                    backing file string to use, or error out if there is no
+                    obvious choice.  Care should be taken when specifying the
+                    string, to specify a valid filename or protocol.
+                    (json-string, optional)
+                    (Since 2.1)
+
+- "speed":          The maximum speed, in bytes per second.
+                    (json-int, optional)
+
+- "on-error":       The action to take on an error (default report).
+                    'stop' and 'enospc' can only be used if the block device
+                    supports io-status (see BlockInfo).
+                    (json-enum, optional) (Since 1.3)
+EQMP
+
     {
         .name       = "block-commit",
         .args_type  = "device:B,base:s?,base-node-name:s?,top:s?,top-node-name:s?,backing-file:s?,speed:o?",
-- 
1.9.3

[Qemu-devel] [PATCH v4 10/10] block: add QAPI command to allow live backing file change

[Jeff Cody]

This allows a user to make a live change to the backing file recorded in
an open image.

The image file to modify can be specified 2 ways:

1) image filename
2) image node-name

Note: this does not cause the backing file itself to be reopened; it
merely changes the backing filename in the image file structure, and
in internal BDS structures.

It is the responsibility of the user to pass a filename string that
can be resolved when the image chain is reopened, and the filename
string is not validated.

A good analogy for this command is that it is a live version of
'qemu-img rebase -u', with respect to changing the backing file string.

Signed-off-by: Jeff Cody <jcody@redhat.com>
---
 blockdev.c       | 102 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 qapi-schema.json |  60 ++++++++++++++++++++++++++++++++
 qmp-commands.hx  |  74 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 236 insertions(+)

diff --git a/blockdev.c b/blockdev.c
index 23a76eb..61150b4 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2408,6 +2408,108 @@ void qmp_block_job_complete(const char *device, Error **errp)
     block_job_complete(job, errp);
 }
 
+void qmp_change_backing_file(const char *device,
+                             bool has_image, const char *image,
+                             bool has_image_node_name,
+                             const char *image_node_name,
+                             const char *backing_file,
+                             Error **errp)
+{
+    BlockDriverState *bs = NULL;
+    BlockDriverState *image_bs = NULL;
+    Error *local_err = NULL;
+    bool ro;
+    int open_flags;
+    int ret;
+
+    /* validate argument combinations */
+    if (has_image && has_image_node_name) {
+        error_setg(errp, "'image' and 'image-node-name' "
+                         "are mutually exclusive");
+        return;
+    }
+
+    /* find the top layer BDS of the chain */
+    bs = bdrv_find(device);
+    if (!bs) {
+        error_set(errp, QERR_DEVICE_NOT_FOUND, device);
+        return;
+    }
+
+    if (has_image_node_name) {
+        image_bs = bdrv_lookup_bs(NULL, image_node_name, &local_err);
+        if (local_err) {
+            error_propagate(errp, local_err);
+            return;
+        }
+    }
+
+    if (has_image) {
+        if (!strcmp(bs->filename, image)) {
+            image_bs = bs;
+        } else {
+            image_bs = bdrv_find_backing_image(bs, image);
+        }
+    }
+
+    if (!has_image && !has_image_node_name) {
+        image_bs = bs;
+    }
+
+    if (!image_bs) {
+        error_setg(errp, "image file not found");
+        return;
+    }
+
+    if (bdrv_find_base(image_bs) == image_bs) {
+        error_setg(errp, "not allowing backing file change on an image "
+                         "without a backing file");
+        return;
+    }
+
+    /* even though we are not necessarily operating on bs, we need it to
+     * determine if block ops are currently prohibited on the chain */
+    if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_CHANGE, errp)) {
+        return;
+    }
+
+    /* final sanity check */
+    if (!bdrv_chain_contains(bs, image_bs)) {
+        error_setg(errp, "'%s' and image file are not in the same chain",
+                   device);
+        return;
+    }
+
+    /* if not r/w, reopen to make r/w */
+    open_flags = image_bs->open_flags;
+    ro = bdrv_is_read_only(image_bs);
+
+    if (ro) {
+        bdrv_reopen(image_bs, open_flags | BDRV_O_RDWR, &local_err);
+        if (local_err) {
+            error_propagate(errp, local_err);
+            return;
+        }
+    }
+
+    ret = bdrv_change_backing_file(image_bs, backing_file,
+                               image_bs->drv ? image_bs->drv->format_name : "");
+
+    if (ret < 0) {
+        error_setg_errno(errp, -ret, "Could not change backing file to '%s'",
+                         backing_file);
+        /* don't exit here, so we can try to restore open flags if
+         * appropriate */
+    }
+
+    if (ro) {
+        bdrv_reopen(image_bs, open_flags, &local_err);
+        if (local_err) {
+            error_propagate(errp, local_err); /* will preserve prior errp */
+        }
+    }
+}
+
 void qmp_blockdev_add(BlockdevOptions *options, Error **errp)
 {
     QmpOutputVisitor *ov = qmp_output_visitor_new();
diff --git a/qapi-schema.json b/qapi-schema.json
index 3400561..e57396b 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -2092,6 +2092,66 @@
   'returns': 'str' }
 
 ##
+# @change-backing-file
+#
+# Change the backing file in the image file metadata.  This does not cause QEMU
+# to reopen the image file to reparse the backing filename (it may, however,
+# perform a reopen to change permissions from r/o -> r/w -> r/o, if needed).
+# The new backing file string is written into the image file metadata, and the
+# QEMU internal strings are updated.
+#
+# The image file to perform the operation on can be specified by two different
+# methods:
+#
+#  Method 1: Supply the device name (e.g. 'virtio0'), and optionally the image
+#            filename.  This would use arguments @device and @image.
+#
+#  Method 2: Supply the device name, and the node-name of the image to modify,
+#            via @image-node-name.
+#
+# Arguments @image and @image-node-name are mutually exclusive.
+#
+# Method 1 interface
+#---------------------
+# @image:          #optional The file name of the image to modify.  If omitted,
+#                            and @image-node-name is not supplied, then the
+#                            default is the active layer of the chain described
+#                            by @device.
+#
+# Method 2 interface
+#---------------------
+# @image-node-name #optional The name of the block driver state node of the
+#                            image to modify.  The @device argumen is used to
+#                            verify @image-node-name is in the chain described
+#                            by @device.
+#
+# Common arguments
+#---------------------
+# @device:          The name of the device.
+#
+# @backing-file:    The string to write as the backing file.  This string is
+#                   not validated, so care should be taken when specifying
+#                   the string or the image chain may not be able to be
+#                   reopened again.
+#
+#                   If a pathname string is such that it cannot be
+#                   resolved by QEMU, that means that subsequent QMP or
+#                   HMP commands must use node-names for the image in
+#                   question, as filename lookup methods will fail.
+#
+#
+# Returns: Nothing on success
+#          If @device does not exist or cannot be determined, DeviceNotFound
+#          If @image is specified, but not @device, GenericError
+#          If both @image and @image-node-name are specified, GenericError
+#
+# Since: 2.1
+##
+{ 'command': 'change-backing-file',
+  'data': { 'device': 'str', '*image': 'str', '*image-node-name': 'str',
+            'backing-file': 'str' } }
+
+##
 # @block-commit
 #
 # Live commit of data from overlay image nodes into backing nodes - i.e.,
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 69d29ae..a17d3d5 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1438,6 +1438,80 @@ Example:
 EQMP
 
     {
+        .name       = "change-backing-file",
+        .args_type  = "device:s,image:s?,image-node-name:s?,backing-file:s",
+        .mhandler.cmd_new = qmp_marshal_input_change_backing_file,
+    },
+
+SQMP
+change-backing-file
+-------------------
+Since: 2.1
+
+Change the backing file in the image file metadata.  This does not cause QEMU
+to reopen the image file to reparse the backing filename (it may, however,
+perform a reopen to change permissions from r/o -> r/w -> r/o, if needed).
+The new backing file string is written into the image file metadata, and the
+QEMU internal strings are updated.
+
+The image file to perform the operation on can be specified by two different
+methods:
+
+ Method 1: Supply the device name (e.g. 'virtio0'), and optionally the image
+           filename.  This would use arguments "device" and "image".
+
+ Method 2: Supply the device name, and the node-name of the image to modify,
+           via "image-node-name".
+
+Arguments:
+
+Arguments "image" or "image-node-name" are mutually exclusive.
+
+
+Method 1 interface
+--------------------
+- "image":              The file name of the image to modify.  If omitted,
+                        and "image-node-name" is not supplied, then the
+                        default is the active layer of the chain described
+                        by device.
+                        (json-string, optional)
+
+
+Method 2 interface
+--------------------
+- "image-node-name":    The name of the block driver state node of the
+                        image to modify.  The "device" is argument is used to
+                        verify "image-node-name" is in the chain described by
+                        "device".
+                        (json-string, optional)
+
+
+Common arguments
+--------------------
+- "device":             The name of the device.
+                        (json-string)
+
+- "backing-file":       The string to write as the backing file.  This string is
+                        not validated, so care should be taken when specifying
+                        the string or the image chain may not be able to be
+                        reopened again.
+                        (json-string)
+
+                        If a pathname string is such that it cannot be
+                        resolved by QEMU, that means that subsequent QMP or
+                        HMP commands must use node-names for the image in
+                        question, as filename lookup methods will fail.
+
+
+Returns: Nothing on success
+         If "device" does not exist or cannot be determined, DeviceNotFound
+         If "image" is specified, but not "device, GenericError
+         If both "image" and "image-node-name" are specified, GenericError
+
+
+EQMP
+
+    {
         .name       = "balloon",
         .args_type  = "value:M",
         .mhandler.cmd_new = qmp_marshal_input_balloon,
-- 
1.9.3

Re: [Qemu-devel] [PATCH v4 03/10] block: simplify bdrv_find_base() and bdrv_find_overlay()

[Eric Blake]

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

On 06/04/2014 07:51 AM, Jeff Cody wrote:
> This simplifies the function bdrv_find_overlay().  With this change,
> bdrv_find_base() is just a subset of usage of bdrv_find_overlay(),
> so this also take advantage of that.

s/take/takes/

> 
> Signed-off-by: Jeff Cody <jcody@redhat.com>
> ---
>  block.c | 45 ++++++++++-----------------------------------
>  1 file changed, 10 insertions(+), 35 deletions(-)
> 

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

-- 
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 v4 04/10] block: make 'top' argument to block-commit optional

[Eric Blake]

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

On 06/04/2014 07:51 AM, Jeff Cody wrote:
> Now that active layer block-commit is supported, the 'top' argument
> no longer needs to be mandatory.
> 
> Change it to optional, with the default being the active layer in the
> device chain.
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Reviewed-by: Benoit Canet <benoit@irqsave.net>
> Signed-off-by: Jeff Cody <jcody@redhat.com>
> ---

Unrelated to my review, but I wish we had done a better job at making
the qemu 2.0 addition of active commit introspectible.  Had we made
_this_ patch at the same time, introspection would be possible by
creating a dummy blockdev, then attempting a block-commit that omits the
'top' argument (since the error message for a missing required argument
[old qemu] is different than the message for a blockdev that can't be
committed [new qemu]).  But since this change is in a different release
than where we supported active commit, I'm stuck coming up with some
reliable way to do a probe of whether active commit is supported so that
libvirt knows whether to expose active commit on to the end user.

-- 
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 v4 05/10] block: Accept node-name arguments for block-commit

[Eric Blake]

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

On 06/04/2014 07:51 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.

Stale paragraph; just delete it.  [We may later want to add patches to
make device optional, if we can figure out a solution for what to do
when a BDS has more than one overlay; but that doesn't need to stall
this series]

> 
> 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       | 55 +++++++++++++++++++++++++++++++++++++++++++++++--------
>  qapi-schema.json | 37 +++++++++++++++++++++++++++----------
>  qmp-commands.hx  | 31 +++++++++++++++++++++++--------
>  3 files changed, 97 insertions(+), 26 deletions(-)
> 

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

An alternative would have been to validate that both 'base' and
'base-node-name' resolve to the same BDS, but I like the simplicity of
erroring out here.

> +
> +    if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_COMMIT, errp)) {
> +        return;
> +    }

[unrelated to this commit - are we sure that the semantics of the commit
blocker are correct? If we implement BDS reuse among multiple chains,
consider what happens if I have:
       / sn1 <- sn2
base <
       \ file3

Even if there is no other block job operating on sn2 or its chain, we
really want to block an attempt to merge file3 into base, as modifying
base would corrupt both sn1 and sn2.  That is, a BDS being a common
backing between more than one chain should automatically behave as an
op-blocker for commits - where the block is what you are committing
into, not where you are committing from.]

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

-- 
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 v4 04/10] block: make 'top' argument to block-commit optional

[Jeff Cody]

On Wed, Jun 04, 2014 at 02:59:44PM -0600, Eric Blake wrote:
> On 06/04/2014 07:51 AM, Jeff Cody wrote:
> > Now that active layer block-commit is supported, the 'top' argument
> > no longer needs to be mandatory.
> > 
> > Change it to optional, with the default being the active layer in the
> > device chain.
> > 
> > Reviewed-by: Eric Blake <eblake@redhat.com>
> > Reviewed-by: Benoit Canet <benoit@irqsave.net>
> > Signed-off-by: Jeff Cody <jcody@redhat.com>
> > ---
> 
> Unrelated to my review, but I wish we had done a better job at making
> the qemu 2.0 addition of active commit introspectible.  Had we made
> _this_ patch at the same time, introspection would be possible by
> creating a dummy blockdev, then attempting a block-commit that omits the
> 'top' argument (since the error message for a missing required argument
> [old qemu] is different than the message for a blockdev that can't be
> committed [new qemu]).  But since this change is in a different release
> than where we supported active commit, I'm stuck coming up with some
> reliable way to do a probe of whether active commit is supported so that
> libvirt knows whether to expose active commit on to the end user.
>

Well, I think what we _really_ need is true QAPI introspection.

Unfortunately, libvirt can't rely on the QEMU version to determine API
levels. This leaves it needing to do various QAPI contortions to
figure out what is supported and what is not supported, which seems
clunky and probably not sustainable long-term.

But I agree with the gist of what you saying, I think: libvirt and
QEMU should make sure to stay in the loop with each other on
command discoverability.

With that said, 2.1 soft freeze is not that far off; what other QAPI
commands might we be saying this about once 2.1 is out?


-Jeff

Re: [Qemu-devel] [PATCH v4 05/10] block: Accept node-name arguments for block-commit

[Jeff Cody]

On Wed, Jun 04, 2014 at 03:38:55PM -0600, Eric Blake wrote:
> On 06/04/2014 07:51 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.
> 
> Stale paragraph; just delete it.  [We may later want to add patches to
> make device optional, if we can figure out a solution for what to do
> when a BDS has more than one overlay; but that doesn't need to stall
> this series]

Argh, yes.  Forgot to update the commit message here, thanks.  If a v5
is needed, I'll delete it, otherwise perhaps a kind maintainer could
fix it up for me before applying :)

> 
> > 
> > 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       | 55 +++++++++++++++++++++++++++++++++++++++++++++++--------
> >  qapi-schema.json | 37 +++++++++++++++++++++++++++----------
> >  qmp-commands.hx  | 31 +++++++++++++++++++++++--------
> >  3 files changed, 97 insertions(+), 26 deletions(-)
> > 
> 
> > -    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;
> > +    }
> 
> An alternative would have been to validate that both 'base' and
> 'base-node-name' resolve to the same BDS, but I like the simplicity of
> erroring out here.
> 
> > +
> > +    if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_COMMIT, errp)) {
> > +        return;
> > +    }
> 
> [unrelated to this commit - are we sure that the semantics of the commit
> blocker are correct? If we implement BDS reuse among multiple chains,
> consider what happens if I have:
>        / sn1 <- sn2
> base <
>        \ file3
> 
> Even if there is no other block job operating on sn2 or its chain, we
> really want to block an attempt to merge file3 into base, as modifying
> base would corrupt both sn1 and sn2.  That is, a BDS being a common
> backing between more than one chain should automatically behave as an
> op-blocker for commits - where the block is what you are committing
> into, not where you are committing from.]


No, the commit blocker likely won't catch everything.  But that is
because there are additional enhancements the blocker code needs (it
is ultimately intended to block on an individual BDS, but in reality
currently operates on the active layer BDS for the whole chain).

For every block job, the appropriate blockers need to be set on each
BDS affected by the job (intermediate images, overlay images in some
cases, the target, etc..).

It currently functions like the older in_use flag (blocks a chain as
referenced by the active layer), but the groundwork is in place for
truly independent BDS blockers to come later.

(For more discussion, see the qemu-devel list replies to:
[PATCH v20 04/15] block: Move op_blocker check from block_job_create to its caller)

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

Thanks!

Re: [Qemu-devel] [PATCH v4 07/10] block: add ability for block-stream to use node-name

[Eric Blake]

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

On 06/04/2014 07:51 AM, Jeff Cody wrote:
> This adds the ability for block-stream to use node-name arguments
> for base, to specify the backing image to stream from.
> 
> Both 'base' and 'base-node-name' are optional, but mutually exclusive.
> Either can be specified, but not both together.
> 
> The argument for "device" is now optional as well, so long as
> base-node-name is specified.  From the node-name, we can determine
> the full device chain.

Another stale paragraph worth deleting.

> 
> Signed-off-by: Jeff Cody <jcody@redhat.com>
> ---
>  blockdev.c       | 48 +++++++++++++++++++++++++++++++++++++++---------
>  hmp.c            |  2 +-
>  qapi-schema.json | 16 ++++++++++++----
>  qmp-commands.hx  |  2 +-
>  4 files changed, 53 insertions(+), 15 deletions(-)
> 

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

-- 
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 v4 10/10] block: add QAPI command to allow live backing file change

[Eric Blake]

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

On 06/04/2014 07:51 AM, Jeff Cody wrote:
> This allows a user to make a live change to the backing file recorded in
> an open image.
> 
> The image file to modify can be specified 2 ways:
> 
> 1) image filename
> 2) image node-name
> 
> Note: this does not cause the backing file itself to be reopened; it
> merely changes the backing filename in the image file structure, and
> in internal BDS structures.
> 
> It is the responsibility of the user to pass a filename string that
> can be resolved when the image chain is reopened, and the filename
> string is not validated.
> 
> A good analogy for this command is that it is a live version of
> 'qemu-img rebase -u', with respect to changing the backing file string.
> 
> Signed-off-by: Jeff Cody <jcody@redhat.com>
> ---
>  blockdev.c       | 102 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  qapi-schema.json |  60 ++++++++++++++++++++++++++++++++
>  qmp-commands.hx  |  74 ++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 236 insertions(+)
> 


> +#
> +# Method 2 interface
> +#---------------------
> +# @image-node-name #optional The name of the block driver state node of the
> +#                            image to modify.  The @device argumen is used to

s/argumen/argument/

Unless there is any other reason for a respin, I'm hoping the committer
can fix the typo, and add:

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

-- 
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 v4 10/10] block: add QAPI command to allow live backing file change

[Benoît Canet]

The Wednesday 04 Jun 2014 à 09:51:12 (-0400), Jeff Cody wrote :
> This allows a user to make a live change to the backing file recorded in
> an open image.
> 
> The image file to modify can be specified 2 ways:
> 
> 1) image filename
> 2) image node-name
> 
> Note: this does not cause the backing file itself to be reopened; it
> merely changes the backing filename in the image file structure, and
> in internal BDS structures.
> 
> It is the responsibility of the user to pass a filename string that
> can be resolved when the image chain is reopened, and the filename
> string is not validated.
> 
> A good analogy for this command is that it is a live version of
> 'qemu-img rebase -u', with respect to changing the backing file string.

If this command was triggering the reopen of the file and drive-mirror was accepting
a node-name argument we would have a way to manage tiered storage.

For example we could move one of the backing file in the chain from harddisk to
SSD by mirroring it and swapping the new one in place.

Best regards

Benoît

> 
> Signed-off-by: Jeff Cody <jcody@redhat.com>
> ---
>  blockdev.c       | 102 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  qapi-schema.json |  60 ++++++++++++++++++++++++++++++++
>  qmp-commands.hx  |  74 ++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 236 insertions(+)
> 
> diff --git a/blockdev.c b/blockdev.c
> index 23a76eb..61150b4 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -2408,6 +2408,108 @@ void qmp_block_job_complete(const char *device, Error **errp)
>      block_job_complete(job, errp);
>  }
>  
> +void qmp_change_backing_file(const char *device,
> +                             bool has_image, const char *image,
> +                             bool has_image_node_name,
> +                             const char *image_node_name,
> +                             const char *backing_file,
> +                             Error **errp)
> +{
> +    BlockDriverState *bs = NULL;
> +    BlockDriverState *image_bs = NULL;
> +    Error *local_err = NULL;
> +    bool ro;
> +    int open_flags;
> +    int ret;
> +
> +    /* validate argument combinations */
> +    if (has_image && has_image_node_name) {
> +        error_setg(errp, "'image' and 'image-node-name' "
> +                         "are mutually exclusive");
> +        return;
> +    }
> +
> +    /* find the top layer BDS of the chain */
> +    bs = bdrv_find(device);
> +    if (!bs) {
> +        error_set(errp, QERR_DEVICE_NOT_FOUND, device);
> +        return;
> +    }
> +
> +    if (has_image_node_name) {
> +        image_bs = bdrv_lookup_bs(NULL, image_node_name, &local_err);
> +        if (local_err) {
> +            error_propagate(errp, local_err);
> +            return;
> +        }
> +    }
> +
> +    if (has_image) {
> +        if (!strcmp(bs->filename, image)) {
> +            image_bs = bs;
> +        } else {
> +            image_bs = bdrv_find_backing_image(bs, image);
> +        }
> +    }
> +
> +    if (!has_image && !has_image_node_name) {
> +        image_bs = bs;
> +    }
> +
> +    if (!image_bs) {
> +        error_setg(errp, "image file not found");
> +        return;
> +    }
> +
> +    if (bdrv_find_base(image_bs) == image_bs) {
> +        error_setg(errp, "not allowing backing file change on an image "
> +                         "without a backing file");
> +        return;
> +    }
> +
> +    /* even though we are not necessarily operating on bs, we need it to
> +     * determine if block ops are currently prohibited on the chain */
> +    if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_CHANGE, errp)) {
> +        return;
> +    }
> +
> +    /* final sanity check */
> +    if (!bdrv_chain_contains(bs, image_bs)) {
> +        error_setg(errp, "'%s' and image file are not in the same chain",
> +                   device);
> +        return;
> +    }
> +
> +    /* if not r/w, reopen to make r/w */
> +    open_flags = image_bs->open_flags;
> +    ro = bdrv_is_read_only(image_bs);
> +
> +    if (ro) {
> +        bdrv_reopen(image_bs, open_flags | BDRV_O_RDWR, &local_err);
> +        if (local_err) {
> +            error_propagate(errp, local_err);
> +            return;
> +        }
> +    }
> +
> +    ret = bdrv_change_backing_file(image_bs, backing_file,
> +                               image_bs->drv ? image_bs->drv->format_name : "");
> +
> +    if (ret < 0) {
> +        error_setg_errno(errp, -ret, "Could not change backing file to '%s'",
> +                         backing_file);
> +        /* don't exit here, so we can try to restore open flags if
> +         * appropriate */
> +    }
> +
> +    if (ro) {
> +        bdrv_reopen(image_bs, open_flags, &local_err);
> +        if (local_err) {
> +            error_propagate(errp, local_err); /* will preserve prior errp */
> +        }
> +    }
> +}
> +
>  void qmp_blockdev_add(BlockdevOptions *options, Error **errp)
>  {
>      QmpOutputVisitor *ov = qmp_output_visitor_new();
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 3400561..e57396b 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -2092,6 +2092,66 @@
>    'returns': 'str' }
>  
>  ##
> +# @change-backing-file
> +#
> +# Change the backing file in the image file metadata.  This does not cause QEMU
> +# to reopen the image file to reparse the backing filename (it may, however,
> +# perform a reopen to change permissions from r/o -> r/w -> r/o, if needed).
> +# The new backing file string is written into the image file metadata, and the
> +# QEMU internal strings are updated.
> +#
> +# The image file to perform the operation on can be specified by two different
> +# methods:
> +#
> +#  Method 1: Supply the device name (e.g. 'virtio0'), and optionally the image
> +#            filename.  This would use arguments @device and @image.
> +#
> +#  Method 2: Supply the device name, and the node-name of the image to modify,
> +#            via @image-node-name.
> +#
> +# Arguments @image and @image-node-name are mutually exclusive.
> +#
> +# Method 1 interface
> +#---------------------
> +# @image:          #optional The file name of the image to modify.  If omitted,
> +#                            and @image-node-name is not supplied, then the
> +#                            default is the active layer of the chain described
> +#                            by @device.
> +#
> +# Method 2 interface
> +#---------------------
> +# @image-node-name #optional The name of the block driver state node of the
> +#                            image to modify.  The @device argumen is used to
> +#                            verify @image-node-name is in the chain described
> +#                            by @device.
> +#
> +# Common arguments
> +#---------------------
> +# @device:          The name of the device.
> +#
> +# @backing-file:    The string to write as the backing file.  This string is
> +#                   not validated, so care should be taken when specifying
> +#                   the string or the image chain may not be able to be
> +#                   reopened again.
> +#
> +#                   If a pathname string is such that it cannot be
> +#                   resolved by QEMU, that means that subsequent QMP or
> +#                   HMP commands must use node-names for the image in
> +#                   question, as filename lookup methods will fail.
> +#
> +#
> +# Returns: Nothing on success
> +#          If @device does not exist or cannot be determined, DeviceNotFound
> +#          If @image is specified, but not @device, GenericError
> +#          If both @image and @image-node-name are specified, GenericError
> +#
> +# Since: 2.1
> +##
> +{ 'command': 'change-backing-file',
> +  'data': { 'device': 'str', '*image': 'str', '*image-node-name': 'str',
> +            'backing-file': 'str' } }
> +
> +##
>  # @block-commit
>  #
>  # Live commit of data from overlay image nodes into backing nodes - i.e.,
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 69d29ae..a17d3d5 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -1438,6 +1438,80 @@ Example:
>  EQMP
>  
>      {
> +        .name       = "change-backing-file",
> +        .args_type  = "device:s,image:s?,image-node-name:s?,backing-file:s",
> +        .mhandler.cmd_new = qmp_marshal_input_change_backing_file,
> +    },
> +
> +SQMP
> +change-backing-file
> +-------------------
> +Since: 2.1
> +
> +Change the backing file in the image file metadata.  This does not cause QEMU
> +to reopen the image file to reparse the backing filename (it may, however,
> +perform a reopen to change permissions from r/o -> r/w -> r/o, if needed).
> +The new backing file string is written into the image file metadata, and the
> +QEMU internal strings are updated.
> +
> +The image file to perform the operation on can be specified by two different
> +methods:
> +
> + Method 1: Supply the device name (e.g. 'virtio0'), and optionally the image
> +           filename.  This would use arguments "device" and "image".
> +
> + Method 2: Supply the device name, and the node-name of the image to modify,
> +           via "image-node-name".
> +
> +Arguments:
> +
> +Arguments "image" or "image-node-name" are mutually exclusive.
> +
> +
> +Method 1 interface
> +--------------------
> +- "image":              The file name of the image to modify.  If omitted,
> +                        and "image-node-name" is not supplied, then the
> +                        default is the active layer of the chain described
> +                        by device.
> +                        (json-string, optional)
> +
> +
> +Method 2 interface
> +--------------------
> +- "image-node-name":    The name of the block driver state node of the
> +                        image to modify.  The "device" is argument is used to
> +                        verify "image-node-name" is in the chain described by
> +                        "device".
> +                        (json-string, optional)
> +
> +
> +Common arguments
> +--------------------
> +- "device":             The name of the device.
> +                        (json-string)
> +
> +- "backing-file":       The string to write as the backing file.  This string is
> +                        not validated, so care should be taken when specifying
> +                        the string or the image chain may not be able to be
> +                        reopened again.
> +                        (json-string)
> +
> +                        If a pathname string is such that it cannot be
> +                        resolved by QEMU, that means that subsequent QMP or
> +                        HMP commands must use node-names for the image in
> +                        question, as filename lookup methods will fail.
> +
> +
> +Returns: Nothing on success
> +         If "device" does not exist or cannot be determined, DeviceNotFound
> +         If "image" is specified, but not "device, GenericError
> +         If both "image" and "image-node-name" are specified, GenericError
> +
> +
> +EQMP
> +
> +    {
>          .name       = "balloon",
>          .args_type  = "value:M",
>          .mhandler.cmd_new = qmp_marshal_input_balloon,
> -- 
> 1.9.3
>