2:24 a.m.
Jim Meyering wrote:
Chris Lalancette wrote:
> Jim Meyering wrote:
>> While this patch stays minimal by simply adding XZ/xz to the list,
>> I think it would be better to remove lzma, since it uses
>> an inferior format (which lacks an integrity check), and
>> has been effectively subsumed by xz.
>>
>> Let me know if you'd like that, and I'll prepare the slightly
>> more invasive patch.
>
> I'm on the fence about it. While I do understand the situation now (thanks for
> explaining), I think keeping lzma for compatibility with older distros might be
> a good idea. Either way, we have to keep the LZMA slot in the enum
"free",
> since it's part of the on-disk ABI for the save format. And on that note...
>
>> diff --git a/src/qemu_driver.c b/src/qemu_driver.c
>> index f64d70b..7b64712 100644
>> --- a/src/qemu_driver.c
>> +++ b/src/qemu_driver.c
>> @@ -3622,7 +3622,8 @@ enum qemud_save_formats {
>> QEMUD_SAVE_FORMAT_RAW,
>> QEMUD_SAVE_FORMAT_GZIP,
>> QEMUD_SAVE_FORMAT_BZIP2,
>> - QEMUD_SAVE_FORMAT_LZMA,
>> + QEMUD_SAVE_FORMAT_LZMA, /* deprecated, in favor of xz */
>> + QEMUD_SAVE_FORMAT_XZ,
>> QEMUD_SAVE_FORMAT_LZOP,
>> };
>
> You'll need to add QEMUD_SAVE_FORMAT_XZ to the end of the enum, to maintain
> on-disk compatibility. Otherwise it looks good.
Thanks. Good point. Here's an amended patch:
Note, I've reordered the if/else placement to match enum member
ordering, too.
...
At Chris' suggestion, I've added a comment warning not to do what I did ;-)
To make it even more obvious that these numbers matter,
I've assigned explicit constants in the enum:
+ QEMUD_SAVE_FORMAT_RAW = 0,
+ QEMUD_SAVE_FORMAT_GZIP = 1,
+ QEMUD_SAVE_FORMAT_BZIP2 = 2,
Currently, if someone accidentally adds this (mistakenly reused number):
+ QEMUD_SAVE_FORMAT_OOPS = 2,
the compiler won't catch it.
However, with a small additional change to use VIR_ENUM_DECL
and VIR_ENUM_IMPL, thus replacing the if/elsif/ chains,
and the compiler *would* detect that mistake.
It'd also help to factor out the QEMUD_SAVE_FORMAT_<> to "<>"
association, rather than having literals like "gzip" and "bzip2"
in two places.
Now that I've described that, I'll prepare a separate patch.
For now, here's the added comment and enum initializers.
From ef653538806f981ca6ca5bb270e7a8fb1618f6d4 Mon Sep 17 00:00:00 2001
From: Jim Meyering <meyering(a)redhat.com>
Date: Tue, 8 Sep 2009 20:52:37 +0200
Subject: [PATCH] also allow use of XZ for Qemu image compression
* src/qemu_driver.c (enum qemud_save_formats) [QEMUD_SAVE_FORMAT_XZ]:
New member.
[QEMUD_SAVE_FORMAT_LZMA]: Mark as deprecated.
Use an explicit value for each member.
(qemudDomainSave, qemudDomainRestore): Handle the new member.
* src/qemu.conf: Mention xz, too.
---
src/qemu.conf | 2 +-
src/qemu_driver.c | 21 ++++++++++++++++-----
2 files changed, 17 insertions(+), 6 deletions(-)
diff --git a/src/qemu.conf b/src/qemu.conf
index 06babc4..342bb8a 100644
--- a/src/qemu.conf
+++ b/src/qemu.conf
@@ -134,7 +134,7 @@
# 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 "gzip", "bzip2",
"lzma"
+# are being saved to disk, you can also set "gzip", "bzip2",
"lzma", "xz",
# or "lzop" for save_image_format. Note that this means you slow down
# the process of saving a domain in order to save disk space.
#
diff --git a/src/qemu_driver.c b/src/qemu_driver.c
index f64d70b..0cdcd98 100644
--- a/src/qemu_driver.c
+++ b/src/qemu_driver.c
@@ -3619,11 +3619,15 @@ static char *qemudEscapeShellArg(const char *in)
#define QEMUD_SAVE_VERSION 2
enum qemud_save_formats {
- QEMUD_SAVE_FORMAT_RAW,
- QEMUD_SAVE_FORMAT_GZIP,
- QEMUD_SAVE_FORMAT_BZIP2,
- QEMUD_SAVE_FORMAT_LZMA,
- QEMUD_SAVE_FORMAT_LZOP,
+ QEMUD_SAVE_FORMAT_RAW = 0,
+ QEMUD_SAVE_FORMAT_GZIP = 1,
+ QEMUD_SAVE_FORMAT_BZIP2 = 2,
+ QEMUD_SAVE_FORMAT_LZMA = 3, /* deprecated, in favor of xz */
+ QEMUD_SAVE_FORMAT_LZOP = 4,
+ QEMUD_SAVE_FORMAT_XZ = 5,
+ /* Note: add new members only at the end.
+ These values are used in the on-disk format.
+ Do not change or re-use numbers. */
};
struct qemud_save_header {
@@ -3666,6 +3670,8 @@ static int qemudDomainSave(virDomainPtr dom,
header.compressed = QEMUD_SAVE_FORMAT_LZMA;
else if (STREQ(driver->saveImageFormat, "lzop"))
header.compressed = QEMUD_SAVE_FORMAT_LZOP;
+ else if (STREQ(driver->saveImageFormat, "xz"))
+ header.compressed = QEMUD_SAVE_FORMAT_XZ;
else {
qemudReportError(dom->conn, dom, NULL, VIR_ERR_OPERATION_FAILED,
"%s", _("Invalid save image format specified in
configuration file"));
@@ -3761,6 +3767,9 @@ static int qemudDomainSave(virDomainPtr dom,
else if (header.compressed == QEMUD_SAVE_FORMAT_LZOP)
internalret = virAsprintf(&command, "migrate \"exec:"
"lzop -c >> '%s'
2>/dev/null\"", safe_path);
+ else if (header.compressed == QEMUD_SAVE_FORMAT_XZ)
+ internalret = virAsprintf(&command, "migrate \"exec:"
+ "xz -c >> '%s'
2>/dev/null\"", safe_path);
else {
qemudReportError(dom->conn, dom, NULL, VIR_ERR_INTERNAL_ERROR,
_("Invalid compress format %d"),
@@ -4385,6 +4394,8 @@ static int qemudDomainRestore(virConnectPtr conn,
intermediate_argv[0] = "lzma";
else if (header.compressed == QEMUD_SAVE_FORMAT_LZOP)
intermediate_argv[0] = "lzop";
+ else if (header.compressed == QEMUD_SAVE_FORMAT_XZ)
+ intermediate_argv[0] = "xz";
else if (header.compressed != QEMUD_SAVE_FORMAT_RAW) {
qemudReportError(conn, NULL, NULL, VIR_ERR_OPERATION_FAILED,
_("Unknown compressed save format %d"),
--
1.6.5.rc0.164.g5f6b0