On Wed, Sep 09, 2009 at 09:24:17AM +0200, Jim Meyering wrote:
> 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@xxxxxxxxxx>
> 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"),
Hum ... in practice I think it adds a new dependancy to libvirt. We
didn't raise that for the lzop patch but if we start to exec binaries
we should check for their presence at runtime somehow.
I'm not against the patch, actually I have xz on my machine but not
lzop, but the whole code right now looks quite fragile, we don't offer
the capacity to detect what compressors are available on the Node and
just leave the API user to a game of try/failure checking. That doesn't
sounds too good to me. One way is to add the dependancy at the packaging
level (at least libvirt.spec.in for now) but right now requesting both
lzop and xz is just bad IMHO, so some kind of runtime detection should
be done. For gzip and bzip2 this wasn't really a probem because they are
really ubiquitous, but now we are suggesting 2 options which are very
likely to not be both present.
I dislike the fact that we are lowering the API reliability for an
optimization in disk space usage.
Daniel
--
Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/
daniel@xxxxxxxxxxxx | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list