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? > >> + >> + 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. > Feels pretty limited - you don't have a length field for variable-length > extension of additional parameters, but have to fit all additions in the > next 8 bytes. Yes, all extension headers are already paired with a > length parameter outside of the struct, sent alongside the header magic > number, but embedding a length directly in the header (while redundant) > makes it easier to keep information local to the header. See > extra_data_size under Bitmap directory, for example. Of course, we may > turn those 8 bytes INTO a length field, that then describe the rest of > the variable length parameters, but why not do it up front? > > If we go with an enum mapping of supported compression formats, then you > can go into further details on exactly what extra parameters are > supports for each algorithm; while leaving it as a free-form text string > makes it harder to interpret what any additional payload will represent. >
I send a V2 of the series including the update of the spec last week. Maybe you can have a look if this version is better. Thanks, Peter