Re: [PATCH v5 2/5] qcow2: introduce compression type feature

[Max Reitz]

On 04.03.20 14:35, Denis Plotnikov wrote:
> The patch adds some preparation parts for incompatible compression type
> feature to qcow2 allowing the use different compression methods for
> image clusters (de)compressing.
> 
> It is implied that the compression type is set on the image creation and
> can be changed only later by image conversion, thus compression type
> defines the only compression algorithm used for the image, and thus,
> for all image clusters.
> 
> The goal of the feature is to add support of other compression methods
> to qcow2. For example, ZSTD which is more effective on compression than ZLIB.
> 
> The default compression is ZLIB. Images created with ZLIB compression type
> are backward compatible with older qemu versions.
> 
> Adding of the compression type breaks a number of tests because now the
> compression type is reported on image creation and there are some changes
> in the qcow2 header in size and offsets.
> 
> The tests are fixed in the following ways:
>     * filter out compression_type for all the tests
>     * fix header size, feature table size and backing file offset
>       affected tests: 031, 036, 061, 080
>       header_size +=8: 1 byte compression type
>                        7 bytes padding
>       feature_table += 48: incompatible feture compression type
>       backing_file_offset += 56 (8 + 48 -> header_change + 
> fature_table_change)
>     * add "compression type" for test output matching when it isn't filtered
>       affected tests: 049, 060, 061, 065, 144, 182, 242, 255
> 
> Signed-off-by: Denis Plotnikov <address@hidden>
> Reviewed-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> ---
>  qapi/block-core.json             |  22 ++++++-
>  block/qcow2.h                    |  18 +++++-
>  include/block/block_int.h        |   1 +
>  block/qcow2.c                    | 101 ++++++++++++++++++++++++++++++
>  tests/qemu-iotests/031.out       |  14 ++---
>  tests/qemu-iotests/036.out       |   4 +-
>  tests/qemu-iotests/049.out       | 102 +++++++++++++++----------------
>  tests/qemu-iotests/060.out       |   1 +
>  tests/qemu-iotests/061.out       |  34 ++++++-----
>  tests/qemu-iotests/065           |  28 ++++++---
>  tests/qemu-iotests/080           |   2 +-
>  tests/qemu-iotests/144.out       |   4 +-
>  tests/qemu-iotests/182.out       |   2 +-
>  tests/qemu-iotests/242.out       |   5 ++
>  tests/qemu-iotests/255.out       |   8 +--
>  tests/qemu-iotests/common.filter |   3 +-
>  16 files changed, 253 insertions(+), 96 deletions(-)

Looks good, just mostly nit picks:

> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 85e27bb61f..a67eb8bff4 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json

[...]

> @@ -4392,6 +4395,18 @@
>    'data': [ 'v2', 'v3' ] }
>  
>  
> +##
> +# @Qcow2CompressionType:

As far as I can see, all other types beginning with /@qcow2/i use the
same capitalization, so it should be the correct way.

> +#
> +# Compression type used in qcow2 image file
> +#
> +# @zlib:  zlib compression, see <http://zlib.net/>

Are the two spaces after the colon intentional?

[...]

> diff --git a/block/qcow2.h b/block/qcow2.h
> index 0942126232..485effcb70 100644
> --- a/block/qcow2.h
> +++ b/block/qcow2.h
> @@ -146,6 +146,12 @@ typedef struct QCowHeader {
>  
>      uint32_t refcount_order;
>      uint32_t header_length;
> +
> +    /* Additional fields */
> +    uint8_t  compression_type;
> +
> +    /* header must be a multiple of 8 */

I like Berto’s idea of adding a static assertion for that.

> +    uint8_t  padding[7];
>  } QEMU_PACKED QCowHeader;
>  
>  typedef struct QEMU_PACKED QCowSnapshotHeader {

[...]

> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index 6f9fd5e20e..2c6bb9dc99 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h

[...]

> @@ -2720,6 +2770,12 @@ int qcow2_update_header(BlockDriverState *bs)
>      total_size = bs->total_sectors * BDRV_SECTOR_SIZE;
>      refcount_table_clusters = s->refcount_table_size >> (s->cluster_bits - 
> 3);
>  
> +    ret = validate_compression_type(s, NULL);
> +

Why this empty line?

> +    if (ret) {
> +        goto fail;
> +    }
> +
>      *header = (QCowHeader) {
>          /* Version 2 fields */
>          .magic                  = cpu_to_be32(QCOW_MAGIC),

[...]

> @@ -5248,6 +5340,9 @@ static int qcow2_amend_options(BlockDriverState *bs, 
> QemuOpts *opts,
>                                   "images");
>                  return -EINVAL;
>              }
> +        } else if (!strcmp(desc->name, BLOCK_OPT_COMPRESSION_TYPE)) {
> +            error_setg(errp, "Changing the compression type is not 
> supported");
> +            return -ENOTSUP;

Most other branches check whether it’s really a change (unless it’d be
unreasonably complicated to do so), so maybe we should do the same here.

Max

>          } else {
>              /* if this point is reached, this probably means a new option was
>               * added without having it covered here */

signature.asc
Description: OpenPGP digital signature