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 <hre...@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 <ebl...@redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization: qemu.org | libguestfs.org
