[Qemu-devel] [PATCH v7 for 2.1 0/4] Allow custom backing-file string in commit/stream

[Qemu-devel] [PATCH v7 for 2.1 0/4] Allow custom backing-file string in commit/stream

[Jeff Cody]

This is part 2 of a split of "Modify block jobs to use node-names".

These patches add a helper function, allow block-commit and block-stream to 
change backing files, and adds a standalone API to change backing files.

The api to change backing files relies still on the "check active layer blocker"
workaround on blockers.

Jeff Cody (4):
  block: add helper function to determine if a BDS is in a chain
  block: extend block-commit to accept a string for the backing file
  block: add backing-file option to block-stream
  block: add QAPI command to allow live backing file change

 block.c                   |  19 ++++++-
 block/commit.c            |   9 ++--
 block/stream.c            |  11 ++--
 blockdev.c                | 133 ++++++++++++++++++++++++++++++++++++++++++++--
 hmp.c                     |   2 +-
 include/block/block.h     |   4 +-
 include/block/block_int.h |   3 +-
 qapi/block-core.json      |  99 ++++++++++++++++++++++++++++++++--
 qmp-commands.hx           |  95 ++++++++++++++++++++++++++++++++-
 9 files changed, 350 insertions(+), 25 deletions(-)

-- 
1.9.3

[Qemu-devel] [PATCH v7 for 2.1 1/4] 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 a91809c..84ddb46 100644
--- a/block.c
+++ b/block.c
@@ -3781,6 +3781,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 d0baf4f..1055990 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -399,6 +399,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 v7 for 2.1 2/4] 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      | 20 ++++++++++++++++++--
 qmp-commands.hx           | 19 ++++++++++++++++++-
 7 files changed, 59 insertions(+), 11 deletions(-)

diff --git a/block.c b/block.c
index 84ddb46..def7e67 100644
--- a/block.c
+++ b/block.c
@@ -2560,12 +2560,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;
@@ -2617,7 +2620,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 858bd75..4a7885e 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1910,6 +1910,7 @@ 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_top, const char *top,
+                      bool has_backing_file, const char *backing_file,
                       bool has_speed, int64_t speed,
                       Error **errp)
 {
@@ -1975,11 +1976,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 1055990..a6a5725 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -284,7 +284,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 715c761..cba5dca 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -459,13 +459,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 ecd90ee..9ebe38c 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -694,6 +694,23 @@
 #                    which contains the topmost data to be committed down. If
 #                    not specified, this is the active layer.
 #
+# @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
@@ -706,7 +723,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
@@ -721,7 +737,7 @@
 ##
 { 'command': 'block-commit',
   'data': { 'device': 'str', '*base': 'str', '*top': 'str',
-            '*speed': 'int' } }
+            '*backing-file': 'str', '*speed': 'int' } }
 
 ##
 # @drive-backup
diff --git a/qmp-commands.hx b/qmp-commands.hx
index a6a8fec..e0dea3f 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?,backing-file:s?,speed:o?",
         .mhandler.cmd_new = qmp_marshal_input_block_commit,
     },
 
@@ -1006,6 +1006,23 @@ Arguments:
           which contains the topmost data to be committed down. If
           not specified, this is the active layer. (json-string, optional)
 
+- 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 v7 for 2.1 3/4] 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           | 23 +++++++++++++++++++----
 hmp.c                |  2 +-
 qapi/block-core.json | 19 +++++++++++++++++--
 qmp-commands.hx      |  2 +-
 5 files changed, 43 insertions(+), 14 deletions(-)

