[PATCH 0/4] qcow2: Add keep_data_file command-line option

[PATCH 0/4] qcow2: Add keep_data_file command-line option

[Hanna Czenczek]

Hi,

This series adds a keep_data_file qemu-img create option to qcow2 that
makes it keep the given external data file for a newly created image
instead of overwriting it.

This allows to create a qcow2 image for an existing raw image using the
qemu-img create command, which previously wasn’t easily possible
(besides work-arounds using a temporary data file or qemu-img amend).

(The “proper” way of doing it without this option is to use QMP
blockdev-create.)

This new option is a pure qemu-img create (i.e. command-line) option,
not available via QMP, because it does not make any sense there.  See
patch 1 for more explanation.

(See https://issues.redhat.com/browse/RHEL-73509 for perhaps a bit more
context.)


Hanna Czenczek (4):
  qcow2: Add keep_data_file command-line option
  qcow2: Simplify size round-up in co_create_opts
  iotests/common.filter: Sort keep_data_file
  iotests/244: Add test cases for keep_data_file

 include/block/block_int-common.h |  1 +
 block/qcow2.c                    | 78 +++++++++++++++++++++++++++++---
 tests/qemu-iotests/082.out       | 18 ++++++++
 tests/qemu-iotests/244           | 71 +++++++++++++++++++++++++++++
 tests/qemu-iotests/244.out       | 53 ++++++++++++++++++++++
 tests/qemu-iotests/common.filter |  2 +-
 6 files changed, 216 insertions(+), 7 deletions(-)

-- 
2.49.0

[PATCH 1/4] qcow2: Add keep_data_file command-line option

[Hanna Czenczek]

Add a command-line-only option to prevent overwriting the file specified
as external data file.

This option is only available on the qemu-img create command line, not
via blockdev-create, as it makes no sense there: That interface
separates file creation and formatting, so where the external data file
attached to a newly formatted qcow2 node comes from is completely up to
the user.

Implementation detail: Enabling this option will not only not overwrite
the external data file, but also assume it already exists, for two
reasons:
- It is simpler than checking whether the file exists, and only skipping
  creating it when it does not.  It is therefore also less error-prone,
  i.e. we can never accidentally overwrite an existing file because we
  made some mistake in checking whether it exists.
- I think it makes sense from a user's perspective: You set this option
  when you want to use an existing data file, and you unset it when you
  want a new one.  Getting an error when you expect to use an existing
  data file seems to me a nice warning that something is not right.

Signed-off-by: Hanna Czenczek <hreitz@redhat.com>
---
 include/block/block_int-common.h |  1 +
 block/qcow2.c                    | 75 ++++++++++++++++++++++++++++++--
 tests/qemu-iotests/082.out       | 18 ++++++++
 3 files changed, 90 insertions(+), 4 deletions(-)

diff --git a/include/block/block_int-common.h b/include/block/block_int-common.h
index 2982dd3118..b52e441b42 100644
--- a/include/block/block_int-common.h
+++ b/include/block/block_int-common.h
@@ -57,6 +57,7 @@
 #define BLOCK_OPT_DATA_FILE_RAW     "data_file_raw"
 #define BLOCK_OPT_COMPRESSION_TYPE  "compression_type"
 #define BLOCK_OPT_EXTL2             "extended_l2"
+#define BLOCK_OPT_KEEP_DATA_FILE    "keep_data_file"
 
 #define BLOCK_PROBE_BUF_SIZE        512
 
