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