Re: [Qemu-devel] [PATCH V5 01/10] specs/qcow2: add compress format extension

[Peter Lieven]

Am 11.09.2017 um 16:22 schrieb Kevin Wolf:
> Am 25.07.2017 um 16:41 hat Peter Lieven geschrieben:
> > Signed-off-by: Peter Lieven <address@hidden>
> > ---
> >   docs/interop/qcow2.txt | 51 +++++++++++++++++++++++++++++++++++++++++++++++++-
> >   roms/ipxe              |  2 +-
> >   2 files changed, 51 insertions(+), 2 deletions(-)
> >
> > diff --git a/docs/interop/qcow2.txt b/docs/interop/qcow2.txt
> > index d7fdb1f..d0d2a8f 100644
> > --- a/docs/interop/qcow2.txt
> > +++ b/docs/interop/qcow2.txt
> > @@ -86,7 +86,12 @@ in the description of a field.
> >                                   be written to (unless for regaining
> >                                   consistency).
> >   
> > -                    Bits 2-63:  Reserved (set to 0)
> > +                    Bit 2:      Compress format bit.  If and only if this bit
> > +                                is set then the compress 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
> > @@ -137,6 +142,7 @@ be stored. Each extension has a structure like the 
> > following:
> >                           0x6803f857 - Feature name table
> >                           0x23852875 - Bitmaps extension
> >                           0x0537be77 - Full disk encryption header pointer
> > +                        0xC03183A3 - Compress format extension
> >                           other      - Unknown header extension, can be safely
> >                                        ignored
> >   
> > @@ -311,6 +317,49 @@ The algorithms used for encryption vary depending on the method
> >      in the LUKS header, with the physical disk sector as the
> >      input tweak.
> >   
> > +
> > +== Compress format extension ==
> > +
> > +The compress format extension is an optional header extension. It provides
> > +the ability to specify the compress algorithm and compress parameters
> > +that are used for compressed clusters. This new header MUST be present if
> > +the incompatible-feature bit "compress format bit" is set and MUST be absent
> > +otherwise.
> > +
> > +The fields of the compress format extension are:
> > +
> > +    Byte  0 - 13:  compress_format_name (padded with zeros, but not
> > +                   necessarily null terminated if it has full length).
> > +                   Valid compression format names currently are:
> > +
> > +                   deflate: Standard zlib deflate compression without
> > +                            compression header
> > +
> > +              14:  compress_level (uint8_t)
> > +
> > +                   0 = default compress level (valid for all formats, default)
> > +
> > +                   Additional valid compression levels for deflate compression:
> > +
> > +                   All values between 1 and 9. 1 gives best speed, 9 gives best
> > +                   compression. The default compression level is defined by 
> > zlib
> > +                   and currently defaults to 6.
> > +
> > +              15:  compress_window_size (uint8_t)
> > +
> > +                   Window or dictionary size used by the compression format.
> > +                   Currently only used by the deflate compression algorithm.
> > +
> > +                   Valid window sizes for deflate compression range from 8 to
> > +                   15 inclusively.
> So what is the plan with respect to adding new compression algorithms?
>
> If I understand correctly, we'll use the same extension type
> (0xC03183A3) and a different compress_format_name. However, the other
> algorithm will likely have different options and also a different number
> of options. Will the meaning of bytes 14-end then depend on the specific
> algorithm?

The idea of the current options is that almost every algorithm will have
a compression level setting and most have a dictionary or window
size. This is why I added them to the common header.

> Part of why I'm asking this is because I wonder why compress_format_name
> is 14 characters. It looks to me as if that is intended to make the
> header a round 16 bytes for the deflate algorithm. But unless we say
> "16 bits ought to be enough for every algorithm", this is just
> optimising a special case. (Or actually not optimising, but just moving
> the padding from the end of the header extension to padding inside the
> compress_format_name field.)
>
> Wouldn't 8 bytes still be plenty of space for a format name, and look
> more natural? Then any format that requires options would have a little
> more space without immediately going to a 24 byte header extension.

Sure 8 characters will still be enough. I can respin the series with
an updated header if you like.

Thanks,
Peter