Re: [libvirt] Re: [PATCH] also allow use of XZ for Qemu image compression

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&gt; 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"),