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@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 :|