[PATCH v3 0/2] block: commit/stream: Allow users to request only format driver names in backing file format

[PATCH v3 0/2] block: commit/stream: Allow users to request only format driver names in backing file format

[Peter Krempa]

Please see patches for rationale.

Libvirt patches using this new flag will be posted soon-ish (after
cleanup).

v3:
 - changed name of flag to 'backing-mask-protocol' (Eric)
 - decided to keep Vladimir's R-b as he requested shorter name too

v2:
 - fixed mistaken argument order in 'hmp_block_stream'
 - changed version in docs to 9.0 as getting this into RC 3 probably
   isn't realistic

Peter Krempa (2):
  block: commit: Allow users to request only format driver names in
    backing file format
  block: stream: Allow users to request only format driver names in
    backing file format

 block.c                                | 37 +++++++++++++++++++++-----
 block/commit.c                         |  6 ++++-
 block/monitor/block-hmp-cmds.c         |  2 +-
 block/stream.c                         | 10 ++++++-
 blockdev.c                             | 13 +++++++++
 include/block/block-global-state.h     |  3 ++-
 include/block/block_int-common.h       |  4 ++-
 include/block/block_int-global-state.h |  6 +++++
 qapi/block-core.json                   | 17 ++++++++++--
 tests/unit/test-bdrv-drain.c           |  3 ++-
 10 files changed, 86 insertions(+), 15 deletions(-)

-- 
2.43.0

[PATCH v3 1/2] block: commit: Allow users to request only format driver names in backing file format

[Peter Krempa]

Introduce a new flag 'backing-mask-protocol' for the block-commit QMP
command which instructs the internals to use 'raw' instead of the
protocol driver in case when a image is used without a dummy 'raw'
wrapper.

The flag is designed such that it can be always asserted by management
tools even when there isn't any update to backing files.

The flag will be used by libvirt so that the backing images still
reference the proper format even when libvirt will stop using the dummy
raw driver (raw driver with no other config). Libvirt needs this so that
the images stay compatible with older libvirt versions which didn't
expect that a protocol driver name can appear in the backing file format
field.

Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
 block.c                                | 37 +++++++++++++++++++++-----
 block/commit.c                         |  6 ++++-
 blockdev.c                             |  6 +++++
 include/block/block-global-state.h     |  3 ++-
 include/block/block_int-common.h       |  4 ++-
 include/block/block_int-global-state.h |  3 +++
 qapi/block-core.json                   |  8 +++++-
 tests/unit/test-bdrv-drain.c           |  3 ++-
 8 files changed, 58 insertions(+), 12 deletions(-)

diff --git a/block.c b/block.c
index bfb0861ec6..771a00a14d 100644
--- a/block.c
+++ b/block.c
@@ -1309,11 +1309,14 @@ static void bdrv_backing_detach(BdrvChild *c)
 }

 static int bdrv_backing_update_filename(BdrvChild *c, BlockDriverState *base,
-                                        const char *filename, Error **errp)
+                                        const char *filename,
+                                        bool backing_mask_protocol,
+                                        Error **errp)
 {
     BlockDriverState *parent = c->opaque;
     bool read_only = bdrv_is_read_only(parent);
     int ret;
+    const char *format_name;
     GLOBAL_STATE_CODE();

     if (read_only) {
@@ -1323,9 +1326,23 @@ static int bdrv_backing_update_filename(BdrvChild *c, BlockDriverState *base,
         }
     }

-    ret = bdrv_change_backing_file(parent, filename,
-                                   base->drv ? base->drv->format_name : "",
-                                   false);
+    if (base->drv) {
+        /*
+         * If the new base image doesn't have a format driver layer, which we
+         * detect by the fact that @base is a protocol driver, we record
+         * 'raw' as the format instead of putting the protocol name as the
+         * backing format
+         */
+        if (backing_mask_protocol && base->drv->protocol_name) {
+            format_name = "raw";
+        } else {
+            format_name = base->drv->format_name;
+        }
+    } else {
+        format_name = "";
+    }
+
+    ret = bdrv_change_backing_file(parent, filename, format_name, false);
     if (ret < 0) {
         error_setg_errno(errp, -ret, "Could not update backing file link");
     }
@@ -1479,10 +1496,14 @@ static void GRAPH_WRLOCK bdrv_child_cb_detach(BdrvChild *child)
 }

 static int bdrv_child_cb_update_filename(BdrvChild *c, BlockDriverState *base,
-                                         const char *filename, Error **errp)
+                                         const char *filename,
+                                         bool backing_mask_protocol,
+                                         Error **errp)
 {
     if (c->role & BDRV_CHILD_COW) {
-        return bdrv_backing_update_filename(c, base, filename, errp);
+        return bdrv_backing_update_filename(c, base, filename,
+                                            backing_mask_protocol,
+                                            errp);
     }
     return 0;
 }