diff --git a/block/qcow2.c b/block/qcow2.c
index 66fba89b41..b11cbfd859 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -3902,6 +3902,8 @@ qcow2_co_create_opts(BlockDriver *drv, const char *filename, QemuOpts *opts,
     BlockDriverState *bs = NULL;
     BlockDriverState *data_bs = NULL;
     const char *val;
+    bool keep_data_file = false;
+    BlockdevCreateOptionsQcow2 *qcow2_opts;
     int ret;
 
     /* Only the keyval visitor supports the dotted syntax needed for
@@ -3933,6 +3935,22 @@ qcow2_co_create_opts(BlockDriver *drv, const char *filename, QemuOpts *opts,
         qdict_put_str(qdict, BLOCK_OPT_COMPAT_LEVEL, "v3");
     }
 
+    val = qdict_get_try_str(qdict, BLOCK_OPT_KEEP_DATA_FILE);
+    if (val) {
+        if (!strcmp(val, "on")) {
+            keep_data_file = true;
+        } else if (!strcmp(val, "off")) {
+            keep_data_file = false;
+        } else {
+            error_setg(errp,
+                       "Invalid value '%s' for '%s': Must be 'on' or 'off'",
+                       val, BLOCK_OPT_KEEP_DATA_FILE);
+            ret = -EINVAL;
+            goto finish;
+        }
+        qdict_del(qdict, BLOCK_OPT_KEEP_DATA_FILE);
+    }
+
     /* Change legacy command line options into QMP ones */
     static const QDictRenames opt_renames[] = {
         { BLOCK_OPT_BACKING_FILE,       "backing-file" },
@@ -3969,9 +3987,11 @@ qcow2_co_create_opts(BlockDriver *drv, const char *filename, QemuOpts *opts,
     /* Create and open an external data file (protocol layer) */
     val = qdict_get_try_str(qdict, BLOCK_OPT_DATA_FILE);
     if (val) {
-        ret = bdrv_co_create_file(val, opts, errp);
-        if (ret < 0) {
-            goto finish;
+        if (!keep_data_file) {
+            ret = bdrv_co_create_file(val, opts, errp);
+            if (ret < 0) {
+                goto finish;
+            }
         }
 
         data_bs = bdrv_co_open(val, NULL, NULL,
@@ -3984,6 +4004,11 @@ qcow2_co_create_opts(BlockDriver *drv, const char *filename, QemuOpts *opts,
 
         qdict_del(qdict, BLOCK_OPT_DATA_FILE);
         qdict_put_str(qdict, "data-file", data_bs->node_name);
+    } else if (keep_data_file) {
+        error_setg(errp, "Must not use '%s=on' without '%s'",
+                   BLOCK_OPT_KEEP_DATA_FILE, BLOCK_OPT_DATA_FILE);
+        ret = -EINVAL;
+        goto finish;
     }
 
     /* Set 'driver' and 'node' options */
@@ -4004,6 +4029,40 @@ qcow2_co_create_opts(BlockDriver *drv, const char *filename, QemuOpts *opts,
         goto finish;
     }
 
+    qcow2_opts = &create_options->u.qcow2;
+
+    if (!qcow2_opts->has_preallocation) {
+        qcow2_opts->preallocation = PREALLOC_MODE_OFF;
+    }
+    if (!qcow2_opts->has_data_file_raw) {
+        qcow2_opts->data_file_raw = false;
+    }
+
+    if (keep_data_file &&
+        qcow2_opts->preallocation != PREALLOC_MODE_OFF &&
+        qcow2_opts->preallocation != PREALLOC_MODE_METADATA)
+    {
+        error_setg(errp, "Preallocating more than only metadata would "
+                   "overwrite the external data file's content and is "
+                   "therefore incompatible with '%s=on'",
+                   BLOCK_OPT_KEEP_DATA_FILE);
+        ret = -EINVAL;
+        goto finish;
+    }
+
+    if (keep_data_file &&
+        qcow2_opts->preallocation == PREALLOC_MODE_OFF &&
+        !qcow2_opts->data_file_raw)
+    {
+        error_setg(errp, "'%s=on' requires '%s=metadata' or '%s=on', or the "
+                   "file contents will not be visible",
+                   BLOCK_OPT_KEEP_DATA_FILE,
+                   BLOCK_OPT_PREALLOC,
+                   BLOCK_OPT_DATA_FILE_RAW);
+        ret = -EINVAL;
+        goto finish;
+    }
+
     /* Silently round up size */
     create_options->u.qcow2.size = ROUND_UP(create_options->u.qcow2.size,
                                             BDRV_SECTOR_SIZE);
@@ -4014,7 +4073,9 @@ finish:
     if (ret < 0) {
         bdrv_graph_co_rdlock();
         bdrv_co_delete_file_noerr(bs);
-        bdrv_co_delete_file_noerr(data_bs);
+        if (!keep_data_file) {
+            bdrv_co_delete_file_noerr(data_bs);
+        }
         bdrv_graph_co_rdunlock();
     } else {
         ret = 0;
@@ -6113,6 +6174,12 @@ static QemuOptsList qcow2_create_opts = {
             .help = "Compression method used for image cluster "        \
                     "compression",                                      \
             .def_value_str = "zlib"                                     \
+        },                                                              \
+        {                                                               \
+            .name = BLOCK_OPT_KEEP_DATA_FILE,                           \
+            .type = QEMU_OPT_BOOL,                                      \
+            .help = "Assume the external data file already exists and " \
+                    "do not overwrite it"                               \
         },
         QCOW_COMMON_OPTIONS,
         { /* end of list */ }
diff --git a/tests/qemu-iotests/082.out b/tests/qemu-iotests/082.out
index d0dd333117..e0463815c6 100644
--- a/tests/qemu-iotests/082.out
+++ b/tests/qemu-iotests/082.out
@@ -66,6 +66,7 @@ Supported options:
   encryption=<bool (on/off)> - Encrypt the image with format 'aes'. (Deprecated in favor of encrypt.format=aes)
   extended_l2=<bool (on/off)> - Extended L2 tables
   extent_size_hint=<size> - Extent size hint for the image file, 0 to disable
+  keep_data_file=<bool (on/off)> - Assume the external data file already exists and do not overwrite it
   lazy_refcounts=<bool (on/off)> - Postpone refcount updates
   nocow=<bool (on/off)>  - Turn off copy-on-write (valid only on btrfs)
   preallocation=<str>    - Preallocation mode (allowed values: off, metadata, falloc, full)
@@ -92,6 +93,7 @@ Supported options:
   encryption=<bool (on/off)> - Encrypt the image with format 'aes'. (Deprecated in favor of encrypt.format=aes)
   extended_l2=<bool (on/off)> - Extended L2 tables
   extent_size_hint=<size> - Extent size hint for the image file, 0 to disable
+  keep_data_file=<bool (on/off)> - Assume the external data file already exists and do not overwrite it
   lazy_refcounts=<bool (on/off)> - Postpone refcount updates
   nocow=<bool (on/off)>  - Turn off copy-on-write (valid only on btrfs)
   preallocation=<str>    - Preallocation mode (allowed values: off, metadata, falloc, full)
@@ -118,6 +120,7 @@ Supported options:
   encryption=<bool (on/off)> - Encrypt the image with format 'aes'. (Deprecated in favor of encrypt.format=aes)
   extended_l2=<bool (on/off)> - Extended L2 tables
   extent_size_hint=<size> - Extent size hint for the image file, 0 to disable
+  keep_data_file=<bool (on/off)> - Assume the external data file already exists and do not overwrite it
   lazy_refcounts=<bool (on/off)> - Postpone refcount updates
   nocow=<bool (on/off)>  - Turn off copy-on-write (valid only on btrfs)
   preallocation=<str>    - Preallocation mode (allowed values: off, metadata, falloc, full)
@@ -144,6 +147,7 @@ Supported options:
   encryption=<bool (on/off)> - Encrypt the image with format 'aes'. (Deprecated in favor of encrypt.format=aes)
   extended_l2=<bool (on/off)> - Extended L2 tables
   extent_size_hint=<size> - Extent size hint for the image file, 0 to disable
+  keep_data_file=<bool (on/off)> - Assume the external data file already exists and do not overwrite it
   lazy_refcounts=<bool (on/off)> - Postpone refcount updates
   nocow=<bool (on/off)>  - Turn off copy-on-write (valid only on btrfs)
   preallocation=<str>    - Preallocation mode (allowed values: off, metadata, falloc, full)
@@ -170,6 +174,7 @@ Supported options:
   encryption=<bool (on/off)> - Encrypt the image with format 'aes'. (Deprecated in favor of encrypt.format=aes)
   extended_l2=<bool (on/off)> - Extended L2 tables
   extent_size_hint=<size> - Extent size hint for the image file, 0 to disable
+  keep_data_file=<bool (on/off)> - Assume the external data file already exists and do not overwrite it
   lazy_refcounts=<bool (on/off)> - Postpone refcount updates
   nocow=<bool (on/off)>  - Turn off copy-on-write (valid only on btrfs)
   preallocation=<str>    - Preallocation mode (allowed values: off, metadata, falloc, full)
@@ -196,6 +201,7 @@ Supported options:
   encryption=<bool (on/off)> - Encrypt the image with format 'aes'. (Deprecated in favor of encrypt.format=aes)
   extended_l2=<bool (on/off)> - Extended L2 tables
   extent_size_hint=<size> - Extent size hint for the image file, 0 to disable
+  keep_data_file=<bool (on/off)> - Assume the external data file already exists and do not overwrite it
   lazy_refcounts=<bool (on/off)> - Postpone refcount updates
   nocow=<bool (on/off)>  - Turn off copy-on-write (valid only on btrfs)
   preallocation=<str>    - Preallocation mode (allowed values: off, metadata, falloc, full)
@@ -222,6 +228,7 @@ Supported options:
   encryption=<bool (on/off)> - Encrypt the image with format 'aes'. (Deprecated in favor of encrypt.format=aes)
   extended_l2=<bool (on/off)> - Extended L2 tables
   extent_size_hint=<size> - Extent size hint for the image file, 0 to disable
+  keep_data_file=<bool (on/off)> - Assume the external data file already exists and do not overwrite it
   lazy_refcounts=<bool (on/off)> - Postpone refcount updates
   nocow=<bool (on/off)>  - Turn off copy-on-write (valid only on btrfs)
   preallocation=<str>    - Preallocation mode (allowed values: off, metadata, falloc, full)
@@ -248,6 +255,7 @@ Supported options:
   encryption=<bool (on/off)> - Encrypt the image with format 'aes'. (Deprecated in favor of encrypt.format=aes)
   extended_l2=<bool (on/off)> - Extended L2 tables
   extent_size_hint=<size> - Extent size hint for the image file, 0 to disable
+  keep_data_file=<bool (on/off)> - Assume the external data file already exists and do not overwrite it
   lazy_refcounts=<bool (on/off)> - Postpone refcount updates
   nocow=<bool (on/off)>  - Turn off copy-on-write (valid only on btrfs)
   preallocation=<str>    - Preallocation mode (allowed values: off, metadata, falloc, full)
@@ -288,6 +296,7 @@ Supported qcow2 options:
   encrypt.key-secret=<str> - ID of secret providing qcow AES key or LUKS passphrase
   encryption=<bool (on/off)> - Encrypt the image with format 'aes'. (Deprecated in favor of encrypt.format=aes)
   extended_l2=<bool (on/off)> - Extended L2 tables
+  keep_data_file=<bool (on/off)> - Assume the external data file already exists and do not overwrite it
   lazy_refcounts=<bool (on/off)> - Postpone refcount updates
   preallocation=<str>    - Preallocation mode (allowed values: off, metadata, falloc, full)
   refcount_bits=<num>    - Width of a reference count entry in bits
@@ -376,6 +385,7 @@ Supported options:
   encryption=<bool (on/off)> - Encrypt the image with format 'aes'. (Deprecated in favor of encrypt.format=aes)
   extended_l2=<bool (on/off)> - Extended L2 tables
   extent_size_hint=<size> - Extent size hint for the image file, 0 to disable
+  keep_data_file=<bool (on/off)> - Assume the external data file already exists and do not overwrite it
   lazy_refcounts=<bool (on/off)> - Postpone refcount updates
   nocow=<bool (on/off)>  - Turn off copy-on-write (valid only on btrfs)
   preallocation=<str>    - Preallocation mode (allowed values: off, metadata, falloc, full)
@@ -402,6 +412,7 @@ Supported options:
   encryption=<bool (on/off)> - Encrypt the image with format 'aes'. (Deprecated in favor of encrypt.format=aes)
   extended_l2=<bool (on/off)> - Extended L2 tables
   extent_size_hint=<size> - Extent size hint for the image file, 0 to disable
+  keep_data_file=<bool (on/off)> - Assume the external data file already exists and do not overwrite it
   lazy_refcounts=<bool (on/off)> - Postpone refcount updates
   nocow=<bool (on/off)>  - Turn off copy-on-write (valid only on btrfs)
   preallocation=<str>    - Preallocation mode (allowed values: off, metadata, falloc, full)
@@ -428,6 +439,7 @@ Supported options:
   encryption=<bool (on/off)> - Encrypt the image with format 'aes'. (Deprecated in favor of encrypt.format=aes)
   extended_l2=<bool (on/off)> - Extended L2 tables
   extent_size_hint=<size> - Extent size hint for the image file, 0 to disable
+  keep_data_file=<bool (on/off)> - Assume the external data file already exists and do not overwrite it
   lazy_refcounts=<bool (on/off)> - Postpone refcount updates
   nocow=<bool (on/off)>  - Turn off copy-on-write (valid only on btrfs)
   preallocation=<str>    - Preallocation mode (allowed values: off, metadata, falloc, full)
@@ -454,6 +466,7 @@ Supported options:
   encryption=<bool (on/off)> - Encrypt the image with format 'aes'. (Deprecated in favor of encrypt.format=aes)
   extended_l2=<bool (on/off)> - Extended L2 tables
   extent_size_hint=<size> - Extent size hint for the image file, 0 to disable
+  keep_data_file=<bool (on/off)> - Assume the external data file already exists and do not overwrite it
   lazy_refcounts=<bool (on/off)> - Postpone refcount updates
   nocow=<bool (on/off)>  - Turn off copy-on-write (valid only on btrfs)
   preallocation=<str>    - Preallocation mode (allowed values: off, metadata, falloc, full)
@@ -480,6 +493,7 @@ Supported options:
   encryption=<bool (on/off)> - Encrypt the image with format 'aes'. (Deprecated in favor of encrypt.format=aes)
   extended_l2=<bool (on/off)> - Extended L2 tables
   extent_size_hint=<size> - Extent size hint for the image file, 0 to disable
+  keep_data_file=<bool (on/off)> - Assume the external data file already exists and do not overwrite it
   lazy_refcounts=<bool (on/off)> - Postpone refcount updates
   nocow=<bool (on/off)>  - Turn off copy-on-write (valid only on btrfs)
   preallocation=<str>    - Preallocation mode (allowed values: off, metadata, falloc, full)
@@ -506,6 +520,7 @@ Supported options:
   encryption=<bool (on/off)> - Encrypt the image with format 'aes'. (Deprecated in favor of encrypt.format=aes)
   extended_l2=<bool (on/off)> - Extended L2 tables
   extent_size_hint=<size> - Extent size hint for the image file, 0 to disable
+  keep_data_file=<bool (on/off)> - Assume the external data file already exists and do not overwrite it
   lazy_refcounts=<bool (on/off)> - Postpone refcount updates
   nocow=<bool (on/off)>  - Turn off copy-on-write (valid only on btrfs)
   preallocation=<str>    - Preallocation mode (allowed values: off, metadata, falloc, full)
@@ -532,6 +547,7 @@ Supported options:
   encryption=<bool (on/off)> - Encrypt the image with format 'aes'. (Deprecated in favor of encrypt.format=aes)
   extended_l2=<bool (on/off)> - Extended L2 tables
   extent_size_hint=<size> - Extent size hint for the image file, 0 to disable
+  keep_data_file=<bool (on/off)> - Assume the external data file already exists and do not overwrite it
   lazy_refcounts=<bool (on/off)> - Postpone refcount updates
   nocow=<bool (on/off)>  - Turn off copy-on-write (valid only on btrfs)
   preallocation=<str>    - Preallocation mode (allowed values: off, metadata, falloc, full)
@@ -558,6 +574,7 @@ Supported options:
   encryption=<bool (on/off)> - Encrypt the image with format 'aes'. (Deprecated in favor of encrypt.format=aes)
   extended_l2=<bool (on/off)> - Extended L2 tables
   extent_size_hint=<size> - Extent size hint for the image file, 0 to disable
+  keep_data_file=<bool (on/off)> - Assume the external data file already exists and do not overwrite it
   lazy_refcounts=<bool (on/off)> - Postpone refcount updates
   nocow=<bool (on/off)>  - Turn off copy-on-write (valid only on btrfs)
   preallocation=<str>    - Preallocation mode (allowed values: off, metadata, falloc, full)
@@ -598,6 +615,7 @@ Supported qcow2 options:
   encrypt.key-secret=<str> - ID of secret providing qcow AES key or LUKS passphrase
   encryption=<bool (on/off)> - Encrypt the image with format 'aes'. (Deprecated in favor of encrypt.format=aes)
   extended_l2=<bool (on/off)> - Extended L2 tables
+  keep_data_file=<bool (on/off)> - Assume the external data file already exists and do not overwrite it
   lazy_refcounts=<bool (on/off)> - Postpone refcount updates
   preallocation=<str>    - Preallocation mode (allowed values: off, metadata, falloc, full)
   refcount_bits=<num>    - Width of a reference count entry in bits
-- 
2.49.0

[PATCH 2/4] qcow2: Simplify size round-up in co_create_opts

[Hanna Czenczek]

Use the now-existing qcow2_opts pointer to simplify the size rounding up
code.

Signed-off-by: Hanna Czenczek <hreitz@redhat.com>
---
 block/qcow2.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index b11cbfd859..988ebcf138 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -4064,8 +4064,7 @@ qcow2_co_create_opts(BlockDriver *drv, const char *filename, QemuOpts *opts,
     }
 
     /* Silently round up size */
-    create_options->u.qcow2.size = ROUND_UP(create_options->u.qcow2.size,
-                                            BDRV_SECTOR_SIZE);
+    qcow2_opts->size = ROUND_UP(qcow2_opts->size, BDRV_SECTOR_SIZE);
 
     /* Create the qcow2 image (format layer) */
     ret = qcow2_co_create(create_options, errp);
-- 
2.49.0

[PATCH 3/4] iotests/common.filter: Sort keep_data_file

[Hanna Czenczek]

Sort the new keep_data_file creation option together with data_file and
data_file_raw.

Signed-off-by: Hanna Czenczek <hreitz@redhat.com>
---
 tests/qemu-iotests/common.filter | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/common.filter b/tests/qemu-iotests/common.filter
index fc3c64bcb8..3744326d2d 100644
--- a/tests/qemu-iotests/common.filter
+++ b/tests/qemu-iotests/common.filter
@@ -176,7 +176,7 @@ _do_filter_img_create()
             -e 's/^\(fmt\)/0-\1/' \
             -e 's/^\(size\)/1-\1/' \
             -e 's/^\(backing\)/2-\1/' \
-            -e 's/^\(data_file\)/3-\1/' \
+            -e 's/^\(\(keep_\)\?data_file\)/3-\1/' \
             -e 's/^\(encryption\)/4-\1/' \
             -e 's/^\(preallocation\)/8-\1/' \
         | LC_ALL=C sort \
-- 
2.49.0

[PATCH 4/4] iotests/244: Add test cases for keep_data_file

[Hanna Czenczek]

Add various test cases around keep_data_file to the existing data_file
test suite 244.

Signed-off-by: Hanna Czenczek <hreitz@redhat.com>
---
 tests/qemu-iotests/244     | 71 ++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/244.out | 53 ++++++++++++++++++++++++++++
 2 files changed, 124 insertions(+)

diff --git a/tests/qemu-iotests/244 b/tests/qemu-iotests/244
index bb9cc6512f..ec81df8d6a 100755
--- a/tests/qemu-iotests/244
+++ b/tests/qemu-iotests/244
@@ -384,6 +384,77 @@ $QEMU_IMG compare --image-opts \
     "driver=raw,file.filename=$TEST_IMG.data"  \
     "file.filename=$TEST_IMG,backing.file.filename=$TEST_IMG.base"
 
+echo
+echo '=== keep_data_file tests ==='
+
+echo
+echo '--- Creating test data file ---'
+
+# Easiest way to create the raw data file without having to create and
+# access it manually
+_make_test_img -o "data_file=$TEST_IMG.data,data_file_raw=on" 1M
+# Values chosen by a fair random.org evaluation
+$QEMU_IO -c 'write -P 3 0 512k' -c 'write -P 96 512k 512k' "$TEST_IMG" |
+    _filter_qemu_io
+
+echo
+echo '--- Testing stand-alone option ---'
+
+# Cannot work, needs data file
+_make_test_img -o "keep_data_file=on" 1M
+
+# Invalid option value
+_make_test_img -o "keep_data_file=true" 1M
+
+# Should be the same as omitting
+_make_test_img -o "keep_data_file=off" 1M
+
+# No preallocation is OK when also specifying data_file_raw; otherwise, none of
+# the data file will be mapped, i.e. its contents will stay hidden, so
+# requesting its contents to be kept (but hidden) doesn't make much sense.
+#
+# Metadata preallocation is OK: It will not overwrite the data file's contents,
+# but ensure the contents are mapped and visible.
+#
+# Any data preallocation (like falloc) is not OK, as this would overwrite the
+# data file's contents despite keep_data_file requesting they should not be
+# overwritten.
+#
+# Note that all of these cases use the data file created above: This verifies
+# that when passing keep_data_file=on, the data file is always kept as-is (and
+# e.g. not deleted on error).
+for prealloc in off metadata falloc; do
+    # Without metadata preallocation, the data_file_raw flag is required so that
+    # the data file's contents are visible.
+    for data_file_raw in off on; do
+        echo
+        echo "--- Testing prealloc=$prealloc data_file_raw=$data_file_raw ---"
+
+        # Remove previously existing qcow2 (metadata) file
+        _cleanup_test_img
+
+        opts="data_file=$TEST_IMG.data,keep_data_file=on"
+        opts+=",preallocation=$prealloc"
+        opts+=",data_file_raw=$data_file_raw"
+
+        _make_test_img -o "$opts" 1M
+        if [ -f "$TEST_IMG" ]; then
+            $QEMU_IO -c 'read -P 3 0 512k' -c 'read -P 96 512k 512k' "$TEST_IMG" |
+                _filter_qemu_io
+        fi
+    done
+done
+
+echo
+echo '--- Testing non-existent data file ---'
+
+# Maybe a matter of taste whether this should fail or create the file, but
+# failing is simpler (= will always skip create) and seems safer (users may
+# expect the file to exist, and the error will warn them when it does not).
+_make_test_img \
+    -o "data_file=$TEST_IMG.doesnotexist,keep_data_file=on,data_file_raw=on" \
+    1M
+
 # success, all done
 echo "*** done"
 rm -f $seq.full
diff --git a/tests/qemu-iotests/244.out b/tests/qemu-iotests/244.out
index f46cfe93f1..9fbfa82cd9 100644
--- a/tests/qemu-iotests/244.out
+++ b/tests/qemu-iotests/244.out
@@ -197,4 +197,57 @@ wrote 1048576/1048576 bytes at offset 0
 
 Comparing qcow2 image and raw data file:
 Images are identical.
+
+=== keep_data_file tests ===
+
+--- Creating test data file ---
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048576 data_file=TEST_DIR/t.IMGFMT.data data_file_raw=on
+wrote 524288/524288 bytes at offset 0
+512 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+wrote 524288/524288 bytes at offset 524288
+512 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+--- Testing stand-alone option ---
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048576 keep_data_file=on
+qemu-img: TEST_DIR/t.IMGFMT: Must not use 'keep_data_file=on' without 'data_file'
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048576 keep_data_file=true
+qemu-img: TEST_DIR/t.IMGFMT: Invalid value 'true' for 'keep_data_file': Must be 'on' or 'off'
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048576 keep_data_file=off
+
+--- Testing prealloc=off data_file_raw=off ---
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048576 data_file=TEST_DIR/t.IMGFMT.data data_file_raw=off keep_data_file=on preallocation=off
+qemu-img: TEST_DIR/t.IMGFMT: 'keep_data_file=on' requires 'preallocation=metadata' or 'data_file_raw=on', or the file contents will not be visible
+
+--- Testing prealloc=off data_file_raw=on ---
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048576 data_file=TEST_DIR/t.IMGFMT.data data_file_raw=on keep_data_file=on preallocation=off
+read 524288/524288 bytes at offset 0
+512 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 524288/524288 bytes at offset 524288
+512 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+--- Testing prealloc=metadata data_file_raw=off ---
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048576 data_file=TEST_DIR/t.IMGFMT.data data_file_raw=off keep_data_file=on preallocation=metadata
+read 524288/524288 bytes at offset 0
+512 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 524288/524288 bytes at offset 524288
+512 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+--- Testing prealloc=metadata data_file_raw=on ---
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048576 data_file=TEST_DIR/t.IMGFMT.data data_file_raw=on keep_data_file=on preallocation=metadata
+read 524288/524288 bytes at offset 0
+512 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 524288/524288 bytes at offset 524288
+512 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+--- Testing prealloc=falloc data_file_raw=off ---
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048576 data_file=TEST_DIR/t.IMGFMT.data data_file_raw=off keep_data_file=on preallocation=falloc
+qemu-img: TEST_DIR/t.IMGFMT: Preallocating more than only metadata would overwrite the external data file's content and is therefore incompatible with 'keep_data_file=on'
+
+--- Testing prealloc=falloc data_file_raw=on ---
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048576 data_file=TEST_DIR/t.IMGFMT.data data_file_raw=on keep_data_file=on preallocation=falloc
+qemu-img: TEST_DIR/t.IMGFMT: Preallocating more than only metadata would overwrite the external data file's content and is therefore incompatible with 'keep_data_file=on'
+
+--- Testing non-existent data file ---
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048576 data_file=TEST_DIR/t.IMGFMT.doesnotexist data_file_raw=on keep_data_file=on
+qemu-img: TEST_DIR/t.IMGFMT: Could not open 'TEST_DIR/t.IMGFMT.doesnotexist': No such file or directory
 *** done
-- 
2.49.0

Re: [PATCH 1/4] qcow2: Add keep_data_file command-line option

[Eric Blake]

On Fri, May 30, 2025 at 10:44:44AM +0200, Hanna Czenczek wrote:
> Add a command-line-only option to prevent overwriting the file specified
> as external data file.
> 
> This option is only available on the qemu-img create command line, not
> via blockdev-create, as it makes no sense there: That interface
> separates file creation and formatting, so where the external data file
> attached to a newly formatted qcow2 node comes from is completely up to
> the user.
> 
> Implementation detail: Enabling this option will not only not overwrite
> the external data file, but also assume it already exists, for two
> reasons:
> - It is simpler than checking whether the file exists, and only skipping
>   creating it when it does not.  It is therefore also less error-prone,
>   i.e. we can never accidentally overwrite an existing file because we
>   made some mistake in checking whether it exists.
> - I think it makes sense from a user's perspective: You set this option
>   when you want to use an existing data file, and you unset it when you
>   want a new one.  Getting an error when you expect to use an existing
>   data file seems to me a nice warning that something is not right.

And it also helps if you accidentally type something like nbd://... as
the data file and expect it to exist, but qemu interprets that as a
local filename ./nbd:/... which (hopefully) does not exist.

> 
> Signed-off-by: Hanna Czenczek <hreitz@redhat.com>
> ---
>  include/block/block_int-common.h |  1 +
>  block/qcow2.c                    | 75 ++++++++++++++++++++++++++++++--
>  tests/qemu-iotests/082.out       | 18 ++++++++
>  3 files changed, 90 insertions(+), 4 deletions(-)
> 
> @@ -3933,6 +3935,22 @@ qcow2_co_create_opts(BlockDriver *drv, const char *filename, QemuOpts *opts,
>          qdict_put_str(qdict, BLOCK_OPT_COMPAT_LEVEL, "v3");
>      }
>  
> +    val = qdict_get_try_str(qdict, BLOCK_OPT_KEEP_DATA_FILE);
> +    if (val) {
> +        if (!strcmp(val, "on")) {
> +            keep_data_file = true;
> +        } else if (!strcmp(val, "off")) {
> +            keep_data_file = false;
> +        } else {
> +            error_setg(errp,
> +                       "Invalid value '%s' for '%s': Must be 'on' or 'off'",
> +                       val, BLOCK_OPT_KEEP_DATA_FILE);

Do we have a function that does this parsing so that we don't have to
open-code copy-paste (with slight differences) this sort of parsing
into similar clients?  A quick grep for "'on' or 'off'" shows at least:

monitor/hmp.c:                    monitor_printf(mon, "Expected 'on' or 'off'\n");
net/filter.c:                         "should be 'on' or 'off'");
qapi/qapi-util.c:               "'on' or 'off'");
qapi/qobject-input-visitor.c:                   full_name(qiv, name), "'on' or 'off'");

Not necessarily a show-stopper to this patch, but food for thought in
the technical debt department.

> @@ -4004,6 +4029,40 @@ qcow2_co_create_opts(BlockDriver *drv, const char *filename, QemuOpts *opts,
>          goto finish;
>      }
>  
> +    qcow2_opts = &create_options->u.qcow2;
> +
> +    if (!qcow2_opts->has_preallocation) {
> +        qcow2_opts->preallocation = PREALLOC_MODE_OFF;
> +    }
> +    if (!qcow2_opts->has_data_file_raw) {
> +        qcow2_opts->data_file_raw = false;
> +    }
> +
> +    if (keep_data_file &&
> +        qcow2_opts->preallocation != PREALLOC_MODE_OFF &&
> +        qcow2_opts->preallocation != PREALLOC_MODE_METADATA)
> +    {
> +        error_setg(errp, "Preallocating more than only metadata would "
> +                   "overwrite the external data file's content and is "
> +                   "therefore incompatible with '%s=on'",
> +                   BLOCK_OPT_KEEP_DATA_FILE);

I like this safety check.

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

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization:  qemu.org | libguestfs.org

Re: [PATCH 2/4] qcow2: Simplify size round-up in co_create_opts

[Eric Blake]

On Fri, May 30, 2025 at 10:44:45AM +0200, Hanna Czenczek wrote:
> Use the now-existing qcow2_opts pointer to simplify the size rounding up
> code.
> 
> Signed-off-by: Hanna Czenczek <hreitz@redhat.com>
> ---
>  block/qcow2.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)

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

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization:  qemu.org | libguestfs.org

Re: [PATCH 3/4] iotests/common.filter: Sort keep_data_file

[Eric Blake]

On Fri, May 30, 2025 at 10:44:46AM +0200, Hanna Czenczek wrote:
> Sort the new keep_data_file creation option together with data_file and
> data_file_raw.
> 
> Signed-off-by: Hanna Czenczek <hreitz@redhat.com>
> ---
>  tests/qemu-iotests/common.filter | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

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

> 
> diff --git a/tests/qemu-iotests/common.filter b/tests/qemu-iotests/common.filter
> index fc3c64bcb8..3744326d2d 100644
> --- a/tests/qemu-iotests/common.filter
> +++ b/tests/qemu-iotests/common.filter
> @@ -176,7 +176,7 @@ _do_filter_img_create()
>              -e 's/^\(fmt\)/0-\1/' \
>              -e 's/^\(size\)/1-\1/' \
>              -e 's/^\(backing\)/2-\1/' \
> -            -e 's/^\(data_file\)/3-\1/' \
> +            -e 's/^\(\(keep_\)\?data_file\)/3-\1/' \
>              -e 's/^\(encryption\)/4-\1/' \
>              -e 's/^\(preallocation\)/8-\1/' \
>          | LC_ALL=C sort \
> -- 
> 2.49.0
> 
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization:  qemu.org | libguestfs.org

Re: [PATCH 4/4] iotests/244: Add test cases for keep_data_file

[Eric Blake]

On Fri, May 30, 2025 at 10:44:47AM +0200, Hanna Czenczek wrote:
> Add various test cases around keep_data_file to the existing data_file
> test suite 244.
> 
> Signed-off-by: Hanna Czenczek <hreitz@redhat.com>
> ---
>  tests/qemu-iotests/244     | 71 ++++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/244.out | 53 ++++++++++++++++++++++++++++
>  2 files changed, 124 insertions(+)
> 
> diff --git a/tests/qemu-iotests/244 b/tests/qemu-iotests/244
> index bb9cc6512f..ec81df8d6a 100755
> --- a/tests/qemu-iotests/244
> +++ b/tests/qemu-iotests/244
> @@ -384,6 +384,77 @@ $QEMU_IMG compare --image-opts \
>      "driver=raw,file.filename=$TEST_IMG.data"  \
>      "file.filename=$TEST_IMG,backing.file.filename=$TEST_IMG.base"
>  

> +
> +# Maybe a matter of taste whether this should fail or create the file, but
> +# failing is simpler (= will always skip create) and seems safer (users may
> +# expect the file to exist, and the error will warn them when it does not).
> +_make_test_img \
> +    -o "data_file=$TEST_IMG.doesnotexist,keep_data_file=on,data_file_raw=on" \
> +    1M

I agree with the failure here.

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

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization:  qemu.org | libguestfs.org

Re: [PATCH 0/4] qcow2: Add keep_data_file command-line option

[Hanna Czenczek]

Ping

On 30.05.25 10:44, Hanna Czenczek wrote:
> Hi,
>
> This series adds a keep_data_file qemu-img create option to qcow2 that
> makes it keep the given external data file for a newly created image
> instead of overwriting it.
>
> This allows to create a qcow2 image for an existing raw image using the
> qemu-img create command, which previously wasn’t easily possible
> (besides work-arounds using a temporary data file or qemu-img amend).
>
> (The “proper” way of doing it without this option is to use QMP
> blockdev-create.)
>
> This new option is a pure qemu-img create (i.e. command-line) option,
> not available via QMP, because it does not make any sense there.  See
> patch 1 for more explanation.
>
> (See https://issues.redhat.com/browse/RHEL-73509 for perhaps a bit more
> context.)
>
>
> Hanna Czenczek (4):
>    qcow2: Add keep_data_file command-line option
>    qcow2: Simplify size round-up in co_create_opts
>    iotests/common.filter: Sort keep_data_file
>    iotests/244: Add test cases for keep_data_file
>
>   include/block/block_int-common.h |  1 +
>   block/qcow2.c                    | 78 +++++++++++++++++++++++++++++---
>   tests/qemu-iotests/082.out       | 18 ++++++++
>   tests/qemu-iotests/244           | 71 +++++++++++++++++++++++++++++
>   tests/qemu-iotests/244.out       | 53 ++++++++++++++++++++++
>   tests/qemu-iotests/common.filter |  2 +-
>   6 files changed, 216 insertions(+), 7 deletions(-)
>