diff mbox

[V5,01/10] specs/qcow2: add compress format extension

Message ID 1500993699-19299-2-git-send-email-pl@kamp.de
State New
Headers show

Commit Message

Peter Lieven July 25, 2017, 2:41 p.m. UTC
Signed-off-by: Peter Lieven <pl@kamp.de>
---
 docs/interop/qcow2.txt | 51 +++++++++++++++++++++++++++++++++++++++++++++++++-
 roms/ipxe              |  2 +-
 2 files changed, 51 insertions(+), 2 deletions(-)

Comments

Eric Blake July 25, 2017, 3:03 p.m. UTC | #1
On 07/25/2017 09:41 AM, Peter Lieven wrote:
> Signed-off-by: Peter Lieven <pl@kamp.de>
> ---
>  docs/interop/qcow2.txt | 51 +++++++++++++++++++++++++++++++++++++++++++++++++-
>  roms/ipxe              |  2 +-
>  2 files changed, 51 insertions(+), 2 deletions(-)
> 

> +
> +== Compress format extension ==
> +
> +The compress format extension is an optional header extension. It provides
> +the ability to specify the compress algorithm and compress parameters

s/the compress algorithm/the compression algorithm/

> +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

Why did you name it "deflate" instead of "zlib" again?

> +
> +              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.

What must this be set to for other algorithms?  I guess we get to that
in later patches.

> +
> +                   Valid window sizes for deflate compression range from 8 to
> +                   15 inclusively.
> +
> +Note: Omitting the incompatible "Compress format bit" results in the usage
> +of deflate compression with default compression level and a window size of 12
> +(which was default before QEMU 2.11). If exactly these parameters are choosen

s/choosen/chosen,/

> +it is free to the implementation to omit the "Compress format bit" and the

s/it is free to the implementation to omit/the implementation may omit/

> +compress format extension when updating the QCOW2 header.
> +
> +
>  == Host cluster management ==
>  
>  qcow2 manages the allocation of host clusters by maintaining a reference count
> diff --git a/roms/ipxe b/roms/ipxe
> index 0600d3a..b991c67 160000
> --- a/roms/ipxe
> +++ b/roms/ipxe
> @@ -1 +1 @@
> -Subproject commit 0600d3ae94f93efd10fc6b3c7420a9557a3a1670
> +Subproject commit b991c67c1d91574ef22336cc3a5944d1e63230c9

Oops.
Peter Lieven July 25, 2017, 8:29 p.m. UTC | #2
Am 25.07.2017 um 17:03 schrieb Eric Blake:
> On 07/25/2017 09:41 AM, Peter Lieven wrote:
>> Signed-off-by: Peter Lieven <pl@kamp.de>
>> ---
>>  docs/interop/qcow2.txt | 51 +++++++++++++++++++++++++++++++++++++++++++++++++-
>>  roms/ipxe              |  2 +-
>>  2 files changed, 51 insertions(+), 2 deletions(-)
>>
>> +
>> +== Compress format extension ==
>> +
>> +The compress format extension is an optional header extension. It provides
>> +the ability to specify the compress algorithm and compress parameters
> s/the compress algorithm/the compression algorithm/
>
>> +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
> Why did you name it "deflate" instead of "zlib" again?

zlib provides raw deflate encoding and gzip encoding both with and without header afaik.
We use raw deflate encoding without header in QEMU. And the name of the algorithm is deflate.
I found it more appropriate than zlib.

>
>> +
>> +              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.
> What must this be set to for other algorithms?  I guess we get to that
> in later patches.

Its only used by the deflate algorithm. At this point we only have that algorithm.
lzo has no such encoding. If we add e.g. lzma it has a dictionary size which is
a power of two (typcial value would be 2^20).

>
>> +
>> +                   Valid window sizes for deflate compression range from 8 to
>> +                   15 inclusively.
>> +
>> +Note: Omitting the incompatible "Compress format bit" results in the usage
>> +of deflate compression with default compression level and a window size of 12
>> +(which was default before QEMU 2.11). If exactly these parameters are choosen
> s/choosen/chosen,/
>
>> +it is free to the implementation to omit the "Compress format bit" and the
> s/it is free to the implementation to omit/the implementation may omit/
>
>> +compress format extension when updating the QCOW2 header.
>> +
>> +
>>  == Host cluster management ==
>>  
>>  qcow2 manages the allocation of host clusters by maintaining a reference count
>> diff --git a/roms/ipxe b/roms/ipxe
>> index 0600d3a..b991c67 160000
>> --- a/roms/ipxe
>> +++ b/roms/ipxe
>> @@ -1 +1 @@
>> -Subproject commit 0600d3ae94f93efd10fc6b3c7420a9557a3a1670
>> +Subproject commit b991c67c1d91574ef22336cc3a5944d1e63230c9
> Oops.

Yes, oops. But I guess I have to respin anyway ;-)

Can you also have a look at the remainder of the series?

Thanks,
Peter
Eric Blake July 25, 2017, 9:01 p.m. UTC | #3
On 07/25/2017 03:29 PM, Peter Lieven wrote:

>>> +                   deflate: Standard zlib deflate compression without
>>> +                            compression header
>> Why did you name it "deflate" instead of "zlib" again?
> 
> zlib provides raw deflate encoding and gzip encoding both with and without header afaik.
> We use raw deflate encoding without header in QEMU. And the name of the algorithm is deflate.
> I found it more appropriate than zlib.

Okay, I can live with that.

> 
>>
>>> +
>>> +              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.
>> What must this be set to for other algorithms?  I guess we get to that
>> in later patches.
> 
> Its only used by the deflate algorithm. At this point we only have that algorithm.
> lzo has no such encoding. If we add e.g. lzma it has a dictionary size which is
> a power of two (typcial value would be 2^20).

So for lzo, we should document that the field must be 0 (when we get to
adding it).

> 
> Can you also have a look at the remainder of the series?

It's on my list for 2.11 reviews, although 2.10 freeze stuff is taking
priority, so it may be a few days yet.
Kevin Wolf Sept. 11, 2017, 2:22 p.m. UTC | #4
Am 25.07.2017 um 16:41 hat Peter Lieven geschrieben:
> Signed-off-by: Peter Lieven <pl@kamp.de>
> ---
>  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?

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.

Kevin
Peter Lieven Sept. 18, 2017, 10:09 a.m. UTC | #5
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 <pl@kamp.de>
>> ---
>>   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
Kevin Wolf Sept. 18, 2017, 10:50 a.m. UTC | #6
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 <pl@kamp.de>
> > > ---
> > >   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.

Kevin
Peter Lieven Dec. 13, 2017, 4:43 p.m. UTC | #7
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 <pl@kamp.de>
>>>> ---
>>>>   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
diff mbox

Patch

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.
+
+Note: Omitting the incompatible "Compress format bit" results in the usage
+of deflate compression with default compression level and a window size of 12
+(which was default before QEMU 2.11). If exactly these parameters are choosen
+it is free to the implementation to omit the "Compress format bit" and the
+compress format extension when updating the QCOW2 header.
+
+
 == Host cluster management ==
 
 qcow2 manages the allocation of host clusters by maintaining a reference count
diff --git a/roms/ipxe b/roms/ipxe
index 0600d3a..b991c67 160000
--- a/roms/ipxe
+++ b/roms/ipxe
@@ -1 +1 @@ 
-Subproject commit 0600d3ae94f93efd10fc6b3c7420a9557a3a1670
+Subproject commit b991c67c1d91574ef22336cc3a5944d1e63230c9