Skip to contentSkip to conversation

Allow custom backing-file string in commit/stream

PATCH In review Jeff Cody 4 participants 14 messages Jun 25, 2014 – Jul 1, 2014 Block layer

lore
Share & tools
lore

Jeff Cody

author
#0 lore raw

v7 cover Allow custom backing-file string in commit/stream

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(-)
Signature · 1 line
1.9.3
author
#1 lore raw

PATCH v7 1/4 block: add helper function to determine if a BDS is in a chain

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.

---
 block.c               | 11 +++++++++++
 include/block/block.h |  1 +
 2 files changed, 12 insertions(+)
block.c +11 -0
index a91809c..84ddb46 100644
@@ -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) {
include/block/block.h +1 -0
index d0baf4f..1055990 100644
@@ -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);
Signature · 1 line
1.9.3
author
#3 lore raw

PATCH v7 2/4 block: extend block-commit to accept a string for the backing file

In reply to 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.

---
 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(-)
block.c +6 -2
index 84ddb46..def7e67 100644
@@ -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;
block/commit.c +6 -3
index 5c09f44..91517d3 100644
@@ -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);
 
blockdev.c +7 -1
index 858bd75..4a7885e 100644
@@ -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);
include/block/block.h +2 -1
index 1055990..a6a5725 100644
@@ -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);
include/block/block_int.h +2 -1
index 715c761..cba5dca 100644
@@ -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.
qapi/block-core.json +18 -2
index ecd90ee..9ebe38c 100644
@@ -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
qmp-commands.hx +18 -1
index a6a8fec..e0dea3f 100644
@@ -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
Signature · 1 line
1.9.3
author
#5 lore raw

PATCH v7 3/4 block: add backing-file option to block-stream

In reply to 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.

---
 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(-)
block/stream.c +5 -6
index 0433409..34de8ba 100644
@@ -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);
blockdev.c +19 -4
index 4a7885e..61b84d0 100644
@@ -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);
hmp.c +1 -1
index dc3d279..0dc2499 100644
@@ -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);
 
qapi/block-core.json +17 -2
index 9ebe38c..f1b505b 100644
@@ -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:
qmp-commands.hx +1 -1
index e0dea3f..7b029c4 100644
@@ -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,
     },
 
Signature · 1 line
1.9.3
author
#6 lore raw

PATCH v7 4/4 block: add QAPI command to allow live backing file change

In reply to 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.

---
 blockdev.c           | 102 +++++++++++++++++++++++++++++++++++++++++++++++++++
 qapi/block-core.json |  60 ++++++++++++++++++++++++++++++
 qmp-commands.hx      |  74 +++++++++++++++++++++++++++++++++++++
 3 files changed, 236 insertions(+)
blockdev.c +102 -0
index 61b84d0..c2fafd1 100644
@@ -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();
qapi/block-core.json +60 -0

66 lines not on this page · raw message

qmp-commands.hx +74 -0

80 lines not on this page · raw message

Signature · 1 line
1.9.3
#7 lore raw

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

Quoted 12 lines · 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.

Quoted 6 lines

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.

Quoted 3 lines

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.

Signature · 2 lines
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org
author
#8 lore raw

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

Quoted 20 lines · Eric Blake
Quoted 12 lines · 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.

Quoted 6 lines

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.

Quoted 8 lines

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.

Quoted 11 lines
Quoted 3 lines

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.

Signature · 2 lines
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Reviewed-by on v7 1/4
#2 lore raw

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

In reply to Jeff Cody

Am 25.06.2014 um 21:40 hat Jeff Cody geschrieben:

Quoted 9 lines

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 on v7 2/4
#4 lore raw

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

In reply to Jeff Cody

Am 25.06.2014 um 21:40 hat Jeff Cody geschrieben:

Quoted 24 lines

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).

Quoted 6 lines

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.

#9 lore raw

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

In reply to Jeff Cody

Am 25.06.2014 um 21:40 hat Jeff Cody geschrieben:

Quoted 16 lines

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.

Quoted 5 lines

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.

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

#10 lore raw

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

Quoted 6 lines · Kevin Wolf

Am 25.06.2014 um 21:40 hat Jeff Cody geschrieben:

Quoted 5 lines

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:

Quoted 7 lines
Quoted 3 lines

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.

Quoted 5 lines

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' } }
Signature · 2 lines
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org
#11 lore raw

PATCH v8 4/4 block: add QAPI command to allow live backing file change

In reply to Jeff Cody

From: Jeff Cody <[email protected]>

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]

---
 blockdev.c           | 79 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 qapi/block-core.json | 26 +++++++++++++++++
 qmp-commands.hx      | 39 ++++++++++++++++++++++++++
 3 files changed, 144 insertions(+)
blockdev.c +79 -0
index dae92bb..48bd9a3 100644
@@ -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();
qapi/block-core.json +26 -0
index c241967..e378653 100644
@@ -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.,
qmp-commands.hx +39 -0
index f900dff..4be4765 100644
@@ -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,
Signature · 1 line
1.9.3
Reviewed-by on v8 4/4
#12 lore raw

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

Am 01.07.2014 um 09:52 hat Stefan Hajnoczi geschrieben:

Quoted 21 lines

From: Jeff Cody <[email protected]>

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.

Quoted 8 lines

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

#13 lore raw

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

In reply to Stefan Hajnoczi
Quoted 10 lines · Stefan Hajnoczi

From: Jeff Cody <[email protected]>

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

Quoted 8 lines

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.

Quoted 7 lines

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.

Quoted 4 lines

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.

Quoted 16 lines
---
 blockdev.c           | 79 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 qapi/block-core.json | 26 +++++++++++++++++
 qmp-commands.hx      | 39 ++++++++++++++++++++++++++
 3 files changed, 144 insertions(+)
qmp-commands.hx +8 -0
+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,

qmp-commands.hx Example: · line 1395

Quoted 10 lines
qmp-commands.hx +10 -0
+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.

Signature · 2 lines
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org