4:48 p.m.
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
4:48 p.m.
New subject: [PATCH 1/3] qemu: Move declaration of virQEMUSaveFormat to header file
Allow use of the enum outside of qemu_saveimage.
Signed-off-by: Jim Fehlig <jfehlig(a)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
4:52 a.m.
New subject: [PATCH 1/3] qemu: Move declaration of virQEMUSaveFormat to
header file
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(a)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(a)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 :|
4:48 p.m.
New subject: [PATCH 2/3] qemu: Move special handling of invalid dump format to only caller
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(a)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
4:51 a.m.
New subject: [PATCH 2/3] qemu: Move special handling of invalid dump format
to only caller
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(a)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(a)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 :|
4:48 p.m.
New subject: [PATCH 3/3] qemu: Change return value of SaveImageGetCompressionProgram
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(a)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
4:57 a.m.
New subject: [PATCH 3/3] qemu: Change return value of
SaveImageGetCompressionProgram
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(a)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(a)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 :|
9:16 p.m.
New subject: [PATCH 0/3] qemu: Small cleanups to
SaveImageGetCompressionProgram and callers
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/IS...
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(-)
24
days inactive
27
days old
7 comments
2 participants
participants (2)
-
Daniel P. Berrangé -
Jim Fehlig