[PATCH V2 0/2] qemu: Clarify purpose of image format settings
The current documentation of the various foo_image_format settings in qemu.conf subtly implies they are only used for specifying compression. Patch1 of this small series attempts to clarify and improve the description of the settings. It defines image format as a way to specify the desired layout of guest memory blocks on disk. Patch2 changes the use of 'compressed' with 'format' throughout the code, removing implication that format == compressed. V2: Replace more uses of 'compressed' with 'format' in patch2 Jim Fehlig (2): qemu: conf: Improve the foo_image_format setting descriptions qemu: Use consistent naming for save image format src/qemu/qemu.conf.in | 40 +++++++++++++++++++++++---------------- src/qemu/qemu_driver.c | 30 ++++++++++++++--------------- src/qemu/qemu_saveimage.c | 32 +++++++++++++++---------------- src/qemu/qemu_saveimage.h | 4 ++-- src/qemu/qemu_snapshot.c | 6 +++--- 5 files changed, 60 insertions(+), 52 deletions(-) -- 2.35.3
The current description of the various foo_image_format settings can be construded to imply the setting is only used to control compression of the image. Improve the documentation to clarify that format describes the representation of guest memory blocks on disk, which includes compression among other possible layouts. Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- src/qemu/qemu.conf.in | 40 ++++++++++++++++++++++++---------------- 1 file changed, 24 insertions(+), 16 deletions(-) diff --git a/src/qemu/qemu.conf.in b/src/qemu/qemu.conf.in index 6bc2140dcb..d4fdd717ba 100644 --- a/src/qemu/qemu.conf.in +++ b/src/qemu/qemu.conf.in @@ -578,27 +578,35 @@ # "/dev/infiniband/uverbs0" -# The default format for QEMU/KVM guest save images is raw; that is, the -# memory from the domain is dumped out directly to a file. If you have -# guests with a large amount of memory, however, this can take up quite -# a bit of space. If you would like to compress the images while they -# are being saved to disk, you can also set "zstd", "lzop", "gzip", "bzip2", -# or "xz" for save_image_format. Note that this means you slow down the process -# of saving a domain in order to save disk space. +# The libvirt QEMU driver supports serveral different save image formats. +# The term "format" is used loosely to describe how the save image data is +# represented on disk. It could be a continguous stream of guest memory blocks, +# or a stream of compressed memory blocks. # -# save_image_format is used when you use 'virsh save' or 'virsh managedsave' -# at scheduled saving, and it is an error if the specified save_image_format -# is not valid, or the requested compression program can't be found. +# A continguous stream of guest memory blocks is the default format for QEMU/KVM +# guest save images and is termed "raw". The raw format can consume considerable +# disk space when saving large memory guests. Various compression formats are +# available for specifying a save image compressed by the named algorithm. +# Supported compression formats are "zstd", "lzop", "gzip", "bzip2", and "xz". + +# save_image_format can be used to select the desired save format. "raw" is +# the traditional format used by libvirt and is also the default. The +# compression formats can be used to save disk space, although this typically +# results in longer save and restore times. # -# dump_image_format is used when you use 'virsh dump' at emergency -# crashdump, and if the specified dump_image_format is not valid, or -# the requested compression program can't be found, this falls -# back to "raw" compression. +# save_image_format is used with 'virsh save' or 'virsh managedsave'. It is +# an error if the specified save_image_format is not valid, or cannot be +# supported by the system. # -# snapshot_image_format specifies the compression algorithm of the memory save +# dump_image_format is analogous to save_image_format and is used with +# 'virsh dump' at emergency crashdump. If the specified dump_image_format is +# not valid or cannot be supported by the system, this falls back to the +# "raw" format. +# +# Likewise, snapshot_image_format specifies the format of the memory save # image when an external snapshot of a domain is taken. This does not apply # on disk image format. It is an error if the specified format isn't valid, -# or the requested compression program can't be found. +# or the system cannot support the requested format. # #save_image_format = "raw" #dump_image_format = "raw" -- 2.35.3
On Fri, Aug 16, 2024 at 04:25:25PM -0600, Jim Fehlig via Devel wrote:
The current description of the various foo_image_format settings can be construded to imply the setting is only used to control compression of the image. Improve the documentation to clarify that format describes the representation of guest memory blocks on disk, which includes compression among other possible layouts.
Signed-off-by: Jim Fehlig <jfehlig@suse.com>
Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
On 10/2/24 02:37, Martin Kletzander wrote:
On Fri, Aug 16, 2024 at 04:25:25PM -0600, Jim Fehlig via Devel wrote:
The current description of the various foo_image_format settings can be construded to imply the setting is only used to control compression of the image. Improve the documentation to clarify that format describes the representation of guest memory blocks on disk, which includes compression among other possible layouts.
Signed-off-by: Jim Fehlig <jfehlig@suse.com>
Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
Thanks for reviewing this series Martin! Does it imply we can use the image_format settings to support mapped-ram? It was previously discussed, but no consensus AFAIK https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/message/RCS5... Regards, Jim
On Tue, Oct 08, 2024 at 11:25:29AM -0600, Jim Fehlig wrote:
On 10/2/24 02:37, Martin Kletzander wrote:
On Fri, Aug 16, 2024 at 04:25:25PM -0600, Jim Fehlig via Devel wrote:
The current description of the various foo_image_format settings can be construded to imply the setting is only used to control compression of the image. Improve the documentation to clarify that format describes the representation of guest memory blocks on disk, which includes compression among other possible layouts.
Signed-off-by: Jim Fehlig <jfehlig@suse.com>
Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
Thanks for reviewing this series Martin! Does it imply we can use the image_format settings to support mapped-ram? It was previously discussed, but no consensus AFAIK
https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/message/RCS5...
I'm fine with that, I have your series selected for review, but to be honest I'm getting a bit lost in the linked mail at the moment.
Regards, Jim
The image format setting in qemu.conf is named 'save_image_format'. The enum of supported format types is declared with name 'virQEMUSaveFormat'. Let's be consistent and use 'format' instead of 'compressed' when referring to the save image format. Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- src/qemu/qemu_driver.c | 30 +++++++++++++++--------------- src/qemu/qemu_saveimage.c | 32 ++++++++++++++++---------------- src/qemu/qemu_saveimage.h | 4 ++-- src/qemu/qemu_snapshot.c | 6 +++--- 4 files changed, 36 insertions(+), 36 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 3801ad623a..b114f4b9c2 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -2749,7 +2749,7 @@ qemuDomainManagedSaveHelper(virQEMUDriver *driver, g_autoptr(virQEMUDriverConfig) cfg = NULL; g_autoptr(virCommand) compressor = NULL; g_autofree char *path = NULL; - int compressed; + int format; if (virDomainObjCheckActive(vm) < 0) return -1; @@ -2761,16 +2761,16 @@ qemuDomainManagedSaveHelper(virQEMUDriver *driver, } cfg = virQEMUDriverGetConfig(driver); - if ((compressed = qemuSaveImageGetCompressionProgram(cfg->saveImageFormat, - &compressor, - "save", false)) < 0) + if ((format = qemuSaveImageGetCompressionProgram(cfg->saveImageFormat, + &compressor, + "save", false)) < 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, compressed, + if (qemuDomainSaveInternal(driver, vm, path, format, compressor, dxml, flags) < 0) return -1; @@ -2784,7 +2784,7 @@ qemuDomainSaveFlags(virDomainPtr dom, const char *path, const char *dxml, unsigned int flags) { virQEMUDriver *driver = dom->conn->privateData; - int compressed; + int format; g_autoptr(virCommand) compressor = NULL; int ret = -1; virDomainObj *vm = NULL; @@ -2795,9 +2795,9 @@ qemuDomainSaveFlags(virDomainPtr dom, const char *path, const char *dxml, VIR_DOMAIN_SAVE_PAUSED, -1); cfg = virQEMUDriverGetConfig(driver); - if ((compressed = qemuSaveImageGetCompressionProgram(cfg->saveImageFormat, - &compressor, - "save", false)) < 0) + if ((format = qemuSaveImageGetCompressionProgram(cfg->saveImageFormat, + &compressor, + "save", false)) < 0) goto cleanup; if (!(vm = qemuDomainObjFromDomain(dom))) @@ -2809,7 +2809,7 @@ qemuDomainSaveFlags(virDomainPtr dom, const char *path, const char *dxml, if (virDomainObjCheckActive(vm) < 0) goto cleanup; - ret = qemuDomainSaveInternal(driver, vm, path, compressed, + ret = qemuDomainSaveInternal(driver, vm, path, format, compressor, dxml, flags); cleanup: @@ -2835,7 +2835,7 @@ qemuDomainSaveParams(virDomainPtr dom, g_autoptr(virCommand) compressor = NULL; const char *to = NULL; const char *dxml = NULL; - int compressed; + int format; int ret = -1; virCheckFlags(VIR_DOMAIN_SAVE_BYPASS_CACHE | @@ -2869,15 +2869,15 @@ qemuDomainSaveParams(virDomainPtr dom, } cfg = virQEMUDriverGetConfig(driver); - if ((compressed = qemuSaveImageGetCompressionProgram(cfg->saveImageFormat, - &compressor, - "save", false)) < 0) + if ((format = qemuSaveImageGetCompressionProgram(cfg->saveImageFormat, + &compressor, + "save", false)) < 0) goto cleanup; if (virDomainObjCheckActive(vm) < 0) goto cleanup; - ret = qemuDomainSaveInternal(driver, vm, to, compressed, + ret = qemuDomainSaveInternal(driver, vm, to, format, compressor, dxml, flags); cleanup: diff --git a/src/qemu/qemu_saveimage.c b/src/qemu/qemu_saveimage.c index 018ab5a222..69617e07eb 100644 --- a/src/qemu/qemu_saveimage.c +++ b/src/qemu/qemu_saveimage.c @@ -55,8 +55,8 @@ typedef enum { QEMU_SAVE_FORMAT_LAST } virQEMUSaveFormat; -VIR_ENUM_DECL(qemuSaveCompression); -VIR_ENUM_IMPL(qemuSaveCompression, +VIR_ENUM_DECL(qemuSaveFormat); +VIR_ENUM_IMPL(qemuSaveFormat, QEMU_SAVE_FORMAT_LAST, "raw", "gzip", @@ -72,7 +72,7 @@ qemuSaveImageBswapHeader(virQEMUSaveHeader *hdr) hdr->version = GUINT32_SWAP_LE_BE(hdr->version); hdr->data_len = GUINT32_SWAP_LE_BE(hdr->data_len); hdr->was_running = GUINT32_SWAP_LE_BE(hdr->was_running); - hdr->compressed = GUINT32_SWAP_LE_BE(hdr->compressed); + hdr->format = GUINT32_SWAP_LE_BE(hdr->format); hdr->cookieOffset = GUINT32_SWAP_LE_BE(hdr->cookieOffset); } @@ -97,7 +97,7 @@ virQEMUSaveData * virQEMUSaveDataNew(char *domXML, qemuDomainSaveCookie *cookieObj, bool running, - int compressed, + int format, virDomainXMLOption *xmlopt) { virQEMUSaveData *data = NULL; @@ -114,7 +114,7 @@ virQEMUSaveDataNew(char *domXML, memcpy(header->magic, QEMU_SAVE_PARTIAL, sizeof(header->magic)); header->version = QEMU_SAVE_VERSION; header->was_running = running ? 1 : 0; - header->compressed = compressed; + header->format = format; data->xml = domXML; return data; @@ -227,22 +227,22 @@ virQEMUSaveDataFinish(virQEMUSaveData *data, static virCommand * -qemuSaveImageGetCompressionCommand(virQEMUSaveFormat compression) +qemuSaveImageGetCompressionCommand(virQEMUSaveFormat format) { virCommand *ret = NULL; - const char *prog = qemuSaveCompressionTypeToString(compression); + const char *prog = qemuSaveFormatTypeToString(format); if (!prog) { virReportError(VIR_ERR_OPERATION_FAILED, _("Invalid compressed save format %1$d"), - compression); + format); return NULL; } ret = virCommandNew(prog); virCommandAddArg(ret, "-dc"); - if (compression == QEMU_SAVE_FORMAT_LZOP) + if (format == QEMU_SAVE_FORMAT_LZOP) virCommandAddArg(ret, "--ignore-warn"); return ret; @@ -282,10 +282,10 @@ qemuSaveImageDecompressionStart(virQEMUSaveData *data, if (header->version != 2) return 0; - if (header->compressed == QEMU_SAVE_FORMAT_RAW) + if (header->format == QEMU_SAVE_FORMAT_RAW) return 0; - if (!(cmd = qemuSaveImageGetCompressionCommand(header->compressed))) + if (!(cmd = qemuSaveImageGetCompressionCommand(header->format))) return -1; *intermediatefd = *fd; @@ -443,8 +443,8 @@ qemuSaveImageCreate(virQEMUDriver *driver, /* qemuSaveImageGetCompressionProgram: - * @imageFormat: String representation from qemu.conf for the compression - * image format being used (dump, save, or snapshot). + * @imageFormat: String representation from qemu.conf 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) @@ -454,8 +454,8 @@ qemuSaveImageCreate(virQEMUDriver *driver, * and let the path remain as NULL. * * Returns: - * virQEMUSaveFormat - Integer representation of the compression - * program to be used for particular style + * 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 @@ -475,7 +475,7 @@ qemuSaveImageGetCompressionProgram(const char *imageFormat, if (!imageFormat) return QEMU_SAVE_FORMAT_RAW; - if ((ret = qemuSaveCompressionTypeFromString(imageFormat)) < 0) + if ((ret = qemuSaveFormatTypeFromString(imageFormat)) < 0) goto error; if (ret == QEMU_SAVE_FORMAT_RAW) diff --git a/src/qemu/qemu_saveimage.h b/src/qemu/qemu_saveimage.h index e541792153..0e58dd14b6 100644 --- a/src/qemu/qemu_saveimage.h +++ b/src/qemu/qemu_saveimage.h @@ -38,7 +38,7 @@ struct _virQEMUSaveHeader { uint32_t version; uint32_t data_len; uint32_t was_running; - uint32_t compressed; + uint32_t format; uint32_t cookieOffset; uint32_t unused[14]; }; @@ -121,7 +121,7 @@ virQEMUSaveData * virQEMUSaveDataNew(char *domXML, qemuDomainSaveCookie *cookieObj, bool running, - int compressed, + int format, virDomainXMLOption *xmlopt); void diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index f5260c4a22..0683bc5939 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -1302,7 +1302,7 @@ qemuSnapshotCreateActiveExternal(virQEMUDriver *driver, bool memory_existing = false; bool thaw = false; bool pmsuspended = false; - int compressed; + int format; g_autoptr(virCommand) compressor = NULL; virQEMUSaveData *data = NULL; g_autoptr(GHashTable) blockNamedNodeData = NULL; @@ -1379,7 +1379,7 @@ qemuSnapshotCreateActiveExternal(virQEMUDriver *driver, JOB_MASK(VIR_JOB_SUSPEND) | JOB_MASK(VIR_JOB_MIGRATION_OP))); - if ((compressed = qemuSaveImageGetCompressionProgram(cfg->snapshotImageFormat, + if ((format = qemuSaveImageGetCompressionProgram(cfg->snapshotImageFormat, &compressor, "snapshot", false)) < 0) goto cleanup; @@ -1392,7 +1392,7 @@ qemuSnapshotCreateActiveExternal(virQEMUDriver *driver, if (!(data = virQEMUSaveDataNew(xml, (qemuDomainSaveCookie *) snapdef->cookie, - resume, compressed, driver->xmlopt))) + resume, format, driver->xmlopt))) goto cleanup; xml = NULL; -- 2.35.3
On Fri, Aug 16, 2024 at 04:25:26PM -0600, Jim Fehlig via Devel wrote:
The image format setting in qemu.conf is named 'save_image_format'. The enum of supported format types is declared with name 'virQEMUSaveFormat'. Let's be consistent and use 'format' instead of 'compressed' when referring to the save image format.
Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- src/qemu/qemu_driver.c | 30 +++++++++++++++--------------- src/qemu/qemu_saveimage.c | 32 ++++++++++++++++---------------- src/qemu/qemu_saveimage.h | 4 ++-- src/qemu/qemu_snapshot.c | 6 +++--- 4 files changed, 36 insertions(+), 36 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 3801ad623a..b114f4b9c2 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -2761,16 +2761,16 @@ qemuDomainManagedSaveHelper(virQEMUDriver *driver, }
cfg = virQEMUDriverGetConfig(driver); - if ((compressed = qemuSaveImageGetCompressionProgram(cfg->saveImageFormat, - &compressor, - "save", false)) < 0) + if ((format = qemuSaveImageGetCompressionProgram(cfg->saveImageFormat, + &compressor, + "save", false)) < 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, compressed, + if (qemuDomainSaveInternal(driver, vm, path, format, compressor, dxml, flags) < 0) return -1;
It'd be nice to also squash this in: diff --git i/src/qemu/qemu_driver.c w/src/qemu/qemu_driver.c index aa5fb9a873b0..f27c21ca8cc9 100644 --- i/src/qemu/qemu_driver.c +++ w/src/qemu/qemu_driver.c @@ -2605,7 +2605,7 @@ static int qemuDomainSaveInternal(virQEMUDriver *driver, virDomainObj *vm, const char *path, - int compressed, + int format, virCommand *compressor, const char *xmlin, unsigned int flags) @@ -2683,7 +2683,7 @@ qemuDomainSaveInternal(virQEMUDriver *driver, if (!(cookie = qemuDomainSaveCookieNew(vm))) goto endjob; - if (!(data = virQEMUSaveDataNew(xml, cookie, was_running, compressed, + if (!(data = virQEMUSaveDataNew(xml, cookie, was_running, format, driver->xmlopt))) goto endjob; xml = NULL; -- [...]
diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index f5260c4a22..0683bc5939 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -1379,7 +1379,7 @@ qemuSnapshotCreateActiveExternal(virQEMUDriver *driver, JOB_MASK(VIR_JOB_SUSPEND) | JOB_MASK(VIR_JOB_MIGRATION_OP)));
- if ((compressed = qemuSaveImageGetCompressionProgram(cfg->snapshotImageFormat, + if ((format = qemuSaveImageGetCompressionProgram(cfg->snapshotImageFormat, &compressor, "snapshot", false)) < 0)
Indentation.
goto cleanup;
With the above changes Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
Hi All, Any comments on this idea? Recall the motivation for this change is to subsequently use the image format settings to request mapped-ram. Regards, Jim On 8/16/24 16:25, Jim Fehlig wrote:
The current documentation of the various foo_image_format settings in qemu.conf subtly implies they are only used for specifying compression. Patch1 of this small series attempts to clarify and improve the description of the settings. It defines image format as a way to specify the desired layout of guest memory blocks on disk.
Patch2 changes the use of 'compressed' with 'format' throughout the code, removing implication that format == compressed.
V2: Replace more uses of 'compressed' with 'format' in patch2
Jim Fehlig (2): qemu: conf: Improve the foo_image_format setting descriptions qemu: Use consistent naming for save image format
src/qemu/qemu.conf.in | 40 +++++++++++++++++++++++---------------- src/qemu/qemu_driver.c | 30 ++++++++++++++--------------- src/qemu/qemu_saveimage.c | 32 +++++++++++++++---------------- src/qemu/qemu_saveimage.h | 4 ++-- src/qemu/qemu_snapshot.c | 6 +++--- 5 files changed, 60 insertions(+), 52 deletions(-)
participants (2)
-
Jim Fehlig -
Martin Kletzander