6:11 p.m.
V2 of
https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/message/S...
Changes in V2:
* V1 moved the special-case handling of invalid dump image format to
doCoreDump. V2 removes it, and fails on invalid configuration similar
to the other save-related operations.
* New patch4 that moves checking for a valid save image format from
time of use to config file parsing at driver startup
Jim Fehlig (4):
qemu: Move declaration of virQEMUSaveFormat to header file
qemu: Don't ignore dump image format errors
qemu: Change return value of SaveImageGetCompressionProgram
qemu: Check for valid save image formats when loading driver config
src/qemu/qemu_conf.c | 35 ++++++++++++---
src/qemu/qemu_conf.h | 6 +--
src/qemu/qemu_driver.c | 30 ++++---------
src/qemu/qemu_saveimage.c | 92 +++++++--------------------------------
src/qemu/qemu_saveimage.h | 25 +++++++++--
src/qemu/qemu_snapshot.c | 8 ++--
6 files changed, 80 insertions(+), 116 deletions(-)
--
2.43.0
6:11 p.m.
New subject: [PATCH V2 1/4] 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>
Reviewed-by: Daniel P. Berrangé <berrange(a)redhat.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
6:11 p.m.
New subject: [PATCH V2 2/4] qemu: Don't ignore dump image format errors
Long ago, without justification, commit 48cb9f0542 changed
qemuGetCompressionProgram (since renamed to
qemuSaveImageGetCompressionProgram) to ignore configuration errors
for dump operations. Like the other save-related operations, user
provided configuration should be verified and an error reported if
it cannot be honored.
Remove the special handling of configuration errors in
qemuSaveImageGetCompressionProgram and change the dump logic to
fail when dump image format cannot be supported.
Signed-off-by: Jim Fehlig <jfehlig(a)suse.com>
---
src/qemu/qemu_driver.c | 19 ++++--------
src/qemu/qemu_saveimage.c | 61 ++++++++++-----------------------------
src/qemu/qemu_saveimage.h | 3 +-
src/qemu/qemu_snapshot.c | 2 +-
4 files changed, 24 insertions(+), 61 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 80c918312b..ac7c67b3a1 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -2744,8 +2744,7 @@ qemuDomainManagedSaveHelper(virQEMUDriver *driver,
cfg = virQEMUDriverGetConfig(driver);
if ((format = qemuSaveImageGetCompressionProgram(cfg->saveImageFormat,
- &compressor,
- "save", false)) < 0)
+ &compressor, "save"))
< 0)
return -1;
path = qemuDomainManagedSavePath(driver, vm);
@@ -2778,8 +2777,7 @@ qemuDomainSaveFlags(virDomainPtr dom, const char *path, const char
*dxml,
cfg = virQEMUDriverGetConfig(driver);
if ((format = qemuSaveImageGetCompressionProgram(cfg->saveImageFormat,
- &compressor,
- "save", false)) < 0)
+ &compressor, "save"))
< 0)
goto cleanup;
if (!(vm = qemuDomainObjFromDomain(dom)))
@@ -2852,8 +2850,7 @@ qemuDomainSaveParams(virDomainPtr dom,
cfg = virQEMUDriverGetConfig(driver);
if ((format = qemuSaveImageGetCompressionProgram(cfg->saveImageFormat,
- &compressor,
- "save", false)) < 0)
+ &compressor, "save"))
< 0)
goto cleanup;
if (virDomainObjCheckActive(vm) < 0)
@@ -3064,13 +3061,9 @@ doCoreDump(virQEMUDriver *driver,
g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
g_autoptr(virCommand) compressor = NULL;
- /* 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));
+ if (qemuSaveImageGetCompressionProgram(cfg->dumpImageFormat,
+ &compressor, "dump") < 0)
+ goto cleanup;
/* 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..9bb76c05f8 100644
--- a/src/qemu/qemu_saveimage.c
+++ b/src/qemu/qemu_saveimage.c
@@ -513,24 +513,15 @@ 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
- * 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.
+ * On success, returns an integer representation of the save image format to be
+ * used for a particular style (e.g. dump, save, or snapshot). Returns -1 on
+ * failure.
*/
int
qemuSaveImageGetCompressionProgram(const char *imageFormat,
virCommand **compressor,
- const char *styleFormat,
- bool use_raw_on_fail)
+ const char *styleFormat)
{
int ret;
const char *prog;
@@ -540,14 +531,22 @@ qemuSaveImageGetCompressionProgram(const char *imageFormat,
if (!imageFormat)
return QEMU_SAVE_FORMAT_RAW;
- if ((ret = qemuSaveFormatTypeFromString(imageFormat)) < 0)
- goto error;
+ if ((ret = qemuSaveFormatTypeFromString(imageFormat)) < 0) {
+ virReportError(VIR_ERR_OPERATION_FAILED,
+ _("Invalid %1$s image format specified in configuration
file"),
+ styleFormat);
+ return -1;
+ }
if (ret == QEMU_SAVE_FORMAT_RAW)
return QEMU_SAVE_FORMAT_RAW;
- if (!(prog = virFindFileInPath(imageFormat)))
- goto error;
+ 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");
@@ -555,34 +554,6 @@ qemuSaveImageGetCompressionProgram(const char *imageFormat,
virCommandAddArg(*compressor, "-3");
return ret;
-
- 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);
- } 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);
- }
-
- /* 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;
}
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 f7d6272907..c2a98c1296 100644
--- a/src/qemu/qemu_snapshot.c
+++ b/src/qemu/qemu_snapshot.c
@@ -1676,7 +1676,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
8:37 a.m.
New subject: [PATCH V2 2/4] qemu: Don't ignore dump image format errors
On Thu, Feb 20, 2025 at 05:11:36PM -0700, Jim Fehlig via Devel wrote:
Long ago, without justification, commit 48cb9f0542 changed
qemuGetCompressionProgram (since renamed to
qemuSaveImageGetCompressionProgram) to ignore configuration errors
for dump operations. Like the other save-related operations, user
provided configuration should be verified and an error reported if
it cannot be honored.
Remove the special handling of configuration errors in
qemuSaveImageGetCompressionProgram and change the dump logic to
fail when dump image format cannot be supported.
Signed-off-by: Jim Fehlig <jfehlig(a)suse.com>
---
src/qemu/qemu_driver.c | 19 ++++--------
src/qemu/qemu_saveimage.c | 61 ++++++++++-----------------------------
src/qemu/qemu_saveimage.h | 3 +-
src/qemu/qemu_snapshot.c | 2 +-
4 files changed, 24 insertions(+), 61 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange(a)redhat.com>
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 :|
6:11 p.m.
New subject: [PATCH V2 3/4] 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>
Reviewed-by: Daniel P. Berrangé <berrange(a)redhat.com>
---
src/qemu/qemu_driver.c | 36 +++++++++++++++++++++++++-----------
src/qemu/qemu_saveimage.c | 30 +++++++++---------------------
src/qemu/qemu_saveimage.h | 2 +-
src/qemu/qemu_snapshot.c | 11 +++++++----
4 files changed, 42 insertions(+), 37 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index ac7c67b3a1..4d94985635 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -2731,7 +2731,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;
@@ -2743,8 +2743,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);
@@ -2765,7 +2768,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;
@@ -2776,8 +2779,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)))
@@ -2815,7 +2821,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 |
@@ -2849,8 +2855,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)
@@ -3060,9 +3069,14 @@ doCoreDump(virQEMUDriver *driver,
const char *memory_dump_format = NULL;
g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
g_autoptr(virCommand) compressor = NULL;
+ int format = QEMU_SAVE_FORMAT_RAW;
- if (qemuSaveImageGetCompressionProgram(cfg->dumpImageFormat,
- &compressor, "dump") < 0)
+ if (cfg->dumpImageFormat) {
+ if ((format = qemuSaveFormatTypeFromString(cfg->dumpImageFormat)) < 0)
+ goto cleanup;
+ }
+
+ if (qemuSaveImageGetCompressionProgram(format, &compressor, "dump")
< 0)
goto cleanup;
/* Create an empty file with appropriate ownership. */
diff --git a/src/qemu/qemu_saveimage.c b/src/qemu/qemu_saveimage.c
index 9bb76c05f8..d940bfb5c3 100644
--- a/src/qemu/qemu_saveimage.c
+++ b/src/qemu/qemu_saveimage.c
@@ -508,38 +508,26 @@ 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)
*
- * On success, returns an integer representation of the save image format to be
- * used for a particular style (e.g. dump, save, or snapshot). Returns -1 on
- * failure.
+ * 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) {
- virReportError(VIR_ERR_OPERATION_FAILED,
- _("Invalid %1$s image format specified in configuration
file"),
- styleFormat);
- return -1;
- }
-
- if (ret == QEMU_SAVE_FORMAT_RAW)
- return QEMU_SAVE_FORMAT_RAW;
+ if (format == QEMU_SAVE_FORMAT_RAW)
+ return 0;
if (!(prog = virFindFileInPath(imageFormat))) {
virReportError(VIR_ERR_OPERATION_FAILED,
@@ -550,10 +538,10 @@ qemuSaveImageGetCompressionProgram(const char *imageFormat,
*compressor = virCommandNew(prog);
virCommandAddArg(*compressor, "-c");
- if (ret == QEMU_SAVE_FORMAT_XZ)
+ if (format == QEMU_SAVE_FORMAT_XZ)
virCommandAddArg(*compressor, "-3");
- 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 c2a98c1296..416a772b92 100644
--- a/src/qemu/qemu_snapshot.c
+++ b/src/qemu/qemu_snapshot.c
@@ -1597,7 +1597,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;
@@ -1674,9 +1674,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
6:11 p.m.
New subject: [PATCH V2 4/4] qemu: Check for valid save image formats when loading driver config
Checking for valid 'foo_image_format' settings in qemu.conf is not done
until the settings are used. Move the checks to
virQEMUDriverConfigLoadSaveEntry, allowing to report incorrect format
settings at driver startup.
This change was made easier by also changing the corresponding fields
in the virQEMUDriverConfig to 'int', which is more in line with the
other fields that represent enumerated types.
Signed-off-by: Jim Fehlig <jfehlig(a)suse.com>
---
Splitting the change to virQEMUDriverConfig struct into a separate patch
felt a bit awkward compared to one patch, but I'm fine to do that if
folks prefer.
src/qemu/qemu_conf.c | 35 +++++++++++++++++++++++++++++------
src/qemu/qemu_conf.h | 6 +++---
src/qemu/qemu_driver.c | 35 +++++++----------------------------
src/qemu/qemu_saveimage.h | 1 -
src/qemu/qemu_snapshot.c | 11 +++--------
5 files changed, 42 insertions(+), 46 deletions(-)
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index b73dda7e10..14cfc289b3 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -36,6 +36,7 @@
#include "qemu_domain.h"
#include "qemu_firmware.h"
#include "qemu_namespace.h"
+#include "qemu_saveimage.h"
#include "qemu_security.h"
#include "viruuid.h"
#include "virconf.h"
@@ -374,9 +375,6 @@ static void virQEMUDriverConfigDispose(void *obj)
g_free(cfg->slirpHelperName);
g_free(cfg->dbusDaemonName);
- g_free(cfg->saveImageFormat);
- g_free(cfg->dumpImageFormat);
- g_free(cfg->snapshotImageFormat);
g_free(cfg->autoDumpPath);
g_strfreev(cfg->securityDriverNames);
@@ -626,12 +624,37 @@ static int
virQEMUDriverConfigLoadSaveEntry(virQEMUDriverConfig *cfg,
virConf *conf)
{
- if (virConfGetValueString(conf, "save_image_format",
&cfg->saveImageFormat) < 0)
+ g_autofree char *savestr = NULL;
+ g_autofree char *dumpstr = NULL;
+ g_autofree char *snapstr = NULL;
+
+ if (virConfGetValueString(conf, "save_image_format", &savestr) < 0)
return -1;
- if (virConfGetValueString(conf, "dump_image_format",
&cfg->dumpImageFormat) < 0)
+ if (savestr && (cfg->saveImageFormat =
qemuSaveFormatTypeFromString(savestr)) < 0) {
+ virReportError(VIR_ERR_OPERATION_FAILED,
+ _("Invalid save_image_format '%1$s' specified in
configuration file"),
+ savestr);
return -1;
- if (virConfGetValueString(conf, "snapshot_image_format",
&cfg->snapshotImageFormat) < 0)
+ }
+
+ if (virConfGetValueString(conf, "dump_image_format", &dumpstr) < 0)
return -1;
+ if (dumpstr && (cfg->dumpImageFormat =
qemuSaveFormatTypeFromString(dumpstr)) < 0) {
+ virReportError(VIR_ERR_OPERATION_FAILED,
+ _("Invalid dump_image_format '%1$s' specified in
configuration file"),
+ dumpstr);
+ return -1;
+ }
+
+ if (virConfGetValueString(conf, "snapshot_image_format", &snapstr) <
0)
+ return -1;
+ if (snapstr && (cfg->snapshotImageFormat =
qemuSaveFormatTypeFromString(snapstr)) < 0) {
+ virReportError(VIR_ERR_OPERATION_FAILED,
+ _("Invalid snapshot_image_format '%1$s' specified in
configuration file"),
+ snapstr);
+ return -1;
+ }
+
if (virConfGetValueString(conf, "auto_dump_path",
&cfg->autoDumpPath) < 0)
return -1;
if (virConfGetValueBool(conf, "auto_dump_bypass_cache",
&cfg->autoDumpBypassCache) < 0)
diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h
index 97214f72d0..3e1b41af73 100644
--- a/src/qemu/qemu_conf.h
+++ b/src/qemu/qemu_conf.h
@@ -193,9 +193,9 @@ struct _virQEMUDriverConfig {
bool securityDefaultConfined;
bool securityRequireConfined;
- char *saveImageFormat;
- char *dumpImageFormat;
- char *snapshotImageFormat;
+ int saveImageFormat;
+ int dumpImageFormat;
+ int snapshotImageFormat;
char *autoDumpPath;
bool autoDumpBypassCache;
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 4d94985635..76b808b98f 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -2731,7 +2731,6 @@ qemuDomainManagedSaveHelper(virQEMUDriver *driver,
g_autoptr(virQEMUDriverConfig) cfg = NULL;
g_autoptr(virCommand) compressor = NULL;
g_autofree char *path = NULL;
- int format = QEMU_SAVE_FORMAT_RAW;
if (virDomainObjCheckActive(vm) < 0)
return -1;
@@ -2743,18 +2742,14 @@ qemuDomainManagedSaveHelper(virQEMUDriver *driver,
}
cfg = virQEMUDriverGetConfig(driver);
- if (cfg->saveImageFormat &&
- (format = qemuSaveFormatTypeFromString(cfg->saveImageFormat)) < 0)
- return -1;
-
- if (qemuSaveImageGetCompressionProgram(format, &compressor, "save")
< 0)
+ if (qemuSaveImageGetCompressionProgram(cfg->saveImageFormat, &compressor,
"save") < 0)
return -1;
path = qemuDomainManagedSavePath(driver, vm);
VIR_INFO("Saving state of domain '%s' to '%s'",
vm->def->name, path);
- if (qemuDomainSaveInternal(driver, vm, path, format,
+ if (qemuDomainSaveInternal(driver, vm, path, cfg->saveImageFormat,
compressor, dxml, flags) < 0)
return -1;
@@ -2768,7 +2763,6 @@ qemuDomainSaveFlags(virDomainPtr dom, const char *path, const char
*dxml,
unsigned int flags)
{
virQEMUDriver *driver = dom->conn->privateData;
- int format = QEMU_SAVE_FORMAT_RAW;
g_autoptr(virCommand) compressor = NULL;
int ret = -1;
virDomainObj *vm = NULL;
@@ -2779,11 +2773,7 @@ qemuDomainSaveFlags(virDomainPtr dom, const char *path, const char
*dxml,
VIR_DOMAIN_SAVE_PAUSED, -1);
cfg = virQEMUDriverGetConfig(driver);
- if (cfg->saveImageFormat &&
- (format = qemuSaveFormatTypeFromString(cfg->saveImageFormat)) < 0)
- goto cleanup;
-
- if (qemuSaveImageGetCompressionProgram(format, &compressor, "save")
< 0)
+ if (qemuSaveImageGetCompressionProgram(cfg->saveImageFormat, &compressor,
"save") < 0)
goto cleanup;
if (!(vm = qemuDomainObjFromDomain(dom)))
@@ -2795,7 +2785,7 @@ qemuDomainSaveFlags(virDomainPtr dom, const char *path, const char
*dxml,
if (virDomainObjCheckActive(vm) < 0)
goto cleanup;
- ret = qemuDomainSaveInternal(driver, vm, path, format,
+ ret = qemuDomainSaveInternal(driver, vm, path, cfg->saveImageFormat,
compressor, dxml, flags);
cleanup:
@@ -2821,7 +2811,6 @@ qemuDomainSaveParams(virDomainPtr dom,
g_autoptr(virCommand) compressor = NULL;
const char *to = NULL;
const char *dxml = NULL;
- int format = QEMU_SAVE_FORMAT_RAW;
int ret = -1;
virCheckFlags(VIR_DOMAIN_SAVE_BYPASS_CACHE |
@@ -2855,17 +2844,13 @@ qemuDomainSaveParams(virDomainPtr dom,
}
cfg = virQEMUDriverGetConfig(driver);
- if (cfg->saveImageFormat &&
- (format = qemuSaveFormatTypeFromString(cfg->saveImageFormat)) < 0)
- goto cleanup;
-
- if (qemuSaveImageGetCompressionProgram(format, &compressor, "save")
< 0)
+ if (qemuSaveImageGetCompressionProgram(cfg->saveImageFormat, &compressor,
"save") < 0)
goto cleanup;
if (virDomainObjCheckActive(vm) < 0)
goto cleanup;
- ret = qemuDomainSaveInternal(driver, vm, to, format,
+ ret = qemuDomainSaveInternal(driver, vm, to, cfg->saveImageFormat,
compressor, dxml, flags);
cleanup:
@@ -3069,14 +3054,8 @@ doCoreDump(virQEMUDriver *driver,
const char *memory_dump_format = NULL;
g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
g_autoptr(virCommand) compressor = NULL;
- int format = QEMU_SAVE_FORMAT_RAW;
- if (cfg->dumpImageFormat) {
- if ((format = qemuSaveFormatTypeFromString(cfg->dumpImageFormat)) < 0)
- goto cleanup;
- }
-
- if (qemuSaveImageGetCompressionProgram(format, &compressor, "dump")
< 0)
+ if (qemuSaveImageGetCompressionProgram(cfg->dumpImageFormat, &compressor,
"dump") < 0)
goto cleanup;
/* Create an empty file with appropriate ownership. */
diff --git a/src/qemu/qemu_saveimage.h b/src/qemu/qemu_saveimage.h
index 0d8ee542af..58f8252c8a 100644
--- a/src/qemu/qemu_saveimage.h
+++ b/src/qemu/qemu_saveimage.h
@@ -20,7 +20,6 @@
#include "virconftypes.h"
-#include "qemu_conf.h"
#include "qemu_domain.h"
/* It would be nice to replace 'Qemud' with 'Qemu' but
diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c
index 416a772b92..891e67e98b 100644
--- a/src/qemu/qemu_snapshot.c
+++ b/src/qemu/qemu_snapshot.c
@@ -1597,7 +1597,6 @@ qemuSnapshotCreateActiveExternal(virQEMUDriver *driver,
bool memory_existing = false;
bool thaw = false;
bool pmsuspended = false;
- int format = QEMU_SAVE_FORMAT_RAW;
g_autoptr(virCommand) compressor = NULL;
virQEMUSaveData *data = NULL;
g_autoptr(GHashTable) blockNamedNodeData = NULL;
@@ -1674,12 +1673,8 @@ qemuSnapshotCreateActiveExternal(virQEMUDriver *driver,
JOB_MASK(VIR_JOB_SUSPEND) |
JOB_MASK(VIR_JOB_MIGRATION_OP)));
- if (cfg->snapshotImageFormat &&
- (format = qemuSaveFormatTypeFromString(cfg->snapshotImageFormat)) < 0)
- goto cleanup;
-
- if (qemuSaveImageGetCompressionProgram(format, &compressor,
- "snapshot") < 0)
+ if (qemuSaveImageGetCompressionProgram(cfg->snapshotImageFormat,
+ &compressor, "snapshot")
< 0)
goto cleanup;
if (!(xml = qemuDomainDefFormatLive(driver, priv->qemuCaps,
@@ -1690,7 +1685,7 @@ qemuSnapshotCreateActiveExternal(virQEMUDriver *driver,
if (!(data = virQEMUSaveDataNew(xml,
(qemuDomainSaveCookie *) snapdef->cookie,
- resume, format, driver->xmlopt)))
+ resume, cfg->snapshotImageFormat,
driver->xmlopt)))
goto cleanup;
xml = NULL;
--
2.43.0
6:39 p.m.
New subject: [PATCH V2 4/4] qemu: Check for valid save image formats when
loading driver config
On 2/20/25 17:11, Jim Fehlig wrote:
Checking for valid 'foo_image_format' settings in qemu.conf
is not done
until the settings are used. Move the checks to
virQEMUDriverConfigLoadSaveEntry, allowing to report incorrect format
settings at driver startup.
This change was made easier by also changing the corresponding fields
in the virQEMUDriverConfig to 'int', which is more in line with the
other fields that represent enumerated types.
Signed-off-by: Jim Fehlig <jfehlig(a)suse.com>
---
Splitting the change to virQEMUDriverConfig struct into a separate patch
felt a bit awkward compared to one patch, but I'm fine to do that if
folks prefer.
src/qemu/qemu_conf.c | 35 +++++++++++++++++++++++++++++------
src/qemu/qemu_conf.h | 6 +++---
src/qemu/qemu_driver.c | 35 +++++++----------------------------
src/qemu/qemu_saveimage.h | 1 -
src/qemu/qemu_snapshot.c | 11 +++--------
5 files changed, 42 insertions(+), 46 deletions(-)
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index b73dda7e10..14cfc289b3 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -36,6 +36,7 @@
#include "qemu_domain.h"
#include "qemu_firmware.h"
#include "qemu_namespace.h"
+#include "qemu_saveimage.h"
#include "qemu_security.h"
#include "viruuid.h"
#include "virconf.h"
@@ -374,9 +375,6 @@ static void virQEMUDriverConfigDispose(void *obj)
g_free(cfg->slirpHelperName);
g_free(cfg->dbusDaemonName);
- g_free(cfg->saveImageFormat);
- g_free(cfg->dumpImageFormat);
- g_free(cfg->snapshotImageFormat);
g_free(cfg->autoDumpPath);
g_strfreev(cfg->securityDriverNames);
@@ -626,12 +624,37 @@ static int
virQEMUDriverConfigLoadSaveEntry(virQEMUDriverConfig *cfg,
virConf *conf)
{
- if (virConfGetValueString(conf, "save_image_format",
&cfg->saveImageFormat) < 0)
+ g_autofree char *savestr = NULL;
+ g_autofree char *dumpstr = NULL;
+ g_autofree char *snapstr = NULL;
+
+ if (virConfGetValueString(conf, "save_image_format", &savestr) <
0)
return -1;
- if (virConfGetValueString(conf, "dump_image_format",
&cfg->dumpImageFormat) < 0)
+ if (savestr && (cfg->saveImageFormat =
qemuSaveFormatTypeFromString(savestr)) < 0) {
+ virReportError(VIR_ERR_OPERATION_FAILED,
+ _("Invalid save_image_format '%1$s' specified in
configuration file"),
+ savestr);
I'm not sure about the best error code here, but VIR_ERR_OPERATION_FAILED isn't
it. Probably VIR_ERR_CONFIG_UNSUPPORTED, over VIR_ERR_CONF_SYNTAX? Also, none of
the existing error messages in this file are suffixed with "specified in
configuration file". I'll remove that bit in my local branch.
Regards,
Jim
return -1;
- if (virConfGetValueString(conf, "snapshot_image_format",
&cfg->snapshotImageFormat) < 0)
+ }
+
+ if (virConfGetValueString(conf, "dump_image_format", &dumpstr) <
0)
return -1;
+ if (dumpstr && (cfg->dumpImageFormat =
qemuSaveFormatTypeFromString(dumpstr)) < 0) {
+ virReportError(VIR_ERR_OPERATION_FAILED,
+ _("Invalid dump_image_format '%1$s' specified in
configuration file"),
+ dumpstr);
+ return -1;
+ }
+
+ if (virConfGetValueString(conf, "snapshot_image_format", &snapstr)
< 0)
+ return -1;
+ if (snapstr && (cfg->snapshotImageFormat =
qemuSaveFormatTypeFromString(snapstr)) < 0) {
+ virReportError(VIR_ERR_OPERATION_FAILED,
+ _("Invalid snapshot_image_format '%1$s' specified in
configuration file"),
+ snapstr);
+ return -1;
+ }
+
if (virConfGetValueString(conf, "auto_dump_path",
&cfg->autoDumpPath) < 0)
return -1;
if (virConfGetValueBool(conf, "auto_dump_bypass_cache",
&cfg->autoDumpBypassCache) < 0)
diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h
index 97214f72d0..3e1b41af73 100644
--- a/src/qemu/qemu_conf.h
+++ b/src/qemu/qemu_conf.h
@@ -193,9 +193,9 @@ struct _virQEMUDriverConfig {
bool securityDefaultConfined;
bool securityRequireConfined;
- char *saveImageFormat;
- char *dumpImageFormat;
- char *snapshotImageFormat;
+ int saveImageFormat;
+ int dumpImageFormat;
+ int snapshotImageFormat;
char *autoDumpPath;
bool autoDumpBypassCache;
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 4d94985635..76b808b98f 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -2731,7 +2731,6 @@ qemuDomainManagedSaveHelper(virQEMUDriver *driver,
g_autoptr(virQEMUDriverConfig) cfg = NULL;
g_autoptr(virCommand) compressor = NULL;
g_autofree char *path = NULL;
- int format = QEMU_SAVE_FORMAT_RAW;
if (virDomainObjCheckActive(vm) < 0)
return -1;
@@ -2743,18 +2742,14 @@ qemuDomainManagedSaveHelper(virQEMUDriver *driver,
}
cfg = virQEMUDriverGetConfig(driver);
- if (cfg->saveImageFormat &&
- (format = qemuSaveFormatTypeFromString(cfg->saveImageFormat)) < 0)
- return -1;
-
- if (qemuSaveImageGetCompressionProgram(format, &compressor, "save")
< 0)
+ if (qemuSaveImageGetCompressionProgram(cfg->saveImageFormat, &compressor,
"save") < 0)
return -1;
path = qemuDomainManagedSavePath(driver, vm);
VIR_INFO("Saving state of domain '%s' to '%s'",
vm->def->name, path);
- if (qemuDomainSaveInternal(driver, vm, path, format,
+ if (qemuDomainSaveInternal(driver, vm, path, cfg->saveImageFormat,
compressor, dxml, flags) < 0)
return -1;
@@ -2768,7 +2763,6 @@ qemuDomainSaveFlags(virDomainPtr dom, const char *path, const char
*dxml,
unsigned int flags)
{
virQEMUDriver *driver = dom->conn->privateData;
- int format = QEMU_SAVE_FORMAT_RAW;
g_autoptr(virCommand) compressor = NULL;
int ret = -1;
virDomainObj *vm = NULL;
@@ -2779,11 +2773,7 @@ qemuDomainSaveFlags(virDomainPtr dom, const char *path, const char
*dxml,
VIR_DOMAIN_SAVE_PAUSED, -1);
cfg = virQEMUDriverGetConfig(driver);
- if (cfg->saveImageFormat &&
- (format = qemuSaveFormatTypeFromString(cfg->saveImageFormat)) < 0)
- goto cleanup;
-
- if (qemuSaveImageGetCompressionProgram(format, &compressor, "save")
< 0)
+ if (qemuSaveImageGetCompressionProgram(cfg->saveImageFormat, &compressor,
"save") < 0)
goto cleanup;
if (!(vm = qemuDomainObjFromDomain(dom)))
@@ -2795,7 +2785,7 @@ qemuDomainSaveFlags(virDomainPtr dom, const char *path, const char
*dxml,
if (virDomainObjCheckActive(vm) < 0)
goto cleanup;
- ret = qemuDomainSaveInternal(driver, vm, path, format,
+ ret = qemuDomainSaveInternal(driver, vm, path, cfg->saveImageFormat,
compressor, dxml, flags);
cleanup:
@@ -2821,7 +2811,6 @@ qemuDomainSaveParams(virDomainPtr dom,
g_autoptr(virCommand) compressor = NULL;
const char *to = NULL;
const char *dxml = NULL;
- int format = QEMU_SAVE_FORMAT_RAW;
int ret = -1;
virCheckFlags(VIR_DOMAIN_SAVE_BYPASS_CACHE |
@@ -2855,17 +2844,13 @@ qemuDomainSaveParams(virDomainPtr dom,
}
cfg = virQEMUDriverGetConfig(driver);
- if (cfg->saveImageFormat &&
- (format = qemuSaveFormatTypeFromString(cfg->saveImageFormat)) < 0)
- goto cleanup;
-
- if (qemuSaveImageGetCompressionProgram(format, &compressor, "save")
< 0)
+ if (qemuSaveImageGetCompressionProgram(cfg->saveImageFormat, &compressor,
"save") < 0)
goto cleanup;
if (virDomainObjCheckActive(vm) < 0)
goto cleanup;
- ret = qemuDomainSaveInternal(driver, vm, to, format,
+ ret = qemuDomainSaveInternal(driver, vm, to, cfg->saveImageFormat,
compressor, dxml, flags);
cleanup:
@@ -3069,14 +3054,8 @@ doCoreDump(virQEMUDriver *driver,
const char *memory_dump_format = NULL;
g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
g_autoptr(virCommand) compressor = NULL;
- int format = QEMU_SAVE_FORMAT_RAW;
- if (cfg->dumpImageFormat) {
- if ((format = qemuSaveFormatTypeFromString(cfg->dumpImageFormat)) < 0)
- goto cleanup;
- }
-
- if (qemuSaveImageGetCompressionProgram(format, &compressor, "dump")
< 0)
+ if (qemuSaveImageGetCompressionProgram(cfg->dumpImageFormat, &compressor,
"dump") < 0)
goto cleanup;
/* Create an empty file with appropriate ownership. */
diff --git a/src/qemu/qemu_saveimage.h b/src/qemu/qemu_saveimage.h
index 0d8ee542af..58f8252c8a 100644
--- a/src/qemu/qemu_saveimage.h
+++ b/src/qemu/qemu_saveimage.h
@@ -20,7 +20,6 @@
#include "virconftypes.h"
-#include "qemu_conf.h"
#include "qemu_domain.h"
/* It would be nice to replace 'Qemud' with 'Qemu' but
diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c
index 416a772b92..891e67e98b 100644
--- a/src/qemu/qemu_snapshot.c
+++ b/src/qemu/qemu_snapshot.c
@@ -1597,7 +1597,6 @@ qemuSnapshotCreateActiveExternal(virQEMUDriver *driver,
bool memory_existing = false;
bool thaw = false;
bool pmsuspended = false;
- int format = QEMU_SAVE_FORMAT_RAW;
g_autoptr(virCommand) compressor = NULL;
virQEMUSaveData *data = NULL;
g_autoptr(GHashTable) blockNamedNodeData = NULL;
@@ -1674,12 +1673,8 @@ qemuSnapshotCreateActiveExternal(virQEMUDriver *driver,
JOB_MASK(VIR_JOB_SUSPEND) |
JOB_MASK(VIR_JOB_MIGRATION_OP)));
- if (cfg->snapshotImageFormat &&
- (format = qemuSaveFormatTypeFromString(cfg->snapshotImageFormat)) <
0)
- goto cleanup;
-
- if (qemuSaveImageGetCompressionProgram(format, &compressor,
- "snapshot") < 0)
+ if (qemuSaveImageGetCompressionProgram(cfg->snapshotImageFormat,
+ &compressor, "snapshot")
< 0)
goto cleanup;
if (!(xml = qemuDomainDefFormatLive(driver, priv->qemuCaps,
@@ -1690,7 +1685,7 @@ qemuSnapshotCreateActiveExternal(virQEMUDriver *driver,
if (!(data = virQEMUSaveDataNew(xml,
(qemuDomainSaveCookie *) snapdef->cookie,
- resume, format, driver->xmlopt)))
+ resume, cfg->snapshotImageFormat,
driver->xmlopt)))
goto cleanup;
xml = NULL;
8:42 a.m.
New subject: [PATCH V2 4/4] qemu: Check for valid save image formats when
loading driver config
On Thu, Feb 20, 2025 at 05:39:17PM -0700, Jim Fehlig via Devel wrote:
On 2/20/25 17:11, Jim Fehlig wrote:
> Checking for valid 'foo_image_format' settings in qemu.conf is not done
> until the settings are used. Move the checks to
> virQEMUDriverConfigLoadSaveEntry, allowing to report incorrect format
> settings at driver startup.
>
> This change was made easier by also changing the corresponding fields
> in the virQEMUDriverConfig to 'int', which is more in line with the
> other fields that represent enumerated types.
>
> Signed-off-by: Jim Fehlig <jfehlig(a)suse.com>
> ---
>
> Splitting the change to virQEMUDriverConfig struct into a separate patch
> felt a bit awkward compared to one patch, but I'm fine to do that if
> folks prefer.
>
> src/qemu/qemu_conf.c | 35 +++++++++++++++++++++++++++++------
> src/qemu/qemu_conf.h | 6 +++---
> src/qemu/qemu_driver.c | 35 +++++++----------------------------
> src/qemu/qemu_saveimage.h | 1 -
> src/qemu/qemu_snapshot.c | 11 +++--------
> 5 files changed, 42 insertions(+), 46 deletions(-)
>
> diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
> index b73dda7e10..14cfc289b3 100644
> --- a/src/qemu/qemu_conf.c
> +++ b/src/qemu/qemu_conf.c
> @@ -36,6 +36,7 @@
> #include "qemu_domain.h"
> #include "qemu_firmware.h"
> #include "qemu_namespace.h"
> +#include "qemu_saveimage.h"
> #include "qemu_security.h"
> #include "viruuid.h"
> #include "virconf.h"
> @@ -374,9 +375,6 @@ static void virQEMUDriverConfigDispose(void *obj)
> g_free(cfg->slirpHelperName);
> g_free(cfg->dbusDaemonName);
> - g_free(cfg->saveImageFormat);
> - g_free(cfg->dumpImageFormat);
> - g_free(cfg->snapshotImageFormat);
> g_free(cfg->autoDumpPath);
> g_strfreev(cfg->securityDriverNames);
> @@ -626,12 +624,37 @@ static int
> virQEMUDriverConfigLoadSaveEntry(virQEMUDriverConfig *cfg,
> virConf *conf)
> {
> - if (virConfGetValueString(conf, "save_image_format",
&cfg->saveImageFormat) < 0)
> + g_autofree char *savestr = NULL;
> + g_autofree char *dumpstr = NULL;
> + g_autofree char *snapstr = NULL;
> +
> + if (virConfGetValueString(conf, "save_image_format", &savestr)
< 0)
> return -1;
> - if (virConfGetValueString(conf, "dump_image_format",
&cfg->dumpImageFormat) < 0)
> + if (savestr && (cfg->saveImageFormat =
qemuSaveFormatTypeFromString(savestr)) < 0) {
> + virReportError(VIR_ERR_OPERATION_FAILED,
> + _("Invalid save_image_format '%1$s' specified
in configuration file"),
> + savestr);
I'm not sure about the best error code here, but VIR_ERR_OPERATION_FAILED
isn't it. Probably VIR_ERR_CONFIG_UNSUPPORTED, over VIR_ERR_CONF_SYNTAX?
Also, none of the existing error messages in this file are suffixed with
"specified in configuration file". I'll remove that bit in my local branch.
Yep, CONFIG_UNSUPPORTED with those few words removed is good.
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 :|
8:41 a.m.
New subject: [PATCH V2 4/4] qemu: Check for valid save image formats when
loading driver config
On Thu, Feb 20, 2025 at 05:11:38PM -0700, Jim Fehlig via Devel wrote:
Checking for valid 'foo_image_format' settings in qemu.conf
is not done
until the settings are used. Move the checks to
virQEMUDriverConfigLoadSaveEntry, allowing to report incorrect format
settings at driver startup.
This change was made easier by also changing the corresponding fields
in the virQEMUDriverConfig to 'int', which is more in line with the
other fields that represent enumerated types.
Signed-off-by: Jim Fehlig <jfehlig(a)suse.com>
---
Splitting the change to virQEMUDriverConfig struct into a separate patch
felt a bit awkward compared to one patch, but I'm fine to do that if
folks prefer.
It is ok as is.
src/qemu/qemu_conf.c | 35 +++++++++++++++++++++++++++++------
src/qemu/qemu_conf.h | 6 +++---
src/qemu/qemu_driver.c | 35 +++++++----------------------------
src/qemu/qemu_saveimage.h | 1 -
src/qemu/qemu_snapshot.c | 11 +++--------
5 files changed, 42 insertions(+), 46 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange(a)redhat.com>
with one comment inline...
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index b73dda7e10..14cfc289b3 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -36,6 +36,7 @@
#include "qemu_domain.h"
#include "qemu_firmware.h"
#include "qemu_namespace.h"
+#include "qemu_saveimage.h"
#include "qemu_security.h"
#include "viruuid.h"
#include "virconf.h"
@@ -374,9 +375,6 @@ static void virQEMUDriverConfigDispose(void *obj)
g_free(cfg->slirpHelperName);
g_free(cfg->dbusDaemonName);
- g_free(cfg->saveImageFormat);
- g_free(cfg->dumpImageFormat);
- g_free(cfg->snapshotImageFormat);
g_free(cfg->autoDumpPath);
g_strfreev(cfg->securityDriverNames);
@@ -626,12 +624,37 @@ static int
virQEMUDriverConfigLoadSaveEntry(virQEMUDriverConfig *cfg,
virConf *conf)
{
- if (virConfGetValueString(conf, "save_image_format",
&cfg->saveImageFormat) < 0)
+ g_autofree char *savestr = NULL;
+ g_autofree char *dumpstr = NULL;
+ g_autofree char *snapstr = NULL;
+
+ if (virConfGetValueString(conf, "save_image_format", &savestr) <
0)
return -1;
- if (virConfGetValueString(conf, "dump_image_format",
&cfg->dumpImageFormat) < 0)
+ if (savestr && (cfg->saveImageFormat =
qemuSaveFormatTypeFromString(savestr)) < 0) {
+ virReportError(VIR_ERR_OPERATION_FAILED,
Probably VIR_ERR_CONFIG_UNSUPPORTED is a better bet.
+ _("Invalid save_image_format
'%1$s' specified in configuration file"),
+ savestr);
return -1;
- if (virConfGetValueString(conf, "snapshot_image_format",
&cfg->snapshotImageFormat) < 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 :|
16
days inactive
20
days old
8 comments
2 participants
participants (2)
-
Daniel P. Berrangé -
Jim Fehlig