@@ -5961,7 +5982,8 @@ void bdrv_unfreeze_backing_chain(BlockDriverState *bs, BlockDriverState *base)
  *
  */
 int bdrv_drop_intermediate(BlockDriverState *top, BlockDriverState *base,
-                           const char *backing_file_str)
+                           const char *backing_file_str,
+                           bool backing_mask_protocol)
 {
     BlockDriverState *explicit_top = top;
     bool update_inherits_from;
@@ -6027,6 +6049,7 @@ int bdrv_drop_intermediate(BlockDriverState *top, BlockDriverState *base,

         if (c->klass->update_filename) {
             ret = c->klass->update_filename(c, base, backing_file_str,
+                                            backing_mask_protocol,
                                             &local_err);
             if (ret < 0) {
                 /*
diff --git a/block/commit.c b/block/commit.c
index 69cc75be0c..4c351644c1 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -42,6 +42,7 @@ typedef struct CommitBlockJob {
     bool base_read_only;
     bool chain_frozen;
     char *backing_file_str;
+    bool backing_mask_protocol;
 } CommitBlockJob;

 static int commit_prepare(Job *job)
@@ -61,7 +62,8 @@ static int commit_prepare(Job *job)
     /* FIXME: bdrv_drop_intermediate treats total failures and partial failures
      * identically. Further work is needed to disambiguate these cases. */
     return bdrv_drop_intermediate(s->commit_top_bs, s->base_bs,
-                                  s->backing_file_str);
+                                  s->backing_file_str,
+                                  s->backing_mask_protocol);
 }

 static void commit_abort(Job *job)
@@ -254,6 +256,7 @@ void commit_start(const char *job_id, BlockDriverState *bs,
                   BlockDriverState *base, BlockDriverState *top,
                   int creation_flags, int64_t speed,
                   BlockdevOnError on_error, const char *backing_file_str,
+                  bool backing_mask_protocol,
                   const char *filter_node_name, Error **errp)
 {
     CommitBlockJob *s;
@@ -408,6 +411,7 @@ void commit_start(const char *job_id, BlockDriverState *bs,
     blk_set_disable_request_queuing(s->top, true);

     s->backing_file_str = g_strdup(backing_file_str);
+    s->backing_mask_protocol = backing_mask_protocol;
     s->on_error = on_error;

     trace_commit_start(bs, base, top, s);
diff --git a/blockdev.c b/blockdev.c
index 4c1177e8db..9ced07a8c4 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2555,6 +2555,8 @@ void qmp_block_commit(const char *job_id, const char *device,
                       const char *top_node,
                       const char *top,
                       const char *backing_file,
+                      bool has_backing_mask_protocol,
+                      bool backing_mask_protocol,
                       bool has_speed, int64_t speed,
                       bool has_on_error, BlockdevOnError on_error,
                       const char *filter_node_name,
@@ -2585,6 +2587,9 @@ void qmp_block_commit(const char *job_id, const char *device,
     if (has_auto_dismiss && !auto_dismiss) {
         job_flags |= JOB_MANUAL_DISMISS;
     }
+    if (!has_backing_mask_protocol) {
+        backing_mask_protocol = false;
+    }

     /* Important Note:
      *  libvirt relies on the DeviceNotFound error class in order to probe for
@@ -2727,6 +2732,7 @@ void qmp_block_commit(const char *job_id, const char *device,
         }
         commit_start(job_id, bs, base_bs, top_bs, job_flags,
                      speed, on_error, backing_file,
+                     backing_mask_protocol,
                      filter_node_name, &local_err);
     }
     if (local_err != NULL) {
diff --git a/include/block/block-global-state.h b/include/block/block-global-state.h
index 6b21fbc73f..5f4a6da1fe 100644
--- a/include/block/block-global-state.h
+++ b/include/block/block-global-state.h
@@ -145,7 +145,8 @@ int GRAPH_RDLOCK bdrv_make_empty(BdrvChild *c, Error **errp);

 void bdrv_register(BlockDriver *bdrv);
 int bdrv_drop_intermediate(BlockDriverState *top, BlockDriverState *base,
-                           const char *backing_file_str);
+                           const char *backing_file_str,
+                           bool backing_mask_protocol);

 BlockDriverState * GRAPH_RDLOCK
 bdrv_find_overlay(BlockDriverState *active, BlockDriverState *bs);
diff --git a/include/block/block_int-common.h b/include/block/block_int-common.h
index 4e31d161c5..561f35ddb6 100644
--- a/include/block/block_int-common.h
+++ b/include/block/block_int-common.h
@@ -985,7 +985,9 @@ struct BdrvChildClass {
      * can update its reference.
      */
     int (*update_filename)(BdrvChild *child, BlockDriverState *new_base,
-                           const char *filename, Error **errp);
+                           const char *filename,
+                           bool backing_mask_protocol,
+                           Error **errp);

     bool (*change_aio_ctx)(BdrvChild *child, AioContext *ctx,
                            GHashTable *visited, Transaction *tran,
diff --git a/include/block/block_int-global-state.h b/include/block/block_int-global-state.h
index ef31c58bb3..2162269df6 100644
--- a/include/block/block_int-global-state.h
+++ b/include/block/block_int-global-state.h
@@ -82,6 +82,8 @@ void stream_start(const char *job_id, BlockDriverState *bs,
  * @speed: The maximum speed, in bytes per second, or 0 for unlimited.
  * @on_error: The action to take upon error.
  * @backing_file_str: String to use as the backing file in @top's overlay
+ * @backing_mask_protocol: Replace potential protocol name with 'raw' in
+ *                         'backing file format' header
  * @filter_node_name: The node name that should be assigned to the filter
  * driver that the commit job inserts into the graph above @top. NULL means
  * that a node name should be autogenerated.
@@ -92,6 +94,7 @@ void commit_start(const char *job_id, BlockDriverState *bs,
                   BlockDriverState *base, BlockDriverState *top,
                   int creation_flags, int64_t speed,
                   BlockdevOnError on_error, const char *backing_file_str,
+                  bool backing_mask_protocol,
                   const char *filter_node_name, Error **errp);
 /**
  * commit_active_start:
diff --git a/qapi/block-core.json b/qapi/block-core.json
index ca390c5700..726145ec8a 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1810,6 +1810,11 @@
 #     Care should be taken when specifying the string, to specify a
 #     valid filename or protocol.  (Since 2.1)
 #
+# @backing-mask-protocol: If true, replace any protocol mentioned in the
+#     'backing file format' with 'raw', rather than storing the protocol
+#     name as the backing format.  Can be used even when no image header
+#     will be updated (default false; since 9.0).
+#
 # @speed: the maximum speed, in bytes per second
 #
 # @on-error: the action to take on an error.  'ignore' means that the
@@ -1856,7 +1861,8 @@
             '*base': { 'type': 'str', 'features': [ 'deprecated' ] },
             '*top-node': 'str',
             '*top': { 'type': 'str', 'features': [ 'deprecated' ] },
-            '*backing-file': 'str', '*speed': 'int',
+            '*backing-file': 'str', '*backing-mask-protocol': 'bool',
+            '*speed': 'int',
             '*on-error': 'BlockdevOnError',
             '*filter-node-name': 'str',
             '*auto-finalize': 'bool', '*auto-dismiss': 'bool' },
diff --git a/tests/unit/test-bdrv-drain.c b/tests/unit/test-bdrv-drain.c
index 704d1a3f36..1009dd2d18 100644
--- a/tests/unit/test-bdrv-drain.c
+++ b/tests/unit/test-bdrv-drain.c
@@ -1638,6 +1638,7 @@ static const BlockJobDriver test_simple_job_driver = {
 static int drop_intermediate_poll_update_filename(BdrvChild *child,
                                                   BlockDriverState *new_base,
                                                   const char *filename,
+                                                  bool backing_mask_protocol,
                                                   Error **errp)
 {
     /*
@@ -1749,7 +1750,7 @@ static void test_drop_intermediate_poll(void)
     job->should_complete = true;

     g_assert(!job_has_completed);
-    ret = bdrv_drop_intermediate(chain[1], chain[0], NULL);
+    ret = bdrv_drop_intermediate(chain[1], chain[0], NULL, false);
     aio_poll(qemu_get_aio_context(), false);
     g_assert(ret == 0);
     g_assert(job_has_completed);
-- 
2.43.0

[PATCH v3 2/2] block: stream: Allow users to request only format driver names in backing file format

[Peter Krempa]

Introduce a new flag 'backing-mask-protocol' for the block-stream QMP
command which instructs the internals to use 'raw' instead of the
protocol driver in case when a image is used without a dummy 'raw'
wrapper.

The flag is designed such that it can be always asserted by management
tools even when there isn't any update to backing files.

The flag will be used by libvirt so that the backing images still
reference the proper format even when libvirt will stop using the dummy
raw driver (raw driver with no other config). Libvirt needs this so that
the images stay compatible with older libvirt versions which didn't
expect that a protocol driver name can appear in the backing file format
field.

Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
 block/monitor/block-hmp-cmds.c         |  2 +-
 block/stream.c                         | 10 +++++++++-
 blockdev.c                             |  7 +++++++
 include/block/block_int-global-state.h |  3 +++
 qapi/block-core.json                   |  9 ++++++++-
 5 files changed, 28 insertions(+), 3 deletions(-)

diff --git a/block/monitor/block-hmp-cmds.c b/block/monitor/block-hmp-cmds.c
index c729cbf1eb..9080e29d4d 100644
--- a/block/monitor/block-hmp-cmds.c
+++ b/block/monitor/block-hmp-cmds.c
@@ -509,7 +509,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, device, base, NULL, NULL, NULL,
+    qmp_block_stream(device, device, base, NULL, NULL, false, false, NULL,
                      qdict_haskey(qdict, "speed"), speed,
                      true, BLOCKDEV_ON_ERROR_REPORT, NULL,
                      false, false, false, false, &error);
diff --git a/block/stream.c b/block/stream.c
index 01fe7c0f16..da62410dde 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -39,6 +39,7 @@ typedef struct StreamBlockJob {
     BlockDriverState *target_bs;
     BlockdevOnError on_error;
     char *backing_file_str;
+    bool backing_mask_protocol;
     bool bs_read_only;
 } StreamBlockJob;

@@ -95,7 +96,12 @@ static int stream_prepare(Job *job)
         if (unfiltered_base) {
             base_id = s->backing_file_str ?: unfiltered_base->filename;
             if (unfiltered_base->drv) {
-                base_fmt = unfiltered_base->drv->format_name;
+                if (s->backing_mask_protocol &&
+                    unfiltered_base->drv->protocol_name) {
+                    base_fmt = "raw";
+                } else {
+                    base_fmt = unfiltered_base->drv->format_name;
+                }
             }
         }

@@ -247,6 +253,7 @@ static const BlockJobDriver stream_job_driver = {

 void stream_start(const char *job_id, BlockDriverState *bs,
                   BlockDriverState *base, const char *backing_file_str,
+                  bool backing_mask_protocol,
                   BlockDriverState *bottom,
                   int creation_flags, int64_t speed,
                   BlockdevOnError on_error,
@@ -398,6 +405,7 @@ void stream_start(const char *job_id, BlockDriverState *bs,
     s->base_overlay = base_overlay;
     s->above_base = above_base;
     s->backing_file_str = g_strdup(backing_file_str);
+    s->backing_mask_protocol = backing_mask_protocol;
     s->cor_filter_bs = cor_filter_bs;
     s->target_bs = bs;
     s->bs_read_only = bs_read_only;
diff --git a/blockdev.c b/blockdev.c
index 9ced07a8c4..a5ca1cf3ad 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2408,6 +2408,8 @@ void qmp_block_stream(const char *job_id, const char *device,
                       const char *base,
                       const char *base_node,
                       const char *backing_file,
+                      bool has_backing_mask_protocol,
+                      bool backing_mask_protocol,
                       const char *bottom,
                       bool has_speed, int64_t speed,
                       bool has_on_error, BlockdevOnError on_error,
@@ -2443,6 +2445,10 @@ void qmp_block_stream(const char *job_id, const char *device,
         return;
     }

+    if (!has_backing_mask_protocol) {
+        backing_mask_protocol = false;
+    }
+
     if (!has_on_error) {
         on_error = BLOCKDEV_ON_ERROR_REPORT;
     }
@@ -2531,6 +2537,7 @@ void qmp_block_stream(const char *job_id, const char *device,
     }

     stream_start(job_id, bs, base_bs, backing_file,
+                 backing_mask_protocol,
                  bottom_bs, job_flags, has_speed ? speed : 0, on_error,
                  filter_node_name, &local_err);
     if (local_err) {
diff --git a/include/block/block_int-global-state.h b/include/block/block_int-global-state.h
index 2162269df6..d2201e27f4 100644
--- a/include/block/block_int-global-state.h
+++ b/include/block/block_int-global-state.h
@@ -46,6 +46,8 @@
  * flatten the whole backing file chain onto @bs.
  * @backing_file_str: The file name that will be written to @bs as the
  * the new backing file if the job completes. Ignored if @base is %NULL.
+ * @backing_mask_protocol: Replace potential protocol name with 'raw' in
+ *                         'backing file format' header
  * @creation_flags: Flags that control the behavior of the Job lifetime.
  *                  See @BlockJobCreateFlags
  * @speed: The maximum speed, in bytes per second, or 0 for unlimited.
@@ -64,6 +66,7 @@
  */
 void stream_start(const char *job_id, BlockDriverState *bs,
                   BlockDriverState *base, const char *backing_file_str,
+                  bool backing_mask_protocol,
                   BlockDriverState *bottom,
                   int creation_flags, int64_t speed,
                   BlockdevOnError on_error,
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 726145ec8a..04982bff96 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2826,6 +2826,11 @@
 #     Care should be taken when specifying the string, to specify a
 #     valid filename or protocol.  (Since 2.1)
 #
+# @backing-mask-protocol: If true, replace any protocol mentioned in the
+#     'backing file format' with 'raw', rather than storing the protocol
+#     name as the backing format.  Can be used even when no image header
+#     will be updated (default false; since 9.0).
+#
 # @speed: the maximum speed, in bytes per second
 #
 # @on-error: the action to take on an error (default report). 'stop'
@@ -2864,7 +2869,9 @@
 ##
 { 'command': 'block-stream',
   'data': { '*job-id': 'str', 'device': 'str', '*base': 'str',
-            '*base-node': 'str', '*backing-file': 'str', '*bottom': 'str',
+            '*base-node': 'str', '*backing-file': 'str',
+            '*backing-mask-protocol': 'bool',
+            '*bottom': 'str',
             '*speed': 'int', '*on-error': 'BlockdevOnError',
             '*filter-node-name': 'str',
             '*auto-finalize': 'bool', '*auto-dismiss': 'bool' },
-- 
2.43.0

Re: [PATCH v3 0/2] block: commit/stream: Allow users to request only format driver names in backing file format

[Peter Krempa]

On Tue, Dec 05, 2023 at 18:14:40 +0100, Peter Krempa wrote:
> Please see patches for rationale.
> 
> Libvirt patches using this new flag will be posted soon-ish (after
> cleanup).

https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/message/GCCZP5ANYTPVXCIEYGSM5NWYCGDL23V6/

Re: [PATCH v3 0/2] block: commit/stream: Allow users to request only format driver names in backing file format

[Peter Krempa]

On Tue, Dec 05, 2023 at 18:14:40 +0100, Peter Krempa wrote:
> Please see patches for rationale.
> 
> Libvirt patches using this new flag will be posted soon-ish (after
> cleanup).
> 
> v3:
>  - changed name of flag to 'backing-mask-protocol' (Eric)
>  - decided to keep Vladimir's R-b as he requested shorter name too
> 
> v2:
>  - fixed mistaken argument order in 'hmp_block_stream'
>  - changed version in docs to 9.0 as getting this into RC 3 probably
>    isn't realistic
> 
> Peter Krempa (2):
>   block: commit: Allow users to request only format driver names in
>     backing file format
>   block: stream: Allow users to request only format driver names in
>     backing file format

Polite ping, now that qemu-8.2 was released.

Re: [PATCH v3 0/2] block: commit/stream: Allow users to request only format driver names in backing file format

[Kevin Wolf]

Am 05.12.2023 um 18:14 hat Peter Krempa geschrieben:
> Please see patches for rationale.
> 
> Libvirt patches using this new flag will be posted soon-ish (after
> cleanup).
> 
> v3:
>  - changed name of flag to 'backing-mask-protocol' (Eric)
>  - decided to keep Vladimir's R-b as he requested shorter name too
> 
> v2:
>  - fixed mistaken argument order in 'hmp_block_stream'
>  - changed version in docs to 9.0 as getting this into RC 3 probably
>    isn't realistic
> 
> Peter Krempa (2):
>   block: commit: Allow users to request only format driver names in
>     backing file format
>   block: stream: Allow users to request only format driver names in
>     backing file format

Thanks, applied to the block branch.

Kevin