Re: [Qemu-devel] [PATCH v0 2/3] qcow2: add compression type processing

[Kevin Wolf]

Am 28.05.2019 um 16:37 hat Denis Plotnikov geschrieben:
> With the patch, qcow2 is able to process image compression type
> defined in the image header and choose the corresponding method
> for clusters compressing.
> 
> Also, it rework the cluster compression code for adding more
> compression types.
> 
> Signed-off-by: Denis Plotnikov <address@hidden>
> ---
>  block/qcow2.c | 103 ++++++++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 92 insertions(+), 11 deletions(-)
> 
> diff --git a/block/qcow2.c b/block/qcow2.c
> index c4b5b93408..90f15cc3c9 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -400,11 +400,39 @@ static int qcow2_read_extensions(BlockDriverState *bs, 
> uint64_t start_offset,
>              break;
>  
>          case QCOW2_EXT_MAGIC_COMPRESSION_TYPE:
> +            /* Compression type always goes with the compression type bit 
> set */
> +            if (!(s->incompatible_features & 
> QCOW2_INCOMPAT_COMPRESSION_TYPE)) {
> +                error_setg(errp,
> +                           "compression_type_ext: "
> +                           "expect compression type bit set");
> +                return -EINVAL;
> +            }
> +
> +            ret = bdrv_pread(bs->file, offset, &s->compression_type, 
> ext.len);
> +            s->compression_type = be32_to_cpu(s->compression_type);
> +
> +            if (ret < 0) {
> +                error_setg_errno(errp, -ret,
> +                                 "ERROR: Could not read compression type");
> +                return ret;
> +            }
> +
>              /*
> -             * Setting compression type to BDRVQcow2State->compression_type
> -             * from the image header is going to be here
> +             * The default compression type is not allowed when the extension
> +             * is present. ZLIB is used as the default compression type.
> +             * When compression type extension header is present then
> +             * compression_type should have a value different from the 
> default.
>               */
> -             break;
> +            if (s->compression_type == QCOW2_COMPRESSION_TYPE_ZLIB) {
> +                error_setg(errp,
> +                           "compression_type_ext:"
> +                           "invalid compression type %d",
> +                           QCOW2_COMPRESSION_TYPE_ZLIB);
> +            }

This is a restriction that the spec doesn't make, so strictly speaking
this implementation wouldn't be compliant to the spec.

We can discuss whether the code or the spec should be changed. At the
moment, I don't see a good reason to make the restriction

> +#ifdef DEBUG_EXT
> +            printf("Qcow2: image compression type %s\n", 
> s->compression_type);
> +#endif
> +            break;
>  
>          case QCOW2_EXT_MAGIC_DATA_FILE:
>          {

We would save most of this code if we added a new field to the header
instead of adding a header extension. Not saying that we should
definitely do this, but let's discuss it at least.

> @@ -1492,6 +1520,9 @@ static int coroutine_fn qcow2_do_open(BlockDriverState 
> *bs, QDict *options,
>      QLIST_INIT(&s->cluster_allocs);
>      QTAILQ_INIT(&s->discards);
>  
> +    /* Set default compression type */
> +    s->compression_type = QCOW2_COMPRESSION_TYPE_ZLIB;
> +
>      /* read qcow2 extensions */
>      if (qcow2_read_extensions(bs, header.header_length, ext_end, NULL,
>                                flags, &update_header, &local_err)) {
> @@ -1500,6 +1531,34 @@ static int coroutine_fn qcow2_do_open(BlockDriverState 
> *bs, QDict *options,
>          goto fail;
>      }
>  
> +    /*
> +     * The compression type is set on the extension header processing
> +     * if the compression type extension header is present.
> +     * When the compression type is different from ZLIB (default) there
> +     * should be both the compression type bit and the compression
> +     * type extension header set. When the ZLIB (default) compression
> +     * type is used there should be neither the compression type bit nor
> +     * the compression type extension header set.
> +     */
> +
> +    if ((s->incompatible_features & QCOW2_INCOMPAT_COMPRESSION_TYPE) &
> +        (s->compression_type == QCOW2_COMPRESSION_TYPE_ZLIB)) {
> +        error_setg(errp, "Illegal compression type setting");
> +        ret = -EINVAL;
> +        goto fail;
> +    }

Almost the same restriction as above, implemented a second time.

The difference is that you get the first one only for compression type
extensions that specify zlib, and you get this one also for images that
have the compression type feature flag, but are missing the extension.

So in the current shape of the patch, this is in fact just the wrong
error message. It should be something like "compression type header
extension missing".

This restriction (if the flag is set, the header extension must be
present) actually makes sense to me. Switching to a new header field
would get rid of the whole case.

> +    /* Check available compression types */
> +    switch (s->compression_type) {
> +    case QCOW2_COMPRESSION_TYPE_ZLIB:
> +        break;
> +
> +    default:
> +        error_setg(errp, "Unknown compression type");
> +        ret = -EINVAL;
> +        goto fail;
> +    }
> +
>      /* Open external data file */
>      s->data_file = bdrv_open_child(NULL, options, "data-file", bs, 
> &child_file,
>                                     true, &local_err);
> @@ -3970,7 +4029,7 @@ fail:
>  }
>  
>  /*
> - * qcow2_compress()
> + * zlib_compress()
>   *
>   * @dest - destination buffer, @dest_size bytes
>   * @src - source buffer, @src_size bytes
> @@ -3979,7 +4038,7 @@ fail:
>   *          -1 destination buffer is not enough to store compressed data
>   *          -2 on any other error
>   */
> -static ssize_t qcow2_compress(void *dest, size_t dest_size,
> +static ssize_t zlib_compress(void *dest, size_t dest_size,
>                                const void *src, size_t src_size)

Indentation is now off in the second line.

>  {
>      ssize_t ret;
> @@ -4013,7 +4072,7 @@ static ssize_t qcow2_compress(void *dest, size_t 
> dest_size,
>  }
>  
>  /*
> - * qcow2_decompress()
> + * zlib_decompress()
>   *
>   * Decompress some data (not more than @src_size bytes) to produce exactly
>   * @dest_size bytes.

Should the description be changed, too? (qcow2_compress() doesn't have
any - should it have some?)

> @@ -4024,7 +4083,7 @@ static ssize_t qcow2_compress(void *dest, size_t 
> dest_size,
>   * Returns: 0 on success
>   *          -1 on fail
>   */
> -static ssize_t qcow2_decompress(void *dest, size_t dest_size,
> +static ssize_t zlib_decompress(void *dest, size_t dest_size,
>                                  const void *src, size_t src_size)

Indentation again.

Maybe keep the qcow2_ prefix and make it qcow2_(de)compress_zlib()?

Kevin