diff --git a/block/stream.c b/block/stream.c
index 0433409..34de8ba 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 4a7885e..61b84d0 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1866,14 +1866,17 @@ 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_backing_file, const char *backing_file,
+                      bool has_speed, int64_t speed,
                       bool has_on_error, BlockdevOnError on_error,
                       Error **errp)
 {
     BlockDriverState *bs;
     BlockDriverState *base_bs = NULL;
     Error *local_err = NULL;
+    const char *base_name = NULL;
 
     if (!has_on_error) {
         on_error = BLOCKDEV_ON_ERROR_REPORT;
@@ -1889,15 +1892,27 @@ void qmp_block_stream(const char *device, bool has_base,
         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;
     }
 
-    stream_start(bs, base_bs, base, has_speed ? speed : 0,
+    /* 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;
+    }
+
+    /* backing_file string overrides base bs filename */
+    base_name = has_backing_file ? backing_file : base_name;
+
+    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 dc3d279..0dc2499 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1175,7 +1175,7 @@ void hmp_block_stream(Monitor *mon, const QDict *qdict)
     const char *base = qdict_get_try_str(qdict, "base");
     int64_t speed = qdict_get_try_int(qdict, "speed", 0);
 
-    qmp_block_stream(device, base != NULL, base,
+    qmp_block_stream(device, base != NULL, base, false, NULL,
                      qdict_haskey(qdict, "speed"), speed,
                      true, BLOCKDEV_ON_ERROR_REPORT, &error);
 
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 9ebe38c..f1b505b 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -888,6 +888,21 @@
 #
 # @base:   #optional the common backing file name
 #
+# @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).
@@ -900,8 +915,8 @@
 # Since: 1.1
 ##
 { 'command': 'block-stream',
-  'data': { 'device': 'str', '*base': 'str', '*speed': 'int',
-            '*on-error': 'BlockdevOnError' } }
+  'data': { 'device': 'str', '*base': 'str', '*backing-file': 'str',
+            '*speed': 'int', '*on-error': 'BlockdevOnError' } }
 
 ##
 # @block-job-set-speed:
diff --git a/qmp-commands.hx b/qmp-commands.hx
index e0dea3f..7b029c4 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?,speed:o?,backing-file:s?,on-error:s?",
         .mhandler.cmd_new = qmp_marshal_input_block_stream,
     },
 
-- 
1.9.3

[Qemu-devel] [PATCH v7 for 2.1 4/4] 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 61b84d0..c2fafd1 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2347,6 +2347,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 f1b505b..ede27f0 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 7b029c4..ffd40d3 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1365,6 +1365,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 v7 for 2.1 4/4] block: add QAPI command to allow live backing file change

[Eric Blake]

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

On 06/25/2014 01:40 PM, 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.

For 2.1, did we want to limit this command to only affecting the active
layer (and failing if it was attempted on a backing file), in order to
simplify dealing with op-blocker work?  Having the command exist, even
if it is limited in nature, is all the more libvirt wants right now; and
it's easier to relax a constraint in a future release than it is to
release something now and have to keep it working even after we rework
op-blockers in the future.

> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Signed-off-by: Jeff Cody <jcody@redhat.com>

I'm still okay with this patch as-is, if we are okay with the ability to
rename the backing file of a non-active node.

-- 
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 v7 for 2.1 4/4] block: add QAPI command to allow live backing file change

[Jeff Cody]

On Wed, Jun 25, 2014 at 01:52:25PM -0600, Eric Blake wrote:
> On 06/25/2014 01:40 PM, 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.
> 
> For 2.1, did we want to limit this command to only affecting the active
> layer (and failing if it was attempted on a backing file), in order to
> simplify dealing with op-blocker work?  Having the command exist, even
> if it is limited in nature, is all the more libvirt wants right now; and
> it's easier to relax a constraint in a future release than it is to
> release something now and have to keep it working even after we rework
> op-blockers in the future.
> 

I left this as-is, because the blocker issue needs to be addressed
somehow for 2.1.  And once we know in what manner, I'll just update
this series to use that methodology (if necessary).  Unless Stefan or
Kevin would prefer that I change this to only operate on the active
layer, of course.

> > 
> > Reviewed-by: Eric Blake <eblake@redhat.com>
> > Signed-off-by: Jeff Cody <jcody@redhat.com>
> 
> I'm still okay with this patch as-is, if we are okay with the ability to
> rename the backing file of a non-active node.
> 
> -- 
> Eric Blake   eblake redhat com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
> 

Re: [Qemu-devel] [PATCH v7 for 2.1 1/4] block: add helper function to determine if a BDS is in a chain

[Kevin Wolf]

Am 25.06.2014 um 21:40 hat Jeff Cody geschrieben:
> 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>

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

Re: [Qemu-devel] [PATCH v7 for 2.1 2/4] block: extend block-commit to accept a string for the backing file

[Kevin Wolf]

Am 25.06.2014 um 21:40 hat Jeff Cody geschrieben:
> 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>

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

Re: [Qemu-devel] [PATCH v7 for 2.1 4/4] block: add QAPI command to allow live backing file change

[Kevin Wolf]

Am 25.06.2014 um 21:40 hat Jeff Cody geschrieben:
> 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>

I'm not a huge fan of adding two different addressing modes to a new QMP
command. I consider using device_name/filename as deprecated and expect
that management tools use node-name for new commands.

Also there's still Eric's reply and Jeff's promise to update the patch
once blockers were sorted out for 2.1.

I'm leaning towards declaring this patch not ready.

Kevin

Re: [Qemu-devel] [PATCH v7 for 2.1 4/4] block: add QAPI command to allow live backing file change

[Eric Blake]

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

On 06/30/2014 08:59 AM, Kevin Wolf wrote:
> Am 25.06.2014 um 21:40 hat Jeff Cody geschrieben:
>> 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
>>

> I'm not a huge fan of adding two different addressing modes to a new QMP
> command. I consider using device_name/filename as deprecated and expect
> that management tools use node-name for new commands.

Good point.  Libvirt does not plan on using this command directly (that
is, right now, we don't see a need to change the backing file of an
image except as part of a bigger operation; but the work with
block-commit and block-stream earlier in this series already adds that
in the bigger operation); but we DO want to have a witness that the
other commands have support for relative backing name selection.  So it
is easiest if this new command exists, in ANY form, as the witness that
the overall series is in place, without regards to the parameters in
this particular command.

