qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH V4 08/10] block/qcow2: start using the compress


From: Eric Blake
Subject: Re: [Qemu-devel] [PATCH V4 08/10] block/qcow2: start using the compress format extension
Date: Thu, 20 Jul 2017 11:00:01 -0500
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.2.1

On 07/20/2017 09:20 AM, Peter Lieven wrote:
> we now pass the parameters to the zlib compressor if the
> extension is present and use the old default values if
> the extension is absent.
> 
> Signed-off-by: Peter Lieven <address@hidden>
> ---
>  block/qcow2-cluster.c | 58 
> ++++++++++++++++++++++++++++++---------------------
>  block/qcow2.c         | 57 +++++++++++++++++++++++++++-----------------------
>  2 files changed, 65 insertions(+), 50 deletions(-)

>  static int decompress_buffer(uint8_t *out_buf, int out_buf_size,
> -                             const uint8_t *buf, int buf_size)
> +                             const uint8_t *buf, int buf_size,
> +                             int compress_format)
>  {

> -
> -    ret = inflateInit2(strm, -12);

The old code initializes with 12,


> +    switch (compress_format) {
> +    case QCOW2_COMPRESS_FORMAT_ZLIB:
> +    case -1: {

What is case -1 supposed to be? When the compression format was not
specified?  Again, I'd really like there a way to encode the default
(when no format is encoded in the qcow2 file) to be representable
alongside the new code, and then we just special-case writing the
compression format to the file to omit the extension header if the user
requested parameters that match the old default.

> +        z_stream z_strm = {};
> +
> +        z_strm.next_in = (uint8_t *)buf;
> +        z_strm.avail_in = buf_size;
> +        z_strm.next_out = out_buf;
> +        z_strm.avail_out = out_buf_size;
> +
> +        ret = inflateInit2(&z_strm, -15);

The new code is now unconditionally initializing with -15 instead of
-12.  Does that matter, or does decompression work regardless of window
size used at creation, as long as the initialized size at decompression
is at least as large?  On the other hand, I guess that means if someone
compresses with a large window, and then I initialize the decompressor
with a small window, my decompression will fail?  That's why knowing the
minimum window size should be part of the spec, whether or not we make
it a tunable.

> +++ b/block/qcow2.c
> @@ -3391,9 +3391,10 @@ qcow2_co_pwritev_compressed(BlockDriverState *bs, 
> uint64_t offset,
>      BDRVQcow2State *s = bs->opaque;
>      QEMUIOVector hd_qiov;
>      struct iovec iov;
> -    z_stream strm;
> -    int ret, out_len;
> -    uint8_t *buf, *out_buf, *local_buf = NULL;
> +    z_stream z_strm = {};
> +    int z_windowBits = -15, z_level = Z_DEFAULT_COMPRESSION;
> +    int ret, out_len = 0;
> +    uint8_t *buf, *out_buf = NULL, *local_buf = NULL;
>      uint64_t cluster_offset;
>  
>      if (bytes == 0) {
> @@ -3418,34 +3419,38 @@ qcow2_co_pwritev_compressed(BlockDriverState *bs, 
> uint64_t offset,
>          buf = qiov->iov[0].iov_base;
>      }
>  
> -    out_buf = g_malloc(s->cluster_size);
> +    switch (s->compress_format) {
> +    case -1:
> +        z_windowBits = -12;
> +    case QCOW2_COMPRESS_FORMAT_ZLIB:
> +        out_buf = g_malloc(s->cluster_size);
> +        if (s->compress_level > 0) {
> +            z_level = s->compress_level;
> +        }

And this is where I wonder if z_windowBits should be explicitly encoded
in the qcow2 format, rather than just magic numbers we use under the
hood, so that someone else trying to reimplement things will create
compatible files.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Attachment: signature.asc
Description: OpenPGP digital signature


reply via email to

[Prev in Thread] Current Thread [Next in Thread]