[PATCH 0/9] qemu: Fix few bugs in snapshot/save code (background snapshot saga part 1)
While looking trhough the snapshot code I've noticed a few bugs: - memleak in cleanup code of migration to file (1/9) - wrong place where 'manual' external disk snapshot is taken (2/9) - bad interaction between 'manual' snapshot and the _QUIESCE flag (3/9) - constants for save image types not exposed (6/9) - use of wrong 'flags' in migration to file code (8/9) The series contains also few cleanups and improvements: - bash completion for save image type (7/9) - use of proper types for save image type (4,5/9) - avoid problems with 'flags' in qemuMigrationParamsForSave (9/9) Part 2 will focus on some code cleanups. Peter Krempa (9): qemuMigrationSrcToFile: Don't leak 'qemuFDPass' in cleanup path qemu: snapshot: Setup disks for manual snapshot only when the VM is actually paused qemuSnapshotPrepare: Prohibit 'manual' disk snapshot mode with VIR_DOMAIN_SNAPSHOT_CREATE_QUIESCE flag qemu: Use 'virQEMUSaveFormat' type everywhere except qemu_conf qemu: conf: Use proper type for (save|dump|snapshot)ImageFormat include: Create constants for save image format values virsh: Add completer for '--image-format' option of 'save' command qemuMigrationSrcToFile: Don't cross-contaminate 'flags' variable qemuMigrationParamsForSave: Don't take opaque 'flags' include/libvirt/libvirt-domain.h | 87 ++++++++++++++++++++++++++++++-- src/qemu/qemu_conf.c | 48 ++++++++++++------ src/qemu/qemu_conf.h | 7 +-- src/qemu/qemu_driver.c | 29 +++++++---- src/qemu/qemu_migration.c | 14 ++--- src/qemu/qemu_migration.h | 2 +- src/qemu/qemu_migration_params.c | 4 +- src/qemu/qemu_migration_params.h | 2 +- src/qemu/qemu_saveimage.c | 22 ++++---- src/qemu/qemu_saveimage.h | 24 ++------- src/qemu/qemu_saveimage_format.h | 25 +++++++++ src/qemu/qemu_snapshot.c | 14 +++-- tools/virsh-domain.c | 21 ++++++++ 13 files changed, 224 insertions(+), 75 deletions(-) create mode 100644 src/qemu/qemu_saveimage_format.h -- 2.52.0
From: Peter Krempa <pkrempa@redhat.com> A temporary 'qemuFDPass' is used when cleaning up after a migration to a file but it's not freed after use. Declare it as autoptr. Fixes: c2518f7bc7d Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_migration.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 9059f9aa3a..4f9b649b63 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -7325,11 +7325,13 @@ qemuMigrationSrcToFile(virQEMUDriver *driver, virDomainObj *vm, /* Remove fdset passed to qemu and restore max migration bandwidth */ if (qemuDomainObjIsActive(vm)) { if (qemuDomainObjEnterMonitorAsync(vm, asyncJob) == 0) { - qemuFDPass *fdPass = - qemuFDPassNewFromMonitor("libvirt-outgoing-migrate", priv->mon); + g_autoptr(qemuFDPass) fdPass = NULL; + + fdPass = qemuFDPassNewFromMonitor("libvirt-outgoing-migrate", priv->mon); if (fdPass) qemuFDPassTransferMonitorRollback(fdPass, priv->mon); + qemuDomainObjExitMonitor(vm); } -- 2.52.0
From: Peter Krempa <pkrempa@redhat.com> When creating a snapshot with 'VIR_DOMAIN_SNAPSHOT_CREATE_LIVE' the VM is paused only after dumping the memory state. This means that also the steps to do a 'manual' disk snapshot (deactivation of the block nodes in qemu) must happen only once the VM is paused. Move the manual snapshot setup code after the memory snapshot code. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_snapshot.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index 302775af92..bf25c70826 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -1707,10 +1707,6 @@ qemuSnapshotCreateActiveExternal(virQEMUDriver *driver, } } - if (has_manual && - qemuSnapshotCreateActiveExternalDisksManual(vm, snap, VIR_ASYNC_JOB_SNAPSHOT) < 0) - goto cleanup; - /* We need to collect reply from 'query-named-block-nodes' prior to the * migration step as qemu deactivates bitmaps after migration so the result * would be wrong */ @@ -1769,6 +1765,10 @@ qemuSnapshotCreateActiveExternal(virQEMUDriver *driver, /* the domain is now paused if a memory snapshot was requested */ + if (has_manual && + qemuSnapshotCreateActiveExternalDisksManual(vm, snap, VIR_ASYNC_JOB_SNAPSHOT) < 0) + goto cleanup; + if ((ret = qemuSnapshotCreateActiveExternalDisks(vm, snap, blockNamedNodeData, flags, VIR_ASYNC_JOB_SNAPSHOT)) < 0) -- 2.52.0
From: Peter Krempa <pkrempa@redhat.com> If the snapshot has a disk using 'manual' snapshot mode we keep the VM paused until the user resumes it (presumably after they've done steps to take the disk snapshot). Since quiescing is done via the guest agent this means it will not be possible while the VM is paused. Rather than trying to implement complex recovery from this state prevent the use of VIR_DOMAIN_SNAPSHOT_CREATE_QUIESCE for the snapshot. The user still can call virDomainFSFreeze/virDomainFSThaw manually. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_snapshot.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index bf25c70826..5b0b52e2ba 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -1075,6 +1075,12 @@ qemuSnapshotPrepare(virDomainObj *vm, } } + if (*has_manual && (*flags & VIR_DOMAIN_SNAPSHOT_CREATE_QUIESCE)) { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", + _("'manual' disk snapshot mode requires explicit quiescing (VIR_DOMAIN_SNAPSHOT_CREATE_QUIESCE is not supported)")); + return -1; + } + /* Alter flags to let later users know what we learned. */ if (external && !active) *flags |= VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY; -- 2.52.0
From: Peter Krempa <pkrempa@redhat.com> Convert all code refering to the save image type to use the proper enum value. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 18 ++++++++++++------ src/qemu/qemu_saveimage.c | 4 ++-- src/qemu/qemu_saveimage.h | 4 ++-- 3 files changed, 16 insertions(+), 10 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index f2e024dae3..d88c0833ad 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -2583,7 +2583,7 @@ static int qemuDomainSaveInternal(virQEMUDriver *driver, virDomainObj *vm, const char *path, - int format, + virQEMUSaveFormat format, virCommand *compressor, const char *xmlin, virTypedParameterPtr params, @@ -2823,7 +2823,7 @@ qemuDomainSaveParams(virDomainPtr dom, const char *to = NULL; const char *dxml = NULL; const char *formatstr = NULL; - int format = cfg->saveImageFormat; + virQEMUSaveFormat format = cfg->saveImageFormat; int ret = -1; virCheckFlags(VIR_DOMAIN_SAVE_BYPASS_CACHE | @@ -2863,10 +2863,16 @@ qemuDomainSaveParams(virDomainPtr dom, return qemuDomainManagedSaveHelper(driver, vm, dxml, flags); } - if (formatstr && (format = qemuSaveFormatTypeFromString(formatstr)) < 0) { - virReportError(VIR_ERR_OPERATION_FAILED, - _("Invalid image_format '%1$s'"), formatstr); - goto cleanup; + if (formatstr) { + int formatVal; + + if ((formatVal = qemuSaveFormatTypeFromString(formatstr)) < 0) { + virReportError(VIR_ERR_OPERATION_FAILED, + _("Invalid image_format '%1$s'"), formatstr); + goto cleanup; + } + + format = formatVal; } if (qemuSaveImageGetCompressionProgram(format, &compressor, "save") < 0) diff --git a/src/qemu/qemu_saveimage.c b/src/qemu/qemu_saveimage.c index 145a0f4832..48f8220dee 100644 --- a/src/qemu/qemu_saveimage.c +++ b/src/qemu/qemu_saveimage.c @@ -79,7 +79,7 @@ virQEMUSaveData * virQEMUSaveDataNew(char *domXML, qemuDomainSaveCookie *cookieObj, bool running, - int format, + virQEMUSaveFormat format, virDomainXMLOption *xmlopt) { virQEMUSaveData *data = NULL; @@ -551,7 +551,7 @@ qemuSaveImageCreate(virQEMUDriver *driver, * Returns -1 on failure, 0 on success. */ int -qemuSaveImageGetCompressionProgram(int format, +qemuSaveImageGetCompressionProgram(virQEMUSaveFormat format, virCommand **compressor, const char *styleFormat) { diff --git a/src/qemu/qemu_saveimage.h b/src/qemu/qemu_saveimage.h index 15b73eb395..0a22ee5f05 100644 --- a/src/qemu/qemu_saveimage.h +++ b/src/qemu/qemu_saveimage.h @@ -114,7 +114,7 @@ qemuSaveImageOpen(virQEMUDriver *driver, ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(4); int -qemuSaveImageGetCompressionProgram(int format, +qemuSaveImageGetCompressionProgram(virQEMUSaveFormat format, virCommand **compressor, const char *styleFormat) ATTRIBUTE_NONNULL(2); @@ -153,7 +153,7 @@ virQEMUSaveData * virQEMUSaveDataNew(char *domXML, qemuDomainSaveCookie *cookieObj, bool running, - int format, + virQEMUSaveFormat format, virDomainXMLOption *xmlopt); void -- 2.52.0
From: Peter Krempa <pkrempa@redhat.com> Extract the definition of the enum into a separate header file and convert the config struct to use the proper types. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_conf.c | 48 ++++++++++++++++++++++---------- src/qemu/qemu_conf.h | 7 +++-- src/qemu/qemu_saveimage.h | 20 ++----------- src/qemu/qemu_saveimage_format.h | 25 +++++++++++++++++ 4 files changed, 64 insertions(+), 36 deletions(-) create mode 100644 src/qemu/qemu_saveimage_format.h diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 242955200a..de6e51177a 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -686,29 +686,47 @@ virQEMUDriverConfigLoadSaveEntry(virQEMUDriverConfig *cfg, if (virConfGetValueString(conf, "save_image_format", &savestr) < 0) return -1; - if (savestr && (cfg->saveImageFormat = qemuSaveFormatTypeFromString(savestr)) < 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("Invalid save_image_format '%1$s'"), - savestr); - return -1; + if (savestr) { + int formatVal; + + if ((formatVal = qemuSaveFormatTypeFromString(savestr)) < 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Invalid save_image_format '%1$s'"), + savestr); + return -1; + } + + cfg->saveImageFormat = formatVal; } if (virConfGetValueString(conf, "dump_image_format", &dumpstr) < 0) return -1; - if (dumpstr && (cfg->dumpImageFormat = qemuSaveFormatTypeFromString(dumpstr)) < 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("Invalid dump_image_format '%1$s'"), - dumpstr); - return -1; + if (dumpstr) { + int formatVal; + + if ((formatVal = qemuSaveFormatTypeFromString(dumpstr)) < 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Invalid dump_image_format '%1$s'"), + dumpstr); + return -1; + } + + cfg->dumpImageFormat = formatVal; } if (virConfGetValueString(conf, "snapshot_image_format", &snapstr) < 0) return -1; - if (snapstr && (cfg->snapshotImageFormat = qemuSaveFormatTypeFromString(snapstr)) < 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("Invalid snapshot_image_format '%1$s'"), - snapstr); - return -1; + if (snapstr) { + int formatVal; + + if ((formatVal = qemuSaveFormatTypeFromString(snapstr)) < 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Invalid snapshot_image_format '%1$s'"), + snapstr); + return -1; + } + + cfg->snapshotImageFormat = formatVal; } if (virConfGetValueString(conf, "auto_dump_path", &cfg->autoDumpPath) < 0) diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h index edb65c99f4..c284e108a1 100644 --- a/src/qemu/qemu_conf.h +++ b/src/qemu/qemu_conf.h @@ -37,6 +37,7 @@ #include "locking/lock_manager.h" #include "qemu_capabilities.h" #include "qemu_nbdkit.h" +#include "qemu_saveimage_format.h" #include "virclosecallbacks.h" #include "virhostdev.h" #include "virfile.h" @@ -216,9 +217,9 @@ struct _virQEMUDriverConfig { bool securityDefaultConfined; bool securityRequireConfined; - int saveImageFormat; - int dumpImageFormat; - int snapshotImageFormat; + virQEMUSaveFormat saveImageFormat; + virQEMUSaveFormat dumpImageFormat; + virQEMUSaveFormat snapshotImageFormat; char *autoDumpPath; bool autoDumpBypassCache; diff --git a/src/qemu/qemu_saveimage.h b/src/qemu/qemu_saveimage.h index 0a22ee5f05..1fd96751a2 100644 --- a/src/qemu/qemu_saveimage.h +++ b/src/qemu/qemu_saveimage.h @@ -21,6 +21,7 @@ #include "virconftypes.h" #include "qemu_domain.h" +#include "qemu_saveimage_format.h" /* It would be nice to replace 'Qemud' with 'Qemu' but * this magic string is ABI, so it can't be changed @@ -31,24 +32,7 @@ G_STATIC_ASSERT(sizeof(QEMU_SAVE_MAGIC) == sizeof(QEMU_SAVE_PARTIAL)); -typedef enum { - QEMU_SAVE_FORMAT_RAW = 0, - QEMU_SAVE_FORMAT_GZIP = 1, - QEMU_SAVE_FORMAT_BZIP2 = 2, - /* - * Deprecated by xz and never used as part of a release - * QEMU_SAVE_FORMAT_LZMA - */ - QEMU_SAVE_FORMAT_XZ = 3, - QEMU_SAVE_FORMAT_LZOP = 4, - QEMU_SAVE_FORMAT_ZSTD = 5, - QEMU_SAVE_FORMAT_SPARSE = 6, - /* Note: add new members only at the end. - These values are used in the on-disk format. - Do not change or re-use numbers. */ - - QEMU_SAVE_FORMAT_LAST -} virQEMUSaveFormat; +/* enum virQEMUSaveFormat is declared in qemu_saveimage_format.h */ VIR_ENUM_DECL(qemuSaveFormat); typedef struct _virQEMUSaveHeader virQEMUSaveHeader; diff --git a/src/qemu/qemu_saveimage_format.h b/src/qemu/qemu_saveimage_format.h new file mode 100644 index 0000000000..53756f9fff --- /dev/null +++ b/src/qemu/qemu_saveimage_format.h @@ -0,0 +1,25 @@ +/* + * SPDX-License-Identifier: LGPL-2.1-or-later + */ + +#pragma once + +/* This enum resides in a separate file to allow inclusion into qemu_conf.h */ +typedef enum { + QEMU_SAVE_FORMAT_RAW = 0, + QEMU_SAVE_FORMAT_GZIP = 1, + QEMU_SAVE_FORMAT_BZIP2 = 2, + /* + * Deprecated by xz and never used as part of a release + * QEMU_SAVE_FORMAT_LZMA + */ + QEMU_SAVE_FORMAT_XZ = 3, + QEMU_SAVE_FORMAT_LZOP = 4, + QEMU_SAVE_FORMAT_ZSTD = 5, + QEMU_SAVE_FORMAT_SPARSE = 6, + /* Note: add new members only at the end. + These values are used in the on-disk format. + Do not change or re-use numbers. */ + + QEMU_SAVE_FORMAT_LAST +} virQEMUSaveFormat; -- 2.52.0
From: Peter Krempa <pkrempa@redhat.com> The 'VIR_DOMAIN_SAVE_PARAM_IMAGE_FORMAT' typed parameter for 'virDomainSaveParams' is implemented as a string but really encodes an enumeration of supported types. We can't change the format any more but can export the corresponding types as constants. Additionally this also mentions the missing 'sparse' format. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- include/libvirt/libvirt-domain.h | 87 ++++++++++++++++++++++++++++++-- src/qemu/qemu_saveimage.c | 14 ++--- 2 files changed, 91 insertions(+), 10 deletions(-) diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h index 16fac6b085..893359aaae 100644 --- a/include/libvirt/libvirt-domain.h +++ b/include/libvirt/libvirt-domain.h @@ -1702,14 +1702,95 @@ int virDomainRestoreParams (virConnectPtr conn, * VIR_DOMAIN_SAVE_PARAM_IMAGE_FORMAT: * * an optional parameter used to specify the format of the save image. - * Valid formats are raw, zstd, lzop, gzip, bzip2, and xz. If not - * specified, the save_image_format setting in qemu.conf is used, which - * defaults to raw. As VIR_TYPED_PARAM_STRING. + * Valid format strings are represented by constants: + * - VIR_DOMAIN_SAVE_PARAM_IMAGE_FORMAT_RAW + * - VIR_DOMAIN_SAVE_PARAM_IMAGE_FORMAT_GZIP + * - VIR_DOMAIN_SAVE_PARAM_IMAGE_FORMAT_BZIP2 + * - VIR_DOMAIN_SAVE_PARAM_IMAGE_FORMAT_XZ + * - VIR_DOMAIN_SAVE_PARAM_IMAGE_FORMAT_LZOP + * - VIR_DOMAIN_SAVE_PARAM_IMAGE_FORMAT_ZSTD + * - VIR_DOMAIN_SAVE_PARAM_IMAGE_FORMAT_SPARSE + * + * In the qemu driver, if not specified, the save_image_format setting in + * qemu.conf is used, which defaults to VIR_DOMAIN_SAVE_PARAM_IMAGE_FORMAT_RAW. + * + * As VIR_TYPED_PARAM_STRING. * * Since: 11.2.0 */ # define VIR_DOMAIN_SAVE_PARAM_IMAGE_FORMAT "image_format" + +/** + * VIR_DOMAIN_SAVE_PARAM_IMAGE_FORMAT_RAW: + * + * raw uncompressed format for the save image + * + * Since: 12.0.0 + */ +# define VIR_DOMAIN_SAVE_PARAM_IMAGE_FORMAT_RAW "raw" + + +/** + * VIR_DOMAIN_SAVE_PARAM_IMAGE_FORMAT_GZIP: + * + * gzip compressed format for the save image + * + * Since: 12.0.0 + */ +# define VIR_DOMAIN_SAVE_PARAM_IMAGE_FORMAT_GZIP "gzip" + + +/** + * VIR_DOMAIN_SAVE_PARAM_IMAGE_FORMAT_BZIP2: + * + * bzip2 compressed format for the save image + * + * Since: 12.0.0 + */ +# define VIR_DOMAIN_SAVE_PARAM_IMAGE_FORMAT_BZIP2 "bzip2" + + +/** + * VIR_DOMAIN_SAVE_PARAM_IMAGE_FORMAT_XZ: + * + * xz compressed format for the save image + * + * Since: 12.0.0 + */ +# define VIR_DOMAIN_SAVE_PARAM_IMAGE_FORMAT_XZ "xz" + + +/** + * VIR_DOMAIN_SAVE_PARAM_IMAGE_FORMAT_LZOP: + * + * lzop compressed format for the save image + * + * Since: 12.0.0 + */ +# define VIR_DOMAIN_SAVE_PARAM_IMAGE_FORMAT_LZOP "lzop" + + +/** + * VIR_DOMAIN_SAVE_PARAM_IMAGE_FORMAT_ZSTD: + * + * zstd compressed format for the save image + * + * Since: 12.0.0 + */ +# define VIR_DOMAIN_SAVE_PARAM_IMAGE_FORMAT_ZSTD "zstd" + + +/** + * VIR_DOMAIN_SAVE_PARAM_IMAGE_FORMAT_SPARSE: + * + * uncompressed sparse file format for the save image + * + * Since: 12.0.0 + */ +# define VIR_DOMAIN_SAVE_PARAM_IMAGE_FORMAT_SPARSE "sparse" + + /* * VIR_DOMAIN_SAVE_PARAM_PARALLEL_CHANNELS: * diff --git a/src/qemu/qemu_saveimage.c b/src/qemu/qemu_saveimage.c index 48f8220dee..58a3f96575 100644 --- a/src/qemu/qemu_saveimage.c +++ b/src/qemu/qemu_saveimage.c @@ -39,13 +39,13 @@ VIR_LOG_INIT("qemu.qemu_saveimage"); VIR_ENUM_IMPL(qemuSaveFormat, QEMU_SAVE_FORMAT_LAST, - "raw", - "gzip", - "bzip2", - "xz", - "lzop", - "zstd", - "sparse", + VIR_DOMAIN_SAVE_PARAM_IMAGE_FORMAT_RAW, + VIR_DOMAIN_SAVE_PARAM_IMAGE_FORMAT_GZIP, + VIR_DOMAIN_SAVE_PARAM_IMAGE_FORMAT_BZIP2, + VIR_DOMAIN_SAVE_PARAM_IMAGE_FORMAT_XZ, + VIR_DOMAIN_SAVE_PARAM_IMAGE_FORMAT_LZOP, + VIR_DOMAIN_SAVE_PARAM_IMAGE_FORMAT_ZSTD, + VIR_DOMAIN_SAVE_PARAM_IMAGE_FORMAT_SPARSE, ); static void -- 2.52.0
From: Peter Krempa <pkrempa@redhat.com> Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tools/virsh-domain.c | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 6e18d195e6..cb9dd069b6 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -4528,6 +4528,26 @@ static const vshCmdInfo info_save = { .desc = N_("Save the RAM state of a running domain."), }; + +static char ** +virshCompleteSaveImageFormat(vshControl *ctl G_GNUC_UNUSED, + const vshCmd *cmd G_GNUC_UNUSED, + unsigned int completerflags G_GNUC_UNUSED) +{ + const char *formats[] = { + VIR_DOMAIN_SAVE_PARAM_IMAGE_FORMAT_RAW, + VIR_DOMAIN_SAVE_PARAM_IMAGE_FORMAT_GZIP, + VIR_DOMAIN_SAVE_PARAM_IMAGE_FORMAT_BZIP2, + VIR_DOMAIN_SAVE_PARAM_IMAGE_FORMAT_XZ, + VIR_DOMAIN_SAVE_PARAM_IMAGE_FORMAT_LZOP, + VIR_DOMAIN_SAVE_PARAM_IMAGE_FORMAT_ZSTD, + VIR_DOMAIN_SAVE_PARAM_IMAGE_FORMAT_SPARSE, + NULL + }; + + return g_strdupv((char **) formats); +} + static const vshCmdOptDef opts_save[] = { VIRSH_COMMON_OPT_DOMAIN_FULL(VIR_CONNECT_LIST_DOMAINS_ACTIVE), {.name = "file", @@ -4546,6 +4566,7 @@ static const vshCmdOptDef opts_save[] = { }, {.name = "image-format", .type = VSH_OT_STRING, + .completer = virshCompleteSaveImageFormat, .help = N_("format of the save image file") }, {.name = "xml", -- 2.52.0
From: Peter Krempa <pkrempa@redhat.com> The meaning of 'flags' is context dependant. 'qemuMigrationSrcToFile' expects 'virDomainSaveRestoreFlags' rather than migration flags which is not expected based on the location of the function. Why this is wrong is clearly visible in 'doCoreDump' which passes in 'dump_flags' which are actually 'virDomainCoreDumpFlags' and the values are different: VIR_DUMP_BYPASS_CACHE = (1 << 2) VIR_DOMAIN_SAVE_BYPASS_CACHE = 1 << 0 Since it checks only for VIR_DOMAIN_SAVE_BYPASS_CACHE pass it in as a boolean instead. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 4 +++- src/qemu/qemu_migration.c | 8 ++++---- src/qemu/qemu_migration.h | 2 +- src/qemu/qemu_saveimage.c | 4 +++- 4 files changed, 11 insertions(+), 7 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index d88c0833ad..cca9d06786 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -3137,7 +3137,9 @@ doCoreDump(virQEMUDriver *driver, goto cleanup; if (qemuMigrationSrcToFile(driver, vm, path, &fd, compressor, - dump_params, dump_flags, VIR_ASYNC_JOB_DUMP) < 0) + dump_params, + (dump_flags & VIR_DUMP_BYPASS_CACHE), + VIR_ASYNC_JOB_DUMP) < 0) goto cleanup; } diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 4f9b649b63..1371742529 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -7210,7 +7210,7 @@ qemuMigrationSrcToSparseFile(virQEMUDriver *driver, virDomainObj *vm, const char *path, int *fd, - unsigned int flags, + bool bypassCache, virDomainAsyncJob asyncJob) { g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); @@ -7222,7 +7222,7 @@ qemuMigrationSrcToSparseFile(virQEMUDriver *driver, /* When using directio with mapped-ram, qemu needs two fds. One with * O_DIRECT set writing the memory, and another without it set for * writing small bits of unaligned state. */ - if ((flags & VIR_DOMAIN_SAVE_BYPASS_CACHE)) { + if (bypassCache) { directFlag = virFileDirectFdFlag(); if (directFlag < 0) { virReportError(VIR_ERR_OPERATION_FAILED, "%s", @@ -7259,7 +7259,7 @@ qemuMigrationSrcToFile(virQEMUDriver *driver, virDomainObj *vm, int *fd, virCommand *compressor, qemuMigrationParams *migParams, - unsigned int flags, + bool bypassCache, virDomainAsyncJob asyncJob) { qemuDomainObjPrivate *priv = vm->privateData; @@ -7293,7 +7293,7 @@ qemuMigrationSrcToFile(virQEMUDriver *driver, virDomainObj *vm, if (migParams && qemuMigrationParamsCapEnabled(migParams, QEMU_MIGRATION_CAP_MAPPED_RAM)) - rc = qemuMigrationSrcToSparseFile(driver, vm, path, fd, flags, asyncJob); + rc = qemuMigrationSrcToSparseFile(driver, vm, path, fd, bypassCache, asyncJob); else rc = qemuMigrationSrcToLegacyFile(driver, vm, *fd, compressor, asyncJob); diff --git a/src/qemu/qemu_migration.h b/src/qemu/qemu_migration.h index 50910ecb1f..51341f453d 100644 --- a/src/qemu/qemu_migration.h +++ b/src/qemu/qemu_migration.h @@ -244,7 +244,7 @@ qemuMigrationSrcToFile(virQEMUDriver *driver, int *fd, virCommand *compressor, qemuMigrationParams *migParams, - unsigned int flags, + bool bypassCache, virDomainAsyncJob asyncJob) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) G_GNUC_WARN_UNUSED_RESULT; diff --git a/src/qemu/qemu_saveimage.c b/src/qemu/qemu_saveimage.c index 58a3f96575..1b68a09595 100644 --- a/src/qemu/qemu_saveimage.c +++ b/src/qemu/qemu_saveimage.c @@ -501,7 +501,9 @@ qemuSaveImageCreate(virQEMUDriver *driver, goto cleanup; /* Perform the migration */ - if (qemuMigrationSrcToFile(driver, vm, path, &fd, compressor, saveParams, flags, asyncJob) < 0) + if (qemuMigrationSrcToFile(driver, vm, path, &fd, compressor, saveParams, + (flags & VIR_DOMAIN_SAVE_BYPASS_CACHE), + asyncJob) < 0) goto cleanup; /* Touch up file header to mark image complete. */ -- 2.52.0
From: Peter Krempa <pkrempa@redhat.com> Similarly to previous commit, 'flags' is really opaque. The function lives in migration code and similar functions there expect migration flags. Here we get virDomainSaveRestoreFlags. Here at least the dump code handles it properly and passes VIR_DOMAIN_SAVE_BYPASS_CACHE rather than VIR_DUMP_BYPASS_CACHE. Note: We, in many cases, encourage use of 'flags' instead of a bunch of boolean parameters. Since C doesn't do proper type checks on enums and in fact with 'flags' we pass a binary or of some flags rather than pure options from the enum there isn't really an elegant solution that would be enforced by the compiler and easy on eyes. With a bunch of booleans at least anyone reading the code will need to look up the function definition to see the header rather than assume that passing in 'flags' is fine without properly checking *which* flags are accepted by the function. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 7 ++++--- src/qemu/qemu_migration_params.c | 4 ++-- src/qemu/qemu_migration_params.h | 2 +- 3 files changed, 7 insertions(+), 6 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index cca9d06786..3c6dd97c04 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -2679,7 +2679,7 @@ qemuDomainSaveInternal(virQEMUDriver *driver, if (!(saveParams = qemuMigrationParamsForSave(params, nparams, format == QEMU_SAVE_FORMAT_SPARSE, - flags))) + (flags & VIR_DOMAIN_SAVE_BYPASS_CACHE)))) goto endjob; ret = qemuSaveImageCreate(driver, vm, path, data, compressor, @@ -5798,7 +5798,8 @@ qemuDomainRestoreInternal(virConnectPtr conn, goto cleanup; sparse = data->header.format == QEMU_SAVE_FORMAT_SPARSE; - if (!(restoreParams = qemuMigrationParamsForSave(params, nparams, sparse, flags))) + if (!(restoreParams = qemuMigrationParamsForSave(params, nparams, sparse, + (flags & VIR_DOMAIN_SAVE_BYPASS_CACHE)))) goto cleanup; fd = qemuSaveImageOpen(driver, path, @@ -6130,7 +6131,7 @@ qemuDomainObjRestore(virConnectPtr conn, sparse = data->header.format == QEMU_SAVE_FORMAT_SPARSE; if (!(restoreParams = qemuMigrationParamsForSave(NULL, 0, sparse, - bypass_cache ? VIR_DOMAIN_SAVE_BYPASS_CACHE : 0))) + bypass_cache))) return -1; fd = qemuSaveImageOpen(driver, path, bypass_cache, sparse, &wrapperFd, false); diff --git a/src/qemu/qemu_migration_params.c b/src/qemu/qemu_migration_params.c index b79bbad5c2..dd47516742 100644 --- a/src/qemu/qemu_migration_params.c +++ b/src/qemu/qemu_migration_params.c @@ -800,7 +800,7 @@ qemuMigrationParams * qemuMigrationParamsForSave(virTypedParameterPtr params, int nparams, bool sparse, - unsigned int flags) + bool bypassCache) { g_autoptr(qemuMigrationParams) saveParams = NULL; int nchannels = 0; @@ -837,7 +837,7 @@ qemuMigrationParamsForSave(virTypedParameterPtr params, saveParams->params[QEMU_MIGRATION_PARAM_MULTIFD_CHANNELS].value.i = nchannels; saveParams->params[QEMU_MIGRATION_PARAM_MULTIFD_CHANNELS].set = true; - if (flags & VIR_DOMAIN_SAVE_BYPASS_CACHE) { + if (bypassCache) { saveParams->params[QEMU_MIGRATION_PARAM_DIRECT_IO].value.b = true; saveParams->params[QEMU_MIGRATION_PARAM_DIRECT_IO].set = true; } diff --git a/src/qemu/qemu_migration_params.h b/src/qemu/qemu_migration_params.h index 9d771d519d..b7a829b85a 100644 --- a/src/qemu/qemu_migration_params.h +++ b/src/qemu/qemu_migration_params.h @@ -92,7 +92,7 @@ qemuMigrationParams * qemuMigrationParamsForSave(virTypedParameterPtr params, int nparams, bool sparse, - unsigned int flags); + bool bypassCache); int qemuMigrationParamsDump(qemuMigrationParams *migParams, -- 2.52.0
On 12/9/25 10:46, Peter Krempa via Devel wrote:
While looking trhough the snapshot code I've noticed a few bugs:
- memleak in cleanup code of migration to file (1/9) - wrong place where 'manual' external disk snapshot is taken (2/9) - bad interaction between 'manual' snapshot and the _QUIESCE flag (3/9) - constants for save image types not exposed (6/9) - use of wrong 'flags' in migration to file code (8/9)
The series contains also few cleanups and improvements: - bash completion for save image type (7/9) - use of proper types for save image type (4,5/9) - avoid problems with 'flags' in qemuMigrationParamsForSave (9/9)
Part 2 will focus on some code cleanups.
Peter Krempa (9): qemuMigrationSrcToFile: Don't leak 'qemuFDPass' in cleanup path qemu: snapshot: Setup disks for manual snapshot only when the VM is actually paused qemuSnapshotPrepare: Prohibit 'manual' disk snapshot mode with VIR_DOMAIN_SNAPSHOT_CREATE_QUIESCE flag qemu: Use 'virQEMUSaveFormat' type everywhere except qemu_conf qemu: conf: Use proper type for (save|dump|snapshot)ImageFormat include: Create constants for save image format values virsh: Add completer for '--image-format' option of 'save' command qemuMigrationSrcToFile: Don't cross-contaminate 'flags' variable qemuMigrationParamsForSave: Don't take opaque 'flags'
include/libvirt/libvirt-domain.h | 87 ++++++++++++++++++++++++++++++-- src/qemu/qemu_conf.c | 48 ++++++++++++------ src/qemu/qemu_conf.h | 7 +-- src/qemu/qemu_driver.c | 29 +++++++---- src/qemu/qemu_migration.c | 14 ++--- src/qemu/qemu_migration.h | 2 +- src/qemu/qemu_migration_params.c | 4 +- src/qemu/qemu_migration_params.h | 2 +- src/qemu/qemu_saveimage.c | 22 ++++---- src/qemu/qemu_saveimage.h | 24 ++------- src/qemu/qemu_saveimage_format.h | 25 +++++++++ src/qemu/qemu_snapshot.c | 14 +++-- tools/virsh-domain.c | 21 ++++++++ 13 files changed, 224 insertions(+), 75 deletions(-) create mode 100644 src/qemu/qemu_saveimage_format.h
Reviewed-by: Michal Privoznik <mprivozn@redhat.com> Michal
participants (2)
-
Michal Prívozník -
Peter Krempa