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.0On 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 :|
© 2016 - 2025 Red Hat, Inc.