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

[Daniel Veillard]

On Wed, Sep 09, 2009 at 10:49:46AM +0200, Daniel Veillard wrote:
> 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.

  Hum, I realize that support of LZOP was added after 0.7.0, so we never
made a release with it (well except for git snapshot which may have been
pushed).
  I wonder if the best is not to just drop the lzop option altogether
and stick xz as a package dependancy until we have found a way to
provide at the API level which compression options are actually
available.

  Opinions ?

Daniel

-- 
Daniel Veillard      | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
daniel at veillard.com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/