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

[Peter Lieven]

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 <address@hidden>
> > ---
> >   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