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

[Peter Lieven <pl@kamp.de>]

Am 27.06.2017 um 14:49 schrieb Eric Blake:
> 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.
>

Before I continue, can you please give feedback on the following spec change:

diff --git a/docs/interop/qcow2.txt b/docs/interop/qcow2.txt
index 80cdfd0..f1428e9 100644
--- a/docs/interop/qcow2.txt
+++ 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:      Compression format bit.  Iff this bit is set then
+                                the compression format extension MUST be present
+                                and MUST be parsed and checked for compatibility.
+
+                    Bits 3-63:  Reserved (set to 0)

           80 -  87:  compatible_features
                      Bitmask of compatible features. An implementation can
@@ -135,6 +139,7 @@ be stored. Each extension has a structure like the following:
                          0xE2792ACA - Backing file format name
                          0x6803f857 - Feature name table
                          0x23852875 - Bitmaps extension
+                        0xC0318300 - Compression format extension
                          other      - Unknown header extension, can be safely
                                       ignored

@@ -208,6 +213,28 @@ The fields of the bitmaps extension are:
                     starts. Must be aligned to a cluster boundary.


+== Compression format extension ==
+
+The compression format extension is an optional header extension. It provides
+the ability to specify the compression algorithm and compression parameters
+that are used for compressed clusters. This new header MUST be present if
+the incompatible-feature bit "compression format bit" is set and MUST be absent
+otherwise.
+
+The fields of the compression format extension are:
+
+    Byte  0 - 15:  compression_format_name (padded with zeros, but not
+                   necessarily null terminated if it has full length)
+
+              16:  compression_level (uint8_t)
+                   0 = default compression level
+                   1 = lowest compression level
+                   x = highest compression level (the highest compression
+                       level may vary for different compression formats)
+
+         17 - 23:  Reserved for future use, must be zero.
+
+
  == Host cluster management ==

  qcow2 manages the allocation of host clusters by maintaining a reference count

Thanks,
Peter