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

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

[Jeff Cody]

Changes from v4->v5:

* Rebased on master
* Fixed commit log typos / stale paragraphs (Eric)
* Fixed comment typo (Eric)
* Added Eric's remaining R-b's


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/block-core.json      | 145 ++++++++++++++++++++++++++---
 qmp-commands.hx           | 181 ++++++++++++++++++++++++++++++++++--
 tests/qemu-iotests/040    |  28 ++++--
 10 files changed, 592 insertions(+), 98 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH v5 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 17f763d..fd43016 100644
--- a/block.c
+++ b/block.c
@@ -844,12 +844,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.8.3.1

[Qemu-devel] [PATCH v5 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 fd43016..f9ea1b4 100644
--- a/block.c
+++ b/block.c
@@ -3822,6 +3822,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 7d86e29..08fe1ac 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.8.3.1

[Qemu-devel] [PATCH v5 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 takes advantage of that.

Reviewed-by: Eric Blake <eblake@redhat.com>
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 f9ea1b4..83996e3 100644
--- a/block.c
+++ b/block.c
@@ -2569,32 +2569,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 {
@@ -4376,22 +4367,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.8.3.1

[Qemu-devel] [PATCH v5 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/block-core.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 4cbcc56..c4a28e3 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1920,7 +1920,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)
 {
@@ -1952,7 +1953,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/block-core.json b/qapi/block-core.json
index 7215e48..6ddce4f 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -690,8 +690,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,
@@ -719,7 +720,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.8.3.1

[Qemu-devel] [PATCH v5 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 preferred and recommended method now of using block-commit
is to specify the BDS to operate on via their node-name arguments.

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

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

diff --git a/blockdev.c b/blockdev.c
index c4a28e3..1608bb9 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1921,12 +1921,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
@@ -1940,20 +1943,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);
         }
@@ -1964,7 +1980,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);
@@ -1975,6 +1997,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/block-core.json b/qapi/block-core.json
index 6ddce4f..dddaa13 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -685,14 +685,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,
@@ -711,17 +726,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.8.3.1

[Qemu-devel] [PATCH v5 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/block-core.json      | 23 +++++++++++++++++++++--
 qmp-commands.hx           | 19 ++++++++++++++++++-
 7 files changed, 62 insertions(+), 11 deletions(-)

diff --git a/block.c b/block.c
index 83996e3..6881746 100644
--- a/block.c
+++ b/block.c
@@ -2616,12 +2616,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;
@@ -2673,7 +2676,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 1608bb9..7bfaffb 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1924,6 +1924,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)
 {
@@ -2015,11 +2016,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 08fe1ac..c49f487 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 8d58334..c5c295a 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -465,13 +465,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/block-core.json b/qapi/block-core.json
index dddaa13..ae1dde9 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -709,6 +709,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
@@ -721,7 +738,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
@@ -732,13 +748,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.8.3.1

[Qemu-devel] [PATCH v5 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.

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

diff --git a/blockdev.c b/blockdev.c
index 7bfaffb..67c9751 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1878,38 +1878,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/block-core.json b/qapi/block-core.json
index ae1dde9..d17e349 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -904,9 +904,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
 #
@@ -920,8 +928,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.8.3.1

[Qemu-devel] [PATCH v5 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/block-core.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 67c9751..9867bf4 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1881,6 +1881,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)
@@ -1932,6 +1933,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)) {
@@ -1939,6 +1943,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/block-core.json b/qapi/block-core.json
index d17e349..c76d45d 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -916,6 +916,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).
@@ -929,7 +944,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.8.3.1

[Qemu-devel] [PATCH v5 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.8.3.1

[Qemu-devel] [PATCH v5 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.

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

diff --git a/blockdev.c b/blockdev.c
index 9867bf4..dd1d71e 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2414,6 +2414,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/block-core.json b/qapi/block-core.json
index c76d45d..0939143 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -680,6 +680,66 @@
   'data': 'BlockdevSnapshot' }
 
 ##
+# @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 argument 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.8.3.1

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

[Benoît Canet]

The Friday 13 Jun 2014 à 14:52:29 (-0400), Jeff Cody wrote :
> 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.

if the __qemu## determined a different name space no collision resolution would be
needed.

Also Eric you must take care that these node name does not ends up in libvirt
XML since a new qemu process can generate a new different set of node name.

> 
> 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 17f763d..fd43016 100644
> --- a/block.c
> +++ b/block.c
> @@ -844,12 +844,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.8.3.1
> 
> 

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

[Eric Blake]

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

On 06/16/2014 08:10 AM, Benoît Canet wrote:

>> 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.
> 
> if the __qemu## determined a different name space no collision resolution would be
> needed.
> 
> Also Eric you must take care that these node name does not ends up in libvirt
> XML since a new qemu process can generate a new different set of node name.

Correct. Either libvirt will always generate its own node names (and the
presence of a qemu-generated name is a bug in libvirt), or libvirt will
query node-names as it runs, and clear them out when the qemu process
goes away.  It will NOT expose qemu names to user-visible XML (although
it should be safe to save the qemu names to internal-only XML used to
cache results across libvirtd restarts while still communicating to the
same qemu process).

-- 
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 v5 01/10] block: Auto-generate node_names for each BDS entry

[Jeff Cody]

On Mon, Jun 16, 2014 at 04:10:41PM +0200, Benoît Canet wrote:
> The Friday 13 Jun 2014 à 14:52:29 (-0400), Jeff Cody wrote :
> > 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.
> 
> if the __qemu## determined a different name space no collision resolution would be
> needed.
> 

True.  The random string generation was pretty trivial, however, and
has an added benefit: it helps prevent the user / management software
from assuming or attempting to predict node names for images.

> Also Eric you must take care that these node name does not ends up in libvirt
> XML since a new qemu process can generate a new different set of node name.
>

Yes.. the node names are guaranteed to be unique, but are ephemeral in
nature.

> > 
> > 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 17f763d..fd43016 100644
> > --- a/block.c
> > +++ b/block.c
> > @@ -844,12 +844,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.8.3.1
> > 
> > 

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

[Benoît Canet]

The Friday 13 Jun 2014 à 14:52:31 (-0400), 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 takes advantage of that.
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> 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 f9ea1b4..83996e3 100644
> --- a/block.c
> +++ b/block.c
> @@ -2569,32 +2569,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 {
> @@ -4376,22 +4367,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.8.3.1
> 
> 

That is a neat simplification.

Reviewed-by: Benoit Canet <benoit@irqsave.net>

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

[Benoît Canet]

The Friday 13 Jun 2014 à 14:52:37 (-0400), Jeff Cody wrote :
> 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.8.3.1
> 
> 
Reviewed-by: Benoit Canet <benoit@irqsave.net>

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

[Eric Blake]

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

On 06/13/2014 12:52 PM, Jeff Cody wrote:
> Changes from v4->v5:
> 
> * Rebased on master
> * Fixed commit log typos / stale paragraphs (Eric)
> * Fixed comment typo (Eric)
> * Added Eric's remaining R-b's
> 

On IRC, we found another bug that needs repairing:

# cd /tmp
# echo tmp > tmp
# qemu-kvm -qmp stdio -drive file=/tmp/tmp
{"QMP": {"version": {"qemu": {"micro": 50, "minor": 0, "major": 2},
"package": ""}, "capabilities": []}}
{"execute":"qmp_capabilities"}
{"return": {}}
{"execute":"block-commit","arguments":{"device":"ide0-hd0"}}
{"return": {}}
{"timestamp": {"seconds": 1402972048, "microseconds": 153175}, "event":
"BLOCK_JOB_READY", "data": {"device": "ide0-hd0", "len": 512, "offset":
512, "speed": 0, "type": "commit"}}
{"execute":"block-job-complete","arguments":{"device":"ide0-hd0"}}
{"return": {}}
qemu-system-x86_64: block.c:2034: bdrv_swap: Assertion
`bs_new->device_name[0] == '\0'' failed.
Aborted (core dumped)

Attempts to commit-to-self should be rejected at block-commit time,
rather than causing an assertion failure down the road.

-- 
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 v5 00/10] Modify block jobs to use node-names

[Jeff Cody]

On Tue, Jun 17, 2014 at 06:19:13AM -0600, Eric Blake wrote:
> On 06/13/2014 12:52 PM, Jeff Cody wrote:
> > Changes from v4->v5:
> > 
> > * Rebased on master
> > * Fixed commit log typos / stale paragraphs (Eric)
> > * Fixed comment typo (Eric)
> > * Added Eric's remaining R-b's
> > 
> 
> On IRC, we found another bug that needs repairing:
> 
> # cd /tmp
> # echo tmp > tmp
> # qemu-kvm -qmp stdio -drive file=/tmp/tmp
> {"QMP": {"version": {"qemu": {"micro": 50, "minor": 0, "major": 2},
> "package": ""}, "capabilities": []}}
> {"execute":"qmp_capabilities"}
> {"return": {}}
> {"execute":"block-commit","arguments":{"device":"ide0-hd0"}}
> {"return": {}}
> {"timestamp": {"seconds": 1402972048, "microseconds": 153175}, "event":
> "BLOCK_JOB_READY", "data": {"device": "ide0-hd0", "len": 512, "offset":
> 512, "speed": 0, "type": "commit"}}
> {"execute":"block-job-complete","arguments":{"device":"ide0-hd0"}}
> {"return": {}}
> qemu-system-x86_64: block.c:2034: bdrv_swap: Assertion
> `bs_new->device_name[0] == '\0'' failed.
> Aborted (core dumped)
> 
> Attempts to commit-to-self should be rejected at block-commit time,
> rather than causing an assertion failure down the road.
>

Thanks... spinning a v6 to prevent this scenario.

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

[Eric Blake]

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

On 06/13/2014 12:52 PM, Jeff Cody wrote:
> This modifies the block operation block-commit so that it will
> accept node-name arguments for either 'top' or 'base' BDS.
> 

>  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
> @@ -1940,20 +1943,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);

On IRC, we discussed that libvirt would like to rely on
QERR_DEVICE_NOT_FOUND (aka "class":"DeviceNotFound") as a witness probe
for whether qemu supports optional 'top' (which is a rough approximation
for whether qemu supports active commit, other than it mis-diagnoses
qemu 2.0; but libvirt probably wants to just require qemu 2.1 for active
commit anyways).  Compare:

on 2.1: {"execute":"block-commit","arguments":{"device":"no-such"}}
{"error": {"class": "DeviceNotFound", "desc": "Device 'no-such' not found"}}
on 1.7: {"execute":"block-commit","arguments":{"device":"no-such"}}
{"error": {"class": "GenericError", "desc": "Parameter 'top' is missing"}}

It might be worth a comment in the code that the device not found error
from a failed bdrv_find() should occur as early as possible, at least in
the case where all optional parameters are omitted, to serve as a
libvirt probe point (it is okay for the code to have various
GenericError sanity checks occur before the bdrv_find, so long as those
checks can only be triggered when optional parameters are present - but
again, worth mentioning in a comment added to the code).


-- 
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 v5 00/10] Modify block jobs to use node-names

[Jeff Cody]

On Tue, Jun 17, 2014 at 06:19:13AM -0600, Eric Blake wrote:
> On 06/13/2014 12:52 PM, Jeff Cody wrote:
> > Changes from v4->v5:
> > 
> > * Rebased on master
> > * Fixed commit log typos / stale paragraphs (Eric)
> > * Fixed comment typo (Eric)
> > * Added Eric's remaining R-b's
> > 
> 
> On IRC, we found another bug that needs repairing:
> 
> # cd /tmp
> # echo tmp > tmp
> # qemu-kvm -qmp stdio -drive file=/tmp/tmp
> {"QMP": {"version": {"qemu": {"micro": 50, "minor": 0, "major": 2},
> "package": ""}, "capabilities": []}}
> {"execute":"qmp_capabilities"}
> {"return": {}}
> {"execute":"block-commit","arguments":{"device":"ide0-hd0"}}
> {"return": {}}
> {"timestamp": {"seconds": 1402972048, "microseconds": 153175}, "event":
> "BLOCK_JOB_READY", "data": {"device": "ide0-hd0", "len": 512, "offset":
> 512, "speed": 0, "type": "commit"}}
> {"execute":"block-job-complete","arguments":{"device":"ide0-hd0"}}
> {"return": {}}
> qemu-system-x86_64: block.c:2034: bdrv_swap: Assertion
> `bs_new->device_name[0] == '\0'' failed.
> Aborted (core dumped)
> 
> Attempts to commit-to-self should be rejected at block-commit time,
> rather than causing an assertion failure down the road.
> 

I put the changes to fix this in patch 4/10 in the v6 (along with the
additional comment).  I mean to drop your R-b in the patch so you
could review it again, but I forgot before sending it (sorry) - but if
you want to review it, it is all contained in patch 4.

Jeff