Re: [PATCH V2 4/4] qemu: Check for valid save image formats when loading driver config

[Jim Fehlig via Devel <devel@xxxxxxxxxxxxxxxxx>]

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.

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;
>