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