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

[Peter Lieven]

Am 27.06.2017 um 17:11 schrieb 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.

Forgot to mention, the idea to store the name here was to be able to display
an appropiate error message if a format is not supported (like compression 
format xyz is not supported).

Peter