Re: [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&gt; > --- > > 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.