[PATCH 0/3] qemu: Small cleanups to SaveImageGetCompressionProgram and callers
Even though my work on supporting mapped-ram is the main motivation for this small cleanup series, IMO is useful in its own right. Jim Fehlig (3): qemu: Move declaration of virQEMUSaveFormat to header file qemu: Move special handling of invalid dump format to only caller qemu: Change return value of SaveImageGetCompressionProgram src/qemu/qemu_driver.c | 52 ++++++++++++++-------- src/qemu/qemu_saveimage.c | 92 +++++++-------------------------------- src/qemu/qemu_saveimage.h | 24 ++++++++-- src/qemu/qemu_snapshot.c | 11 +++-- 4 files changed, 78 insertions(+), 101 deletions(-) -- 2.43.0
Allow use of the enum outside of qemu_saveimage. Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- src/qemu/qemu_saveimage.c | 19 ------------------- src/qemu/qemu_saveimage.h | 19 +++++++++++++++++++ 2 files changed, 19 insertions(+), 19 deletions(-) diff --git a/src/qemu/qemu_saveimage.c b/src/qemu/qemu_saveimage.c index 5c889fee11..403e4c9679 100644 --- a/src/qemu/qemu_saveimage.c +++ b/src/qemu/qemu_saveimage.c @@ -37,25 +37,6 @@ VIR_LOG_INIT("qemu.qemu_saveimage"); -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, - /* 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; - -VIR_ENUM_DECL(qemuSaveFormat); VIR_ENUM_IMPL(qemuSaveFormat, QEMU_SAVE_FORMAT_LAST, "raw", diff --git a/src/qemu/qemu_saveimage.h b/src/qemu/qemu_saveimage.h index 53ae222467..8e755e1eb5 100644 --- a/src/qemu/qemu_saveimage.h +++ b/src/qemu/qemu_saveimage.h @@ -32,6 +32,25 @@ 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, + /* 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; +VIR_ENUM_DECL(qemuSaveFormat); + typedef struct _virQEMUSaveHeader virQEMUSaveHeader; struct _virQEMUSaveHeader { char magic[sizeof(QEMU_SAVE_MAGIC)-1]; -- 2.43.0
On Fri, Feb 14, 2025 at 03:48:16PM -0700, Jim Fehlig via Devel wrote:
Allow use of the enum outside of qemu_saveimage.
Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- src/qemu/qemu_saveimage.c | 19 ------------------- src/qemu/qemu_saveimage.h | 19 +++++++++++++++++++ 2 files changed, 19 insertions(+), 19 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
diff --git a/src/qemu/qemu_saveimage.h b/src/qemu/qemu_saveimage.h index 53ae222467..8e755e1eb5 100644 --- a/src/qemu/qemu_saveimage.h +++ b/src/qemu/qemu_saveimage.h @@ -32,6 +32,25 @@
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 + */
As a separate commit we can cull that comment. We don't need to record the fact we deleted that years ago before it ever it was ever visible to users.
+ QEMU_SAVE_FORMAT_XZ = 3, + QEMU_SAVE_FORMAT_LZOP = 4, + QEMU_SAVE_FORMAT_ZSTD = 5, + /* 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; +VIR_ENUM_DECL(qemuSaveFormat); +
With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
The 'use_raw_on_fail' logic in qemuSaveImageGetCompressionProgram is only used by doCoreDump in qemu_driver.c. Move the logic to the single call site and remove the parameter from qemuSaveImageGetCompressionProgram. Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- src/qemu/qemu_driver.c | 29 ++++++++++++++++++++--------- src/qemu/qemu_saveimage.c | 38 ++++++++++---------------------------- src/qemu/qemu_saveimage.h | 3 +-- src/qemu/qemu_snapshot.c | 2 +- 4 files changed, 32 insertions(+), 40 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 78bfaa5b3a..0a1bcc0ed5 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -2766,7 +2766,7 @@ qemuDomainManagedSaveHelper(virQEMUDriver *driver, cfg = virQEMUDriverGetConfig(driver); if ((format = qemuSaveImageGetCompressionProgram(cfg->saveImageFormat, &compressor, - "save", false)) < 0) + "save")) < 0) return -1; path = qemuDomainManagedSavePath(driver, vm); @@ -2800,7 +2800,7 @@ qemuDomainSaveFlags(virDomainPtr dom, const char *path, const char *dxml, cfg = virQEMUDriverGetConfig(driver); if ((format = qemuSaveImageGetCompressionProgram(cfg->saveImageFormat, &compressor, - "save", false)) < 0) + "save")) < 0) goto cleanup; if (!(vm = qemuDomainObjFromDomain(dom))) @@ -2874,7 +2874,7 @@ qemuDomainSaveParams(virDomainPtr dom, cfg = virQEMUDriverGetConfig(driver); if ((format = qemuSaveImageGetCompressionProgram(cfg->saveImageFormat, &compressor, - "save", false)) < 0) + "save")) < 0) goto cleanup; if (virDomainObjCheckActive(vm) < 0) @@ -3077,6 +3077,7 @@ doCoreDump(virQEMUDriver *driver, { int fd = -1; int ret = -1; + int format = QEMU_SAVE_FORMAT_RAW; virFileWrapperFd *wrapperFd = NULL; int directFlag = 0; bool needUnlink = false; @@ -3085,13 +3086,23 @@ doCoreDump(virQEMUDriver *driver, g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); g_autoptr(virCommand) compressor = NULL; + if (cfg->dumpImageFormat) { + if ((format = qemuSaveFormatTypeFromString(cfg->dumpImageFormat)) < 0) { + VIR_WARN("Invalid dump image format specified in configuration file, using raw"); + format = QEMU_SAVE_FORMAT_RAW; + } + } + /* We reuse "save" flag for "dump" here. Then, we can support the same - * format in "save" and "dump". This path doesn't need the compression - * program to exist and can ignore the return value - it only cares to - * get the compressor */ - ignore_value(qemuSaveImageGetCompressionProgram(cfg->dumpImageFormat, - &compressor, - "dump", true)); + * format in "save" and "dump". If the compression program doesn't exist, + * reset any errors and continue on using the raw format. + */ + if (qemuSaveImageGetCompressionProgram(cfg->dumpImageFormat, + &compressor, "dump") < 0) { + virResetLastError(); + VIR_WARN("Compression program for dump image format in " + "configuration file isn't available, using raw"); + } /* Create an empty file with appropriate ownership. */ if (dump_flags & VIR_DUMP_BYPASS_CACHE) { diff --git a/src/qemu/qemu_saveimage.c b/src/qemu/qemu_saveimage.c index 403e4c9679..eea35df175 100644 --- a/src/qemu/qemu_saveimage.c +++ b/src/qemu/qemu_saveimage.c @@ -513,10 +513,6 @@ qemuSaveImageCreate(virQEMUDriver *driver, * @compresspath: Pointer to a character string to store the fully qualified * path from virFindFileInPath. * @styleFormat: String representing the style of format (dump, save, snapshot) - * @use_raw_on_fail: Boolean indicating how to handle the error path. For - * callers that are OK with invalid data or inability to - * find the compression program, just return a raw format - * and let the path remain as NULL. * * Returns: * virQEMUSaveFormat - Integer representation of the save image @@ -529,8 +525,7 @@ qemuSaveImageCreate(virQEMUDriver *driver, int qemuSaveImageGetCompressionProgram(const char *imageFormat, virCommand **compressor, - const char *styleFormat, - bool use_raw_on_fail) + const char *styleFormat) { int ret; const char *prog; @@ -549,6 +544,7 @@ qemuSaveImageGetCompressionProgram(const char *imageFormat, if (!(prog = virFindFileInPath(imageFormat))) goto error; + *compressor = virCommandNew(prog); virCommandAddArg(*compressor, "-c"); if (ret == QEMU_SAVE_FORMAT_XZ) @@ -558,31 +554,17 @@ qemuSaveImageGetCompressionProgram(const char *imageFormat, error: if (ret < 0) { - if (use_raw_on_fail) - VIR_WARN("Invalid %s image format specified in " - "configuration file, using raw", - styleFormat); - else - virReportError(VIR_ERR_OPERATION_FAILED, - _("Invalid %1$s image format specified in configuration file"), - styleFormat); + ret = QEMU_SAVE_FORMAT_RAW; + virReportError(VIR_ERR_OPERATION_FAILED, + _("Invalid %1$s image format specified in configuration file"), + styleFormat); } else { - if (use_raw_on_fail) - VIR_WARN("Compression program for %s image format in " - "configuration file isn't available, using raw", - styleFormat); - else - virReportError(VIR_ERR_OPERATION_FAILED, - _("Compression program for %1$s image format in configuration file isn't available"), - styleFormat); + virReportError(VIR_ERR_OPERATION_FAILED, + _("Compression program for %1$s image format in configuration file isn't available"), + styleFormat); } - /* Use "raw" as the format if the specified format is not valid, - * or the compress program is not available. */ - if (use_raw_on_fail) - return QEMU_SAVE_FORMAT_RAW; - - return -1; + return ret; } diff --git a/src/qemu/qemu_saveimage.h b/src/qemu/qemu_saveimage.h index 8e755e1eb5..aa905768de 100644 --- a/src/qemu/qemu_saveimage.h +++ b/src/qemu/qemu_saveimage.h @@ -112,8 +112,7 @@ qemuSaveImageOpen(virQEMUDriver *driver, int qemuSaveImageGetCompressionProgram(const char *imageFormat, virCommand **compressor, - const char *styleFormat, - bool use_raw_on_fail) + const char *styleFormat) ATTRIBUTE_NONNULL(2); int diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index ed140dd41c..4ff7e09bd4 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -1658,7 +1658,7 @@ qemuSnapshotCreateActiveExternal(virQEMUDriver *driver, if ((format = qemuSaveImageGetCompressionProgram(cfg->snapshotImageFormat, &compressor, - "snapshot", false)) < 0) + "snapshot")) < 0) goto cleanup; if (!(xml = qemuDomainDefFormatLive(driver, priv->qemuCaps, -- 2.43.0
On Fri, Feb 14, 2025 at 03:48:17PM -0700, Jim Fehlig via Devel wrote:
The 'use_raw_on_fail' logic in qemuSaveImageGetCompressionProgram is only used by doCoreDump in qemu_driver.c. Move the logic to the single call site and remove the parameter from qemuSaveImageGetCompressionProgram.
Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- src/qemu/qemu_driver.c | 29 ++++++++++++++++++++--------- src/qemu/qemu_saveimage.c | 38 ++++++++++---------------------------- src/qemu/qemu_saveimage.h | 3 +-- src/qemu/qemu_snapshot.c | 2 +- 4 files changed, 32 insertions(+), 40 deletions(-)
@@ -3085,13 +3086,23 @@ doCoreDump(virQEMUDriver *driver, g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); g_autoptr(virCommand) compressor = NULL;
+ if (cfg->dumpImageFormat) { + if ((format = qemuSaveFormatTypeFromString(cfg->dumpImageFormat)) < 0) { + VIR_WARN("Invalid dump image format specified in configuration file, using raw"); + format = QEMU_SAVE_FORMAT_RAW; + } + } +
IMHO this is not something we should do - if the user typo'd in their configuration file that should always be an hard error, so they see their mistake immediately.
/* We reuse "save" flag for "dump" here. Then, we can support the same - * format in "save" and "dump". This path doesn't need the compression - * program to exist and can ignore the return value - it only cares to - * get the compressor */ - ignore_value(qemuSaveImageGetCompressionProgram(cfg->dumpImageFormat, - &compressor, - "dump", true)); + * format in "save" and "dump". If the compression program doesn't exist, + * reset any errors and continue on using the raw format. + */ + if (qemuSaveImageGetCompressionProgram(cfg->dumpImageFormat, + &compressor, "dump") < 0) { + virResetLastError(); + VIR_WARN("Compression program for dump image format in " + "configuration file isn't available, using raw"); + }
I'm trying to understand why we should special case 'dump' in this way. This special case goes back to: commit 48cb9f0542d70f9e3ac91b9b7d82fc9b85807b4e Author: John Ferlan <jferlan@redhat.com> Date: Tue Sep 13 10:11:00 2016 -0400 qemu: Use qemuGetCompressionProgram for error paths Let's do some more code reuse - there are 3 other callers that care to check/get the compress program. Each of those though cares whether the requested cfg image is valid and exists. So, add a parameter to handle those cases. NB: We won't need to initialize the returned value in the case where the cfg image doesn't exist since the called program will handle that. That commit message doesn't justify why dump needs special handling. We can see though the original code it was "unifying", lacked any error handling for the dump case, and this refactoring preserved that. I'm rather inclined to say that was a mistake. Unless someone can just an attractive justification, I think dump should be doing the same error checking as the other cases. ie if the user asked for gzip, they should get gzip, or an error if that's impossible, rather than ignoring their requested config. With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
qemuSaveImageGetCompressionProgram is a bit overloaded. Along with getting a compression program, it checks the validity of the image format and returns the integer representation of the format. Change the function to only handle retrieving the specified compression program, returning success or failure. Checking the validity of the image format can be left to the calling functions. Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- src/qemu/qemu_driver.c | 33 +++++++++++++---------- src/qemu/qemu_saveimage.c | 55 ++++++++++++--------------------------- src/qemu/qemu_saveimage.h | 2 +- src/qemu/qemu_snapshot.c | 11 +++++--- 4 files changed, 43 insertions(+), 58 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 0a1bcc0ed5..577e799c2b 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -2752,7 +2752,7 @@ qemuDomainManagedSaveHelper(virQEMUDriver *driver, g_autoptr(virQEMUDriverConfig) cfg = NULL; g_autoptr(virCommand) compressor = NULL; g_autofree char *path = NULL; - int format; + int format = QEMU_SAVE_FORMAT_RAW; if (virDomainObjCheckActive(vm) < 0) return -1; @@ -2764,9 +2764,11 @@ qemuDomainManagedSaveHelper(virQEMUDriver *driver, } cfg = virQEMUDriverGetConfig(driver); - if ((format = qemuSaveImageGetCompressionProgram(cfg->saveImageFormat, - &compressor, - "save")) < 0) + if (cfg->saveImageFormat && + (format = qemuSaveFormatTypeFromString(cfg->saveImageFormat)) < 0) + return -1; + + if (qemuSaveImageGetCompressionProgram(format, &compressor, "save") < 0) return -1; path = qemuDomainManagedSavePath(driver, vm); @@ -2787,7 +2789,7 @@ qemuDomainSaveFlags(virDomainPtr dom, const char *path, const char *dxml, unsigned int flags) { virQEMUDriver *driver = dom->conn->privateData; - int format; + int format = QEMU_SAVE_FORMAT_RAW; g_autoptr(virCommand) compressor = NULL; int ret = -1; virDomainObj *vm = NULL; @@ -2798,9 +2800,11 @@ qemuDomainSaveFlags(virDomainPtr dom, const char *path, const char *dxml, VIR_DOMAIN_SAVE_PAUSED, -1); cfg = virQEMUDriverGetConfig(driver); - if ((format = qemuSaveImageGetCompressionProgram(cfg->saveImageFormat, - &compressor, - "save")) < 0) + if (cfg->saveImageFormat && + (format = qemuSaveFormatTypeFromString(cfg->saveImageFormat)) < 0) + goto cleanup; + + if (qemuSaveImageGetCompressionProgram(format, &compressor, "save") < 0) goto cleanup; if (!(vm = qemuDomainObjFromDomain(dom))) @@ -2838,7 +2842,7 @@ qemuDomainSaveParams(virDomainPtr dom, g_autoptr(virCommand) compressor = NULL; const char *to = NULL; const char *dxml = NULL; - int format; + int format = QEMU_SAVE_FORMAT_RAW; int ret = -1; virCheckFlags(VIR_DOMAIN_SAVE_BYPASS_CACHE | @@ -2872,9 +2876,11 @@ qemuDomainSaveParams(virDomainPtr dom, } cfg = virQEMUDriverGetConfig(driver); - if ((format = qemuSaveImageGetCompressionProgram(cfg->saveImageFormat, - &compressor, - "save")) < 0) + if (cfg->saveImageFormat && + (format = qemuSaveFormatTypeFromString(cfg->saveImageFormat)) < 0) + goto cleanup; + + if (qemuSaveImageGetCompressionProgram(format, &compressor, "save") < 0) goto cleanup; if (virDomainObjCheckActive(vm) < 0) @@ -3097,8 +3103,7 @@ doCoreDump(virQEMUDriver *driver, * format in "save" and "dump". If the compression program doesn't exist, * reset any errors and continue on using the raw format. */ - if (qemuSaveImageGetCompressionProgram(cfg->dumpImageFormat, - &compressor, "dump") < 0) { + if (qemuSaveImageGetCompressionProgram(format, &compressor, "dump") < 0) { virResetLastError(); VIR_WARN("Compression program for dump image format in " "configuration file isn't available, using raw"); diff --git a/src/qemu/qemu_saveimage.c b/src/qemu/qemu_saveimage.c index eea35df175..d940bfb5c3 100644 --- a/src/qemu/qemu_saveimage.c +++ b/src/qemu/qemu_saveimage.c @@ -508,63 +508,40 @@ qemuSaveImageCreate(virQEMUDriver *driver, /* qemuSaveImageGetCompressionProgram: - * @imageFormat: String representation from qemu.conf of the image format - * being used (dump, save, or snapshot). + * @format: Integer representation of the image format being used + * (dump, save, or snapshot). * @compresspath: Pointer to a character string to store the fully qualified * path from virFindFileInPath. * @styleFormat: String representing the style of format (dump, save, snapshot) * - * Returns: - * virQEMUSaveFormat - Integer representation of the save image - * format to be used for particular style - * (e.g. dump, save, or snapshot). - * QEMU_SAVE_FORMAT_RAW - If there is no qemu.conf imageFormat value or - * no there was an error, then just return RAW - * indicating none. + * Returns -1 on failure, 0 on success. */ int -qemuSaveImageGetCompressionProgram(const char *imageFormat, +qemuSaveImageGetCompressionProgram(int format, virCommand **compressor, const char *styleFormat) { - int ret; + const char *imageFormat = qemuSaveFormatTypeToString(format); const char *prog; *compressor = NULL; - if (!imageFormat) - return QEMU_SAVE_FORMAT_RAW; - - if ((ret = qemuSaveFormatTypeFromString(imageFormat)) < 0) - goto error; - - if (ret == QEMU_SAVE_FORMAT_RAW) - return QEMU_SAVE_FORMAT_RAW; - - if (!(prog = virFindFileInPath(imageFormat))) - goto error; - + if (format == QEMU_SAVE_FORMAT_RAW) + return 0; + + if (!(prog = virFindFileInPath(imageFormat))) { + virReportError(VIR_ERR_OPERATION_FAILED, + _("Compression program for %1$s image format in configuration file isn't available"), + styleFormat); + return -1; + } *compressor = virCommandNew(prog); virCommandAddArg(*compressor, "-c"); - if (ret == QEMU_SAVE_FORMAT_XZ) + if (format == QEMU_SAVE_FORMAT_XZ) virCommandAddArg(*compressor, "-3"); - return ret; - - error: - if (ret < 0) { - ret = QEMU_SAVE_FORMAT_RAW; - virReportError(VIR_ERR_OPERATION_FAILED, - _("Invalid %1$s image format specified in configuration file"), - styleFormat); - } else { - virReportError(VIR_ERR_OPERATION_FAILED, - _("Compression program for %1$s image format in configuration file isn't available"), - styleFormat); - } - - return ret; + return 0; } diff --git a/src/qemu/qemu_saveimage.h b/src/qemu/qemu_saveimage.h index aa905768de..0d8ee542af 100644 --- a/src/qemu/qemu_saveimage.h +++ b/src/qemu/qemu_saveimage.h @@ -110,7 +110,7 @@ qemuSaveImageOpen(virQEMUDriver *driver, ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(4); int -qemuSaveImageGetCompressionProgram(const char *imageFormat, +qemuSaveImageGetCompressionProgram(int format, virCommand **compressor, const char *styleFormat) ATTRIBUTE_NONNULL(2); diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index 4ff7e09bd4..c672d4f24f 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -1579,7 +1579,7 @@ qemuSnapshotCreateActiveExternal(virQEMUDriver *driver, bool memory_existing = false; bool thaw = false; bool pmsuspended = false; - int format; + int format = QEMU_SAVE_FORMAT_RAW; g_autoptr(virCommand) compressor = NULL; virQEMUSaveData *data = NULL; g_autoptr(GHashTable) blockNamedNodeData = NULL; @@ -1656,9 +1656,12 @@ qemuSnapshotCreateActiveExternal(virQEMUDriver *driver, JOB_MASK(VIR_JOB_SUSPEND) | JOB_MASK(VIR_JOB_MIGRATION_OP))); - if ((format = qemuSaveImageGetCompressionProgram(cfg->snapshotImageFormat, - &compressor, - "snapshot")) < 0) + if (cfg->snapshotImageFormat && + (format = qemuSaveFormatTypeFromString(cfg->snapshotImageFormat)) < 0) + goto cleanup; + + if (qemuSaveImageGetCompressionProgram(format, &compressor, + "snapshot") < 0) goto cleanup; if (!(xml = qemuDomainDefFormatLive(driver, priv->qemuCaps, -- 2.43.0
On Fri, Feb 14, 2025 at 03:48:18PM -0700, Jim Fehlig via Devel wrote:
qemuSaveImageGetCompressionProgram is a bit overloaded. Along with getting a compression program, it checks the validity of the image format and returns the integer representation of the format. Change the function to only handle retrieving the specified compression program, returning success or failure. Checking the validity of the image format can be left to the calling functions.
Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- src/qemu/qemu_driver.c | 33 +++++++++++++---------- src/qemu/qemu_saveimage.c | 55 ++++++++++++--------------------------- src/qemu/qemu_saveimage.h | 2 +- src/qemu/qemu_snapshot.c | 11 +++++--- 4 files changed, 43 insertions(+), 58 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 0a1bcc0ed5..577e799c2b 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -2752,7 +2752,7 @@ qemuDomainManagedSaveHelper(virQEMUDriver *driver, g_autoptr(virQEMUDriverConfig) cfg = NULL; g_autoptr(virCommand) compressor = NULL; g_autofree char *path = NULL; - int format; + int format = QEMU_SAVE_FORMAT_RAW;
if (virDomainObjCheckActive(vm) < 0) return -1; @@ -2764,9 +2764,11 @@ qemuDomainManagedSaveHelper(virQEMUDriver *driver, }
cfg = virQEMUDriverGetConfig(driver); - if ((format = qemuSaveImageGetCompressionProgram(cfg->saveImageFormat, - &compressor, - "save")) < 0) + if (cfg->saveImageFormat && + (format = qemuSaveFormatTypeFromString(cfg->saveImageFormat)) < 0) + return -1;
As a further patch, I'd suggest we should move the qemuSaveFormatTypeFromString calls into qemu_conf.c, virQEMUDriverConfigLoadSaveEntry, so that we diagnose incorrect format strings immediately at startup, rather than delayed to time of first use.
+ if (qemuSaveImageGetCompressionProgram(format, &compressor, "save") < 0) return -1;
Leaving this here would be graceful allowing the user to install the binaries after libvirtd is running, avoiding wierd errors you would get about a binary not existing if we probed the binary at startup.
path = qemuDomainManagedSavePath(driver, vm); @@ -2787,7 +2789,7 @@ qemuDomainSaveFlags(virDomainPtr dom, const char *path, const char *dxml, unsigned int flags) { virQEMUDriver *driver = dom->conn->privateData; - int format; + int format = QEMU_SAVE_FORMAT_RAW; g_autoptr(virCommand) compressor = NULL; int ret = -1; virDomainObj *vm = NULL; @@ -2798,9 +2800,11 @@ qemuDomainSaveFlags(virDomainPtr dom, const char *path, const char *dxml, VIR_DOMAIN_SAVE_PAUSED, -1);
cfg = virQEMUDriverGetConfig(driver); - if ((format = qemuSaveImageGetCompressionProgram(cfg->saveImageFormat, - &compressor, - "save")) < 0) + if (cfg->saveImageFormat && + (format = qemuSaveFormatTypeFromString(cfg->saveImageFormat)) < 0) + goto cleanup; + + if (qemuSaveImageGetCompressionProgram(format, &compressor, "save") < 0) goto cleanup;
if (!(vm = qemuDomainObjFromDomain(dom))) @@ -2838,7 +2842,7 @@ qemuDomainSaveParams(virDomainPtr dom, g_autoptr(virCommand) compressor = NULL; const char *to = NULL; const char *dxml = NULL; - int format; + int format = QEMU_SAVE_FORMAT_RAW; int ret = -1;
virCheckFlags(VIR_DOMAIN_SAVE_BYPASS_CACHE | @@ -2872,9 +2876,11 @@ qemuDomainSaveParams(virDomainPtr dom, }
cfg = virQEMUDriverGetConfig(driver); - if ((format = qemuSaveImageGetCompressionProgram(cfg->saveImageFormat, - &compressor, - "save")) < 0) + if (cfg->saveImageFormat && + (format = qemuSaveFormatTypeFromString(cfg->saveImageFormat)) < 0) + goto cleanup; + + if (qemuSaveImageGetCompressionProgram(format, &compressor, "save") < 0) goto cleanup;
if (virDomainObjCheckActive(vm) < 0) @@ -3097,8 +3103,7 @@ doCoreDump(virQEMUDriver *driver, * format in "save" and "dump". If the compression program doesn't exist, * reset any errors and continue on using the raw format. */ - if (qemuSaveImageGetCompressionProgram(cfg->dumpImageFormat, - &compressor, "dump") < 0) { + if (qemuSaveImageGetCompressionProgram(format, &compressor, "dump") < 0) { virResetLastError(); VIR_WARN("Compression program for dump image format in " "configuration file isn't available, using raw"); diff --git a/src/qemu/qemu_saveimage.c b/src/qemu/qemu_saveimage.c index eea35df175..d940bfb5c3 100644 --- a/src/qemu/qemu_saveimage.c +++ b/src/qemu/qemu_saveimage.c @@ -508,63 +508,40 @@ qemuSaveImageCreate(virQEMUDriver *driver,
/* qemuSaveImageGetCompressionProgram: - * @imageFormat: String representation from qemu.conf of the image format - * being used (dump, save, or snapshot). + * @format: Integer representation of the image format being used + * (dump, save, or snapshot). * @compresspath: Pointer to a character string to store the fully qualified * path from virFindFileInPath. * @styleFormat: String representing the style of format (dump, save, snapshot) * - * Returns: - * virQEMUSaveFormat - Integer representation of the save image - * format to be used for particular style - * (e.g. dump, save, or snapshot). - * QEMU_SAVE_FORMAT_RAW - If there is no qemu.conf imageFormat value or - * no there was an error, then just return RAW - * indicating none. + * Returns -1 on failure, 0 on success. */ int -qemuSaveImageGetCompressionProgram(const char *imageFormat, +qemuSaveImageGetCompressionProgram(int format, virCommand **compressor, const char *styleFormat) { - int ret; + const char *imageFormat = qemuSaveFormatTypeToString(format); const char *prog;
*compressor = NULL;
- if (!imageFormat) - return QEMU_SAVE_FORMAT_RAW; - - if ((ret = qemuSaveFormatTypeFromString(imageFormat)) < 0) - goto error; - - if (ret == QEMU_SAVE_FORMAT_RAW) - return QEMU_SAVE_FORMAT_RAW; - - if (!(prog = virFindFileInPath(imageFormat))) - goto error; - + if (format == QEMU_SAVE_FORMAT_RAW) + return 0; + + if (!(prog = virFindFileInPath(imageFormat))) { + virReportError(VIR_ERR_OPERATION_FAILED, + _("Compression program for %1$s image format in configuration file isn't available"), + styleFormat); + return -1; + }
*compressor = virCommandNew(prog); virCommandAddArg(*compressor, "-c"); - if (ret == QEMU_SAVE_FORMAT_XZ) + if (format == QEMU_SAVE_FORMAT_XZ) virCommandAddArg(*compressor, "-3");
- return ret; - - error: - if (ret < 0) { - ret = QEMU_SAVE_FORMAT_RAW; - virReportError(VIR_ERR_OPERATION_FAILED, - _("Invalid %1$s image format specified in configuration file"), - styleFormat); - } else { - virReportError(VIR_ERR_OPERATION_FAILED, - _("Compression program for %1$s image format in configuration file isn't available"), - styleFormat); - } - - return ret; + return 0; }
diff --git a/src/qemu/qemu_saveimage.h b/src/qemu/qemu_saveimage.h index aa905768de..0d8ee542af 100644 --- a/src/qemu/qemu_saveimage.h +++ b/src/qemu/qemu_saveimage.h @@ -110,7 +110,7 @@ qemuSaveImageOpen(virQEMUDriver *driver, ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(4);
int -qemuSaveImageGetCompressionProgram(const char *imageFormat, +qemuSaveImageGetCompressionProgram(int format, virCommand **compressor, const char *styleFormat) ATTRIBUTE_NONNULL(2); diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index 4ff7e09bd4..c672d4f24f 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -1579,7 +1579,7 @@ qemuSnapshotCreateActiveExternal(virQEMUDriver *driver, bool memory_existing = false; bool thaw = false; bool pmsuspended = false; - int format; + int format = QEMU_SAVE_FORMAT_RAW; g_autoptr(virCommand) compressor = NULL; virQEMUSaveData *data = NULL; g_autoptr(GHashTable) blockNamedNodeData = NULL; @@ -1656,9 +1656,12 @@ qemuSnapshotCreateActiveExternal(virQEMUDriver *driver, JOB_MASK(VIR_JOB_SUSPEND) | JOB_MASK(VIR_JOB_MIGRATION_OP)));
- if ((format = qemuSaveImageGetCompressionProgram(cfg->snapshotImageFormat, - &compressor, - "snapshot")) < 0) + if (cfg->snapshotImageFormat && + (format = qemuSaveFormatTypeFromString(cfg->snapshotImageFormat)) < 0) + goto cleanup; + + if (qemuSaveImageGetCompressionProgram(format, &compressor, + "snapshot") < 0) goto cleanup;
if (!(xml = qemuDomainDefFormatLive(driver, priv->qemuCaps, -- 2.43.0
With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On 2/14/25 15:48, Jim Fehlig wrote:
Even though my work on supporting mapped-ram is the main motivation for this small cleanup series, IMO is useful in its own right.
Here's an example of that usefulness https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/thread/ISSWU... Regards, Jim
Jim Fehlig (3): qemu: Move declaration of virQEMUSaveFormat to header file qemu: Move special handling of invalid dump format to only caller qemu: Change return value of SaveImageGetCompressionProgram
src/qemu/qemu_driver.c | 52 ++++++++++++++-------- src/qemu/qemu_saveimage.c | 92 +++++++-------------------------------- src/qemu/qemu_saveimage.h | 24 ++++++++-- src/qemu/qemu_snapshot.c | 11 +++-- 4 files changed, 78 insertions(+), 101 deletions(-)
participants (2)
-
Daniel P. Berrangé -
Jim Fehlig