> 
> Also there's still Eric's reply and Jeff's promise to update the patch
> once blockers were sorted out for 2.1.
> 
> I'm leaning towards declaring this patch not ready.

How about a compromise - simplify the command to always require a
node-name and device for 2.1. For 2.2, we may later add optional
arguments (for example, making @device optional instead of mandatory),
and/or provide alternative interfaces (if it turns out we wanted
filename addressing after all), but the immediate concern is getting the
command installed, with the bare minimum interface.  That is, my
proposal is to completely drop the addressing by filename, and only
allow addressing by node-name.

##
# @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.
#
# @image-node-name: The name of the block driver state node of the
#                   image to modify.
#
# @device:          The name of the device that owns image-node-name.
#
# @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.
#
# Since: 2.1
##
{ 'command': 'change-backing-file',
  'data': { 'device': 'str', 'image-node-name': 'str',
            'backing-file': 'str' } }

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

[Qemu-devel] [PATCH v8 for 2.1 4/4] block: add QAPI command to allow live backing file change

[Stefan Hajnoczi]

From: Jeff Cody <jcody@redhat.com>

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.

[Jeff is offline so I respun this patch in his absence.  Dropped image
filename since using node-name is preferred and this is a new command.
No need to introduce the limitations of finding images by filename.
--Stefan]

Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Jeff Cody <jcody@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 blockdev.c           | 79 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 qapi/block-core.json | 26 +++++++++++++++++
 qmp-commands.hx      | 39 ++++++++++++++++++++++++++
 3 files changed, 144 insertions(+)

diff --git a/blockdev.c b/blockdev.c
index dae92bb..48bd9a3 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2388,6 +2388,85 @@ void qmp_block_job_complete(const char *device, Error **errp)
     block_job_complete(job, errp);
 }
 
+void qmp_change_backing_file(const char *device,
+                             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;
+
+    /* find the top layer BDS of the chain */
+    bs = bdrv_find(device);
+    if (!bs) {
+        error_set(errp, QERR_DEVICE_NOT_FOUND, device);
+        return;
+    }
+
+    image_bs = bdrv_lookup_bs(NULL, image_node_name, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return;
+    }
+
+    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 c241967..e378653 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -680,6 +680,32 @@
   '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.
+#
+# @image-node-name: The name of the block driver state node of the
+#                   image to modify.
+#
+# @device:          The name of the device that owns image-node-name.
+#
+# @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.
+#
+# Since: 2.1
+##
+{ 'command': 'change-backing-file',
+  'data': { 'device': '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 f900dff..4be4765 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1370,6 +1370,45 @@ Example:
 EQMP
 
     {
+        .name       = "change-backing-file",
+        .args_type  = "device: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.
+
+Arguments:
+
+- "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)
+
+- "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)
+
+Returns: Nothing on success
+         If "device" does not exist or cannot be determined, DeviceNotFound
+
+EQMP
+
+    {
         .name       = "balloon",
         .args_type  = "value:M",
         .mhandler.cmd_new = qmp_marshal_input_balloon,
-- 
1.9.3

Re: [Qemu-devel] [PATCH v8 for 2.1 4/4] block: add QAPI command to allow live backing file change

[Kevin Wolf]

Am 01.07.2014 um 09:52 hat Stefan Hajnoczi geschrieben:
> From: Jeff Cody <jcody@redhat.com>
> 
> 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.
> 
> [Jeff is offline so I respun this patch in his absence.  Dropped image
> filename since using node-name is preferred and this is a new command.
> No need to introduce the limitations of finding images by filename.
> --Stefan]
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Signed-off-by: Jeff Cody <jcody@redhat.com>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

Re: [Qemu-devel] [PATCH v8 for 2.1 4/4] block: add QAPI command to allow live backing file change

[Eric Blake]

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

On 07/01/2014 01:52 AM, Stefan Hajnoczi wrote:
> From: Jeff Cody <jcody@redhat.com>
> 
> 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
> 

This part of the commit message is now stale, but that's okay...

> 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.
> 
> [Jeff is offline so I respun this patch in his absence.  Dropped image
> filename since using node-name is preferred and this is a new command.
> No need to introduce the limitations of finding images by filename.
> --Stefan]

...given that you are preserving history while documenting changes.

> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Signed-off-by: Jeff Cody <jcody@redhat.com>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>

This is a big enough change that I re-reviewed the patch.  The reduced
capability of the command is not an issue for how libvirt plans to use
it (mainly as a witness that libvirt has control over backing file
strings); and it is still desirable from libvirt's point of view to
introduce the command.

Thanks for fixing this up.

> ---
>  blockdev.c           | 79 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  qapi/block-core.json | 26 +++++++++++++++++
>  qmp-commands.hx      | 39 ++++++++++++++++++++++++++
>  3 files changed, 144 insertions(+)
> 


> +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.
> +
> +Arguments:
> +
> +- "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)

No longer optional.  If you still have time to fix it, great; if not,
it's minor enough.  My R-b still stands.
-- 
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 --]