Re: [PATCH 2/3] qemu: Move special handling of invalid dump format to only caller
On Fri, Feb 14, 2025 at 03:48:17PM -0700, Jim Fehlig via Devel wrote:
The 'use_raw_on_fail' logic in
qemuSaveImageGetCompressionProgram is only
used by doCoreDump in qemu_driver.c. Move the logic to the single call site
and remove the parameter from qemuSaveImageGetCompressionProgram.
Signed-off-by: Jim Fehlig <jfehlig(a)suse.com>
---
src/qemu/qemu_driver.c | 29 ++++++++++++++++++++---------
src/qemu/qemu_saveimage.c | 38 ++++++++++----------------------------
src/qemu/qemu_saveimage.h | 3 +--
src/qemu/qemu_snapshot.c | 2 +-
4 files changed, 32 insertions(+), 40 deletions(-)
@@ -3085,13 +3086,23 @@ doCoreDump(virQEMUDriver *driver,
g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
g_autoptr(virCommand) compressor = NULL;
+ if (cfg->dumpImageFormat) {
+ if ((format = qemuSaveFormatTypeFromString(cfg->dumpImageFormat)) < 0) {
+ VIR_WARN("Invalid dump image format specified in configuration file,
using raw");
+ format = QEMU_SAVE_FORMAT_RAW;
+ }
+ }
+
IMHO this is not something we should do - if the user typo'd in their
configuration file that should always be an hard error, so they see
their mistake immediately.
/* We reuse "save" flag for "dump" here.
Then, we can support the same
- * format in "save" and "dump". This path doesn't need the
compression
- * program to exist and can ignore the return value - it only cares to
- * get the compressor */
- ignore_value(qemuSaveImageGetCompressionProgram(cfg->dumpImageFormat,
- &compressor,
- "dump", true));
+ * 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) {
+ virResetLastError();
+ VIR_WARN("Compression program for dump image format in "
+ "configuration file isn't available, using raw");
+ }
I'm trying to understand why we should special case 'dump' in this way.
This special case goes back to:
commit 48cb9f0542d70f9e3ac91b9b7d82fc9b85807b4e
Author: John Ferlan <jferlan(a)redhat.com>
Date: Tue Sep 13 10:11:00 2016 -0400
qemu: Use qemuGetCompressionProgram for error paths
Let's do some more code reuse - there are 3 other callers that care to
check/get the compress program. Each of those though cares whether the
requested cfg image is valid and exists. So, add a parameter to handle
those cases.
NB: We won't need to initialize the returned value in the case where
the cfg image doesn't exist since the called program will handle that.
That commit message doesn't justify why dump needs special handling.
We can see though the original code it was "unifying", lacked any error
handling for the dump case, and this refactoring preserved that.
I'm rather inclined to say that was a mistake. Unless someone can just
an attractive justification, I think dump should be doing the same error
checking as the other cases. ie if the user asked for gzip, they should
get gzip, or an error if that's impossible, rather than ignoring their
requested config.
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 :|