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

[Peter Lieven]

Am 18.09.2017 um 12:50 schrieb Kevin Wolf:
> Am 18.09.2017 um 12:09 hat Peter Lieven geschrieben:
>> 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.
> It's this kind of assumptions that lead to awkward interfaces in the
> long run, because if you say "almost every" case looks like this, you
> can be sure that one day you'll get one of the remaining cases where the
> field becomes useless.
>
> Also, while most formats may support a compress_level, it is also likely
> that each of them uses a different range of values and a different
> default. So they look very similar, but are in fact different.
>
> I think this is best dealt with by treating these options as specific to
> the format, and if many formats coincide to have a field with the same
> name at the same place, so be it.
>
> If one day we finally get to a state where 'qemu-img create' options are
> expressed in the QAPI schema, I would include the fields in the
> individual branches of the union type (with a documentation what the
> exact semantics are for the specific format) rather than in the base
> type where you have to explain the semantics without actually referring
> to the branches.
>
>>> 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.
> Yes, I would prefer this.

Somehow, I forgot to respin this series. I will send a new version for 2.12

Peter

>
> Kevin