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

[Peter Lieven]

Am 27.06.2017 um 17:04 schrieb Eric Blake:
> On 06/27/2017 09:49 AM, Peter Lieven wrote:
>
> > 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
> I know what this means, but spelling it "If and only if" or "When" might
> make more sense to other readers, as "Iff" is not common in English.
>
> > 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
> Now that you aren't burning 256 magic numbers, it may make sense to have
> the last two hex digits be non-zero.
>
>
> > +== Compression format extension ==
> > +
> > +The compression format extension is an optional header extension. It
> > provides
> Inline pasting created interesting wrapping, but the actual patch will
> obviously read better.
>
> > +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)
> Do we really want arbitrary names of formats, or do we want to specify
> specific algorithms (gzip, lzo, zstd) as an enum?  Which way gives us
> maximum likelihood of interoperability?

At some point I have to parse the name and convert it into a number.
This could be the same routine for the parsing of the compress.format
parameter and the compression_format_name in the header field.
The idea was that an accidently change in the enum cannot break
anything as it is local to the qemu binary. But I am ok to have an enum
if the values for each format a fixed. In this case it might be sufficient
to have an uint8_t for the compression format + an uint8_t for the compression
level. Then 2 bytes reserved + an uint32_t for the extra data size. So 
currently (the extra
data is not necessary at the moment for any format) the header would
be limited to 8 bytes.

Anyway if we glue this into a QAPI scheme, I would appreciate to get
some help with it as I am not that familiar with it yet.

Thanks,
Peter