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

On Wed, Sep 09, 2009 at 09:24:17AM +0200, Jim Meyering wrote: > >>> 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.

> 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.