Re: [Qemu-devel] [PATCH 1/4] block/qcow2: add compression_algorithm create option

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3278 bytes --]

On 06/27/2017 07:34 AM, Peter Lieven wrote:
> this patch adds a new compression_algorithm option when creating qcow2 images.
> The current default for the compresison algorithm is zlib and zlib will be

s/compresison/compression/

> used when this option is omitted (like before).
> 
> If the option is specified e.g. with:
> 
>  qemu-img create -f qcow2 -o compression_algorithm=zlib image.qcow2 1G
> 
> then a new compression algorithm header extension is added and an incompatible
> feature bit is set. This means that if the header is present it must be parsed
> by Qemu on qcow2_open and it must be validated if the specified compression
> algorithm is supported by the current build of Qemu.
> 
> This means if the compression_algorithm option is specified Qemu prior to this
> commit will not be able to open the created image.
> 
> Signed-off-by: Peter Lieven <pl@kamp.de>
> ---
>  block/qcow2.c             | 93 ++++++++++++++++++++++++++++++++++++++++++++---
>  block/qcow2.h             | 20 +++++++---
>  docs/interop/qcow2.txt    |  8 +++-

Focusing on just the spec change first:

> +++ b/docs/interop/qcow2.txt
> @@ -85,7 +85,11 @@ in the description of a field.
>                                  be written to (unless for regaining
>                                  consistency).
>  
> -                    Bits 2-63:  Reserved (set to 0)
> +                    Bit 2:      Compress algorithm bit.  If this bit is set then
> +                                the compress algorithm extension must be parsed
> +                                and checked for compatiblity.

s/compatiblity/compatibility/

> +
> +                    Bits 3-63:  Reserved (set to 0)
>  
>           80 -  87:  compatible_features
>                      Bitmask of compatible features. An implementation can
> @@ -135,6 +139,8 @@ be stored. Each extension has a structure like the following:
>                          0xE2792ACA - Backing file format name
>                          0x6803f857 - Feature name table
>                          0x23852875 - Bitmaps extension
> +                        0xC0318300 - Compression Algorithm
> +                        0xC03183xx - Reserved for compression algorithm params

s/params/parameters/

You have now introduced 256 different reserved headers, without
documenting any of their formats.  You absolutely MUST include a
documentation of how the new 0xC0318300 header is laid out (see, for
example, our section on "Bitmaps extension"), along with text mentioning
that the new header MUST be present if incompatible-feature bit is set
and MUST be absent otherwise.  But I also think that with a bit of
proper design work, you only need ONE header for all possible algorithm
parameters, rather than burning an additional 255 unspecified
reservations.  That is, make sure your new header includes a common
prefix including a length field and the algorightm in use, and then the
length covers a variable-length suffix that can be parsed in a
per-algorithm-specific manner for whatever additional parameters are
needed for that algorithm.

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]