On Fri, Feb 14, 2025 at 03:48:18PM -0700, Jim Fehlig via Devel wrote:
> qemuSaveImageGetCompressionProgram is a bit overloaded. Along with
> getting a compression program, it checks the validity of the image
> format and returns the integer representation of the format. Change
> the function to only handle retrieving the specified compression
> program, returning success or failure. Checking the validity of
> the image format can be left to the calling functions.
>
> Signed-off-by: Jim Fehlig <jfehlig@xxxxxxxx>
> ---
> src/qemu/qemu_driver.c | 33 +++++++++++++----------
> src/qemu/qemu_saveimage.c | 55 ++++++++++++---------------------------
> src/qemu/qemu_saveimage.h | 2 +-
> src/qemu/qemu_snapshot.c | 11 +++++---
> 4 files changed, 43 insertions(+), 58 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@xxxxxxxxxx>
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 0a1bcc0ed5..577e799c2b 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -2752,7 +2752,7 @@ qemuDomainManagedSaveHelper(virQEMUDriver *driver,
> g_autoptr(virQEMUDriverConfig) cfg = NULL;
> g_autoptr(virCommand) compressor = NULL;
> g_autofree char *path = NULL;
> - int format;
> + int format = QEMU_SAVE_FORMAT_RAW;
>
> if (virDomainObjCheckActive(vm) < 0)
> return -1;
> @@ -2764,9 +2764,11 @@ qemuDomainManagedSaveHelper(virQEMUDriver *driver,
> }
>
> cfg = virQEMUDriverGetConfig(driver);
> - if ((format = qemuSaveImageGetCompressionProgram(cfg->saveImageFormat,
> - &compressor,
> - "save")) < 0)
> + if (cfg->saveImageFormat &&
> + (format = qemuSaveFormatTypeFromString(cfg->saveImageFormat)) < 0)
> + return -1;
As a further patch, I'd suggest we should move the qemuSaveFormatTypeFromString
calls into qemu_conf.c, virQEMUDriverConfigLoadSaveEntry, so that we diagnose
incorrect format strings immediately at startup, rather than delayed to time of
first use.
> + if (qemuSaveImageGetCompressionProgram(format, &compressor, "save") < 0)
> return -1;
Leaving this here would be graceful allowing the user to install the binaries
after libvirtd is running, avoiding wierd errors you would get about a binary
not existing if we probed the binary at startup.
>
> path = qemuDomainManagedSavePath(driver, vm);
> @@ -2787,7 +2789,7 @@ qemuDomainSaveFlags(virDomainPtr dom, const char *path, const char *dxml,
> unsigned int flags)
> {
> virQEMUDriver *driver = dom->conn->privateData;
> - int format;
> + int format = QEMU_SAVE_FORMAT_RAW;
> g_autoptr(virCommand) compressor = NULL;
> int ret = -1;
> virDomainObj *vm = NULL;
> @@ -2798,9 +2800,11 @@ qemuDomainSaveFlags(virDomainPtr dom, const char *path, const char *dxml,
> VIR_DOMAIN_SAVE_PAUSED, -1);
>
> cfg = virQEMUDriverGetConfig(driver);
> - if ((format = qemuSaveImageGetCompressionProgram(cfg->saveImageFormat,
> - &compressor,
> - "save")) < 0)
> + if (cfg->saveImageFormat &&
> + (format = qemuSaveFormatTypeFromString(cfg->saveImageFormat)) < 0)
> + goto cleanup;
> +
> + if (qemuSaveImageGetCompressionProgram(format, &compressor, "save") < 0)
> goto cleanup;
>
> if (!(vm = qemuDomainObjFromDomain(dom)))
> @@ -2838,7 +2842,7 @@ qemuDomainSaveParams(virDomainPtr dom,
> g_autoptr(virCommand) compressor = NULL;
> const char *to = NULL;
> const char *dxml = NULL;
> - int format;
> + int format = QEMU_SAVE_FORMAT_RAW;
> int ret = -1;
>
> virCheckFlags(VIR_DOMAIN_SAVE_BYPASS_CACHE |
> @@ -2872,9 +2876,11 @@ qemuDomainSaveParams(virDomainPtr dom,
> }
>
> cfg = virQEMUDriverGetConfig(driver);
> - if ((format = qemuSaveImageGetCompressionProgram(cfg->saveImageFormat,
> - &compressor,
> - "save")) < 0)
> + if (cfg->saveImageFormat &&
> + (format = qemuSaveFormatTypeFromString(cfg->saveImageFormat)) < 0)
> + goto cleanup;
> +
> + if (qemuSaveImageGetCompressionProgram(format, &compressor, "save") < 0)
> goto cleanup;
>
> if (virDomainObjCheckActive(vm) < 0)
> @@ -3097,8 +3103,7 @@ doCoreDump(virQEMUDriver *driver,
> * format in "save" and "dump". If the compression program doesn't exist,
> * reset any errors and continue on using the raw format.
> */
> - if (qemuSaveImageGetCompressionProgram(cfg->dumpImageFormat,
> - &compressor, "dump") < 0) {
> + if (qemuSaveImageGetCompressionProgram(format, &compressor, "dump") < 0) {
> virResetLastError();
> VIR_WARN("Compression program for dump image format in "
> "configuration file isn't available, using raw");
> diff --git a/src/qemu/qemu_saveimage.c b/src/qemu/qemu_saveimage.c
> index eea35df175..d940bfb5c3 100644
> --- a/src/qemu/qemu_saveimage.c
> +++ b/src/qemu/qemu_saveimage.c
> @@ -508,63 +508,40 @@ qemuSaveImageCreate(virQEMUDriver *driver,
>
>
> /* qemuSaveImageGetCompressionProgram:
> - * @imageFormat: String representation from qemu.conf of the image format
> - * being used (dump, save, or snapshot).
> + * @format: Integer representation of the image format being used
> + * (dump, save, or snapshot).
> * @compresspath: Pointer to a character string to store the fully qualified
> * path from virFindFileInPath.
> * @styleFormat: String representing the style of format (dump, save, snapshot)
> *
> - * Returns:
> - * virQEMUSaveFormat - Integer representation of the save image
> - * format to be used for particular style
> - * (e.g. dump, save, or snapshot).
> - * QEMU_SAVE_FORMAT_RAW - If there is no qemu.conf imageFormat value or
> - * no there was an error, then just return RAW
> - * indicating none.
> + * Returns -1 on failure, 0 on success.
> */
> int
> -qemuSaveImageGetCompressionProgram(const char *imageFormat,
> +qemuSaveImageGetCompressionProgram(int format,
> virCommand **compressor,
> const char *styleFormat)
> {
> - int ret;
> + const char *imageFormat = qemuSaveFormatTypeToString(format);
> const char *prog;
>
> *compressor = NULL;
>
> - if (!imageFormat)
> - return QEMU_SAVE_FORMAT_RAW;
> -
> - if ((ret = qemuSaveFormatTypeFromString(imageFormat)) < 0)
> - goto error;
> -
> - if (ret == QEMU_SAVE_FORMAT_RAW)
> - return QEMU_SAVE_FORMAT_RAW;
> -
> - if (!(prog = virFindFileInPath(imageFormat)))
> - goto error;
> -
> + if (format == QEMU_SAVE_FORMAT_RAW)
> + return 0;
> +
> + if (!(prog = virFindFileInPath(imageFormat))) {
> + virReportError(VIR_ERR_OPERATION_FAILED,
> + _("Compression program for %1$s image format in configuration file isn't available"),
> + styleFormat);
> + return -1;
> + }
>
> *compressor = virCommandNew(prog);
> virCommandAddArg(*compressor, "-c");
> - if (ret == QEMU_SAVE_FORMAT_XZ)
> + if (format == QEMU_SAVE_FORMAT_XZ)
> virCommandAddArg(*compressor, "-3");
>
> - return ret;
> -
> - error:
> - if (ret < 0) {
> - ret = QEMU_SAVE_FORMAT_RAW;
> - virReportError(VIR_ERR_OPERATION_FAILED,
> - _("Invalid %1$s image format specified in configuration file"),
> - styleFormat);
> - } else {
> - virReportError(VIR_ERR_OPERATION_FAILED,
> - _("Compression program for %1$s image format in configuration file isn't available"),
> - styleFormat);
> - }
> -
> - return ret;
> + return 0;
> }
>
>
> diff --git a/src/qemu/qemu_saveimage.h b/src/qemu/qemu_saveimage.h
> index aa905768de..0d8ee542af 100644
> --- a/src/qemu/qemu_saveimage.h
> +++ b/src/qemu/qemu_saveimage.h
> @@ -110,7 +110,7 @@ qemuSaveImageOpen(virQEMUDriver *driver,
> ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(4);
>
> int
> -qemuSaveImageGetCompressionProgram(const char *imageFormat,
> +qemuSaveImageGetCompressionProgram(int format,
> virCommand **compressor,
> const char *styleFormat)
> ATTRIBUTE_NONNULL(2);
> diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c
> index 4ff7e09bd4..c672d4f24f 100644
> --- a/src/qemu/qemu_snapshot.c
> +++ b/src/qemu/qemu_snapshot.c
> @@ -1579,7 +1579,7 @@ qemuSnapshotCreateActiveExternal(virQEMUDriver *driver,
> bool memory_existing = false;
> bool thaw = false;
> bool pmsuspended = false;
> - int format;
> + int format = QEMU_SAVE_FORMAT_RAW;
> g_autoptr(virCommand) compressor = NULL;
> virQEMUSaveData *data = NULL;
> g_autoptr(GHashTable) blockNamedNodeData = NULL;
> @@ -1656,9 +1656,12 @@ qemuSnapshotCreateActiveExternal(virQEMUDriver *driver,
> JOB_MASK(VIR_JOB_SUSPEND) |
> JOB_MASK(VIR_JOB_MIGRATION_OP)));
>
> - if ((format = qemuSaveImageGetCompressionProgram(cfg->snapshotImageFormat,
> - &compressor,
> - "snapshot")) < 0)
> + if (cfg->snapshotImageFormat &&
> + (format = qemuSaveFormatTypeFromString(cfg->snapshotImageFormat)) < 0)
> + goto cleanup;
> +
> + if (qemuSaveImageGetCompressionProgram(format, &compressor,
> + "snapshot") < 0)
> goto cleanup;
>
> if (!(xml = qemuDomainDefFormatLive(driver, priv->qemuCaps,
> --
> 2.43.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 :|
