[PATCH v8 0/3] qcow2: add zstd cluster compression

[PATCH v8 0/3] qcow2: add zstd cluster compression

[Vladimir Sementsov-Ogievskiy]

Hi all!

Here is my proposal, about how to correctly update qcow2 specification
to introduce new field, keeping in mind currently existing images and
downstream Qemu instances.

v8: Add padding, and clarify "zero equals absence" concept.
    Move some points to commit message from the spec itself.
    Fix s/108/105 in 02

Vladimij Sementsov-Ogievskiy (3):
  docs: improve qcow2 spec about extending image header
  docs: define padding for qcow2 header
  docs: qcow2: introduce compression type feature

 docs/interop/qcow2.txt | 48 +++++++++++++++++++++++++++++++++++++++---
 1 file changed, 45 insertions(+), 3 deletions(-)

-- 
2.21.0

[PATCH v8 1/3] docs: improve qcow2 spec about extending image header

[Vladimir Sementsov-Ogievskiy]

Make it more obvious how to add new fields to the version 3 header and
how to interpret them.

The specification is adjusted so for new defined optional fields:

1. Software may support some of these optional fields and ignore the
   others, which means that features may be backported to downstream
   Qemu independently.
3. If @header_length is higher than the highest field end that software
   knows, it should assume that topmost unknown additional fields are
   correct, and keep additional unknown fields as is on rewriting the
   image.
3. If we want to add incompatible field (or a field, for which some its
   values would be incompatible), it must be accompanied by
   incompatible feature bit.

Also the concept of "default is zero" is clarified, as it's strange to
say that the value of the field is assumed to be zero for the software
version which don't know about the field at all and don't know how to
treat it be it zero or not.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 docs/interop/qcow2.txt | 26 +++++++++++++++++++++++---
 1 file changed, 23 insertions(+), 3 deletions(-)

diff --git a/docs/interop/qcow2.txt b/docs/interop/qcow2.txt
index af5711e533..4709f3bb30 100644
--- a/docs/interop/qcow2.txt
+++ b/docs/interop/qcow2.txt
@@ -79,9 +79,9 @@ The first cluster of a qcow2 image contains the file header:
                     Offset into the image file at which the snapshot table
                     starts. Must be aligned to a cluster boundary.
 
-If the version is 3 or higher, the header has the following additional fields.
-For version 2, the values are assumed to be zero, unless specified otherwise
-in the description of a field.
+For version 2, header is always 72 bytes length and finishes here.
+For version 3 or higher the header length is at least 104 bytes and has at
+least next five fields, up to the @header_length field.
 
          72 -  79:  incompatible_features
                     Bitmask of incompatible features. An implementation must
@@ -164,6 +164,26 @@ in the description of a field.
         100 - 103:  header_length
                     Length of the header structure in bytes. For version 2
                     images, the length is always assumed to be 72 bytes.
+                    For version 3 it's at least 104 bytes.
+
+Additional fields (version 3 and higher)
+
+The following fields of the header are optional: if software doesn't know how
+to interpret the field, it may be safely ignored, other than preserving the
+field unchanged when rewriting the image header.
+
+For all additional fields zero value equals to absence of field (absence is
+when field.offset + field.size > @header_length). This implies
+that if software want's to set fields up to some field not aligned to multiply
+of 8 it must align header up by zeroes. And on the other hand, if software
+need some optional field which is absent it should assume that it's value is
+zero.
+
+It's allowed for the header end to cut some field in the middle (in this case
+the field is considered as absent), but in this case the part of the field
+which is covered by @header_length must be zeroed.
+
+        < ... No additional fields in the header currently ... >
 
 Directly after the image header, optional sections called header extensions can
 be stored. Each extension has a structure like the following:
-- 
2.21.0

Re: [PATCH v8 1/3] docs: improve qcow2 spec about extending image header

[Eric Blake]

On 10/18/19 4:47 AM, Vladimir Sementsov-Ogievskiy wrote:
> Make it more obvious how to add new fields to the version 3 header and
> how to interpret them.
> 
> The specification is adjusted so for new defined optional fields:

The specification is adjusted to make it clear that future fields are 
optional:

> 
> 1. Software may support some of these optional fields and ignore the
>     others, which means that features may be backported to downstream
>     Qemu independently.
> 3. If @header_length is higher than the highest field end that software
>     knows, it should assume that topmost unknown additional fields are
>     correct, and keep additional unknown fields as is on rewriting the
>     image.
> 3. If we want to add incompatible field (or a field, for which some its
>     values would be incompatible), it must be accompanied by
>     incompatible feature bit.
> 
> Also the concept of "default is zero" is clarified, as it's strange to
> say that the value of the field is assumed to be zero for the software
> version which don't know about the field at all and don't know how to
> treat it be it zero or not.
> 

I'd also mention that we want to enforce 8-byte alignment in this cover 
letter.

> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>   docs/interop/qcow2.txt | 26 +++++++++++++++++++++++---
>   1 file changed, 23 insertions(+), 3 deletions(-)
> 
> diff --git a/docs/interop/qcow2.txt b/docs/interop/qcow2.txt
> index af5711e533..4709f3bb30 100644
> --- a/docs/interop/qcow2.txt
> +++ b/docs/interop/qcow2.txt
> @@ -79,9 +79,9 @@ The first cluster of a qcow2 image contains the file header:
>                       Offset into the image file at which the snapshot table
>                       starts. Must be aligned to a cluster boundary.
>   
> -If the version is 3 or higher, the header has the following additional fields.
> -For version 2, the values are assumed to be zero, unless specified otherwise
> -in the description of a field.
> +For version 2, header is always 72 bytes length and finishes here.
> +For version 3 or higher the header length is at least 104 bytes and has at
> +least next five fields, up to the @header_length field.

For version 2, the header is exactly 72 bytes in length, and finishes here.
For version 3 or higher, the header length is at least 104 bytes, 
including the next fields through header_length.

>   
>            72 -  79:  incompatible_features
>                       Bitmask of incompatible features. An implementation must
> @@ -164,6 +164,26 @@ in the description of a field.
>           100 - 103:  header_length
>                       Length of the header structure in bytes. For version 2
>                       images, the length is always assumed to be 72 bytes.
> +                    For version 3 it's at least 104 bytes.

I'd also add a sentence that this field must be a multiple of 8.

> +
> +Additional fields (version 3 and higher)
> +
> +The following fields of the header are optional: if software doesn't know how
> +to interpret the field, it may be safely ignored, other than preserving the
> +field unchanged when rewriting the image header.

Maybe:

if software doesn't know how to interpret the field, it may be safely 
ignored unless a corresponding incompatible feature flag bit is set; 
however, the field should be preserved unchanged when rewriting the 
image header.

> +
> +For all additional fields zero value equals to absence of field (absence is
> +when field.offset + field.size > @header_length). This implies
> +that if software want's to set fields up to some field not aligned to multiply
> +of 8 it must align header up by zeroes. And on the other hand, if software
> +need some optional field which is absent it should assume that it's value is
> +zero.

Maybe:

Each optional field that does not have a corresponding incompatible 
feature bit must support the value 0 that gives the same default 
behavior as when the optional field is omitted.

> +
> +It's allowed for the header end to cut some field in the middle (in this case
> +the field is considered as absent), but in this case the part of the field
> +which is covered by @header_length must be zeroed.
> +
> +        < ... No additional fields in the header currently ... >

Do we even still need this if we require 8-byte alignment?  We'd never 
be able to cut a single field in the middle, but I suppose you are 
worried about cutting a 2-field 16-byte addition tied to a single 
feature in the middle.  But that's not going to happen in practice.  The 
only time the header will be longer than 104 bytes is if at least one 
documented optional feature has been implemented/backported, and that 
feature will be implemented in its entirety.  If you backport a later 
feature but not the earlier, you're still going to set header_length to 
the boundary of the feature that you ARE backporting.  Thus, I argue 
that blindly setting header_length to 120 prior to the standard ever 
defining optional field(s) at 112-120 is premature, and that if we ever 
add a feature requiring bytes 112-128 for a new feature, you will never 
see a valid qcow2 file with a header length of 120.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH v8 1/3] docs: improve qcow2 spec about extending image header

[Vladimir Sementsov-Ogievskiy]

18.10.2019 17:00, Eric Blake wrote:
> On 10/18/19 4:47 AM, Vladimir Sementsov-Ogievskiy wrote:
>> Make it more obvious how to add new fields to the version 3 header and
>> how to interpret them.
>>
>> The specification is adjusted so for new defined optional fields:
> 
> The specification is adjusted to make it clear that future fields are optional:
> 
>>
>> 1. Software may support some of these optional fields and ignore the
>>     others, which means that features may be backported to downstream
>>     Qemu independently.
>> 3. If @header_length is higher than the highest field end that software
>>     knows, it should assume that topmost unknown additional fields are
>>     correct, and keep additional unknown fields as is on rewriting the
>>     image.
>> 3. If we want to add incompatible field (or a field, for which some its
>>     values would be incompatible), it must be accompanied by
>>     incompatible feature bit.
>>
>> Also the concept of "default is zero" is clarified, as it's strange to
>> say that the value of the field is assumed to be zero for the software
>> version which don't know about the field at all and don't know how to
>> treat it be it zero or not.
>>
> 
> I'd also mention that we want to enforce 8-byte alignment in this cover letter.
> 
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   docs/interop/qcow2.txt | 26 +++++++++++++++++++++++---
>>   1 file changed, 23 insertions(+), 3 deletions(-)
>>
>> diff --git a/docs/interop/qcow2.txt b/docs/interop/qcow2.txt
>> index af5711e533..4709f3bb30 100644
>> --- a/docs/interop/qcow2.txt
>> +++ b/docs/interop/qcow2.txt
>> @@ -79,9 +79,9 @@ The first cluster of a qcow2 image contains the file header:
>>                       Offset into the image file at which the snapshot table
>>                       starts. Must be aligned to a cluster boundary.
>> -If the version is 3 or higher, the header has the following additional fields.
>> -For version 2, the values are assumed to be zero, unless specified otherwise
>> -in the description of a field.
>> +For version 2, header is always 72 bytes length and finishes here.
>> +For version 3 or higher the header length is at least 104 bytes and has at
>> +least next five fields, up to the @header_length field.
> 
> For version 2, the header is exactly 72 bytes in length, and finishes here.
> For version 3 or higher, the header length is at least 104 bytes, including the next fields through header_length.
> 
>>            72 -  79:  incompatible_features
>>                       Bitmask of incompatible features. An implementation must
>> @@ -164,6 +164,26 @@ in the description of a field.
>>           100 - 103:  header_length
>>                       Length of the header structure in bytes. For version 2
>>                       images, the length is always assumed to be 72 bytes.
>> +                    For version 3 it's at least 104 bytes.
> 
> I'd also add a sentence that this field must be a multiple of 8.
> 
>> +
>> +Additional fields (version 3 and higher)
>> +
>> +The following fields of the header are optional: if software doesn't know how
>> +to interpret the field, it may be safely ignored, other than preserving the
>> +field unchanged when rewriting the image header.
> 
> Maybe:
> 
> if software doesn't know how to interpret the field, it may be safely ignored unless a corresponding incompatible feature flag bit is set; however, the field should be preserved unchanged when rewriting the image header.
> 
>> +
>> +For all additional fields zero value equals to absence of field (absence is
>> +when field.offset + field.size > @header_length). This implies
>> +that if software want's to set fields up to some field not aligned to multiply
>> +of 8 it must align header up by zeroes. And on the other hand, if software
>> +need some optional field which is absent it should assume that it's value is
>> +zero.
> 
> Maybe:
> 
> Each optional field that does not have a corresponding incompatible feature bit must support the value 0 that gives the same default behavior as when the optional field is omitted.

Hmmm. That doesn't work, as "corresponding" is something not actually defined. Consider our zstd extension.

It has corresponding incompatible bit, therefore, this sentence doesn't apply to it. But still, if incompatible bit is unset we can have this field. And it's zero value must correspond
to the absence of the field.

So, additional field may use incomaptible bit only for subset of its values.

But, I see, that you want to allow 0 value to not match field-absence if incompatible bit is set?

So, may be

Additional fields has the following compatible behavior by default:

1. If software doesn't know how to interpret the field, it may be safely ignored, other than preserving the field unchanged when rewriting the image header.
2. Zeroed additional field gives the same behavior as when this field is omitted.

This default behavior may be altered with help of incompatible feature bits. So, if, for example, additional field has corresponding incompatible feature bit, and it is set, we are sure that software which opens the image knows how to interpret the field, so,
1. The field definitely will not be ignored when corresponding incompatible bit is set.
2. The field may define any meaning it wants for zero value for the case when corresponding incompatible bit is set.

> 
>> +
>> +It's allowed for the header end to cut some field in the middle (in this case
>> +the field is considered as absent), but in this case the part of the field
>> +which is covered by @header_length must be zeroed.
>> +
>> +        < ... No additional fields in the header currently ... >
> 
> Do we even still need this if we require 8-byte alignment?  We'd never be able to cut a single field in the middle

hmm, for example:
105: compression byte
106-113: some other 8-bytes field, unalinged
113-119: padding to multiply of 8

- bad example, for sure. But to prevent it, we should also define some field alignment requirements..


>, but I suppose you are worried about cutting a 2-field 16-byte addition tied to a single feature in the middle.

and this too.

>  But that's not going to happen in practice.

why not?

4 bytes: feature 1

4 bytes: feature 2
8 bytes: feature 2

so, last 12 bytes may be considered as one field.. And software which don't know about feature2, will pad header to the middle of feature2

> The only time the header will be longer than 104 bytes is if at least one documented optional feature has been implemented/backported, and that feature will be implemented in its entirety.  If you backport a later feature but not the earlier, you're still going to set header_length to the boundary of the feature that you ARE backporting.

That's true, of course.

>  Thus, I argue that blindly setting header_length to 120 prior to the standard ever defining optional field(s) at 112-120 is premature, and that if we ever add a feature requiring bytes 112-128 for a new feature, you will never see a valid qcow2 file with a header length of 120.

consider my example above.



-- 
Best regards,
Vladimir

Re: [PATCH v8 1/3] docs: improve qcow2 spec about extending image header

[Vladimir Sementsov-Ogievskiy]

18.10.2019 17:36, Vladimir Sementsov-Ogievskiy wrote:
> 18.10.2019 17:00, Eric Blake wrote:
>> On 10/18/19 4:47 AM, Vladimir Sementsov-Ogievskiy wrote:
>>> Make it more obvious how to add new fields to the version 3 header and
>>> how to interpret them.
>>>
>>> The specification is adjusted so for new defined optional fields:
>>
>> The specification is adjusted to make it clear that future fields are optional:
>>
>>>
>>> 1. Software may support some of these optional fields and ignore the
>>>     others, which means that features may be backported to downstream
>>>     Qemu independently.
>>> 3. If @header_length is higher than the highest field end that software
>>>     knows, it should assume that topmost unknown additional fields are
>>>     correct, and keep additional unknown fields as is on rewriting the
>>>     image.
>>> 3. If we want to add incompatible field (or a field, for which some its
>>>     values would be incompatible), it must be accompanied by
>>>     incompatible feature bit.
>>>
>>> Also the concept of "default is zero" is clarified, as it's strange to
>>> say that the value of the field is assumed to be zero for the software
>>> version which don't know about the field at all and don't know how to
>>> treat it be it zero or not.
>>>
>>
>> I'd also mention that we want to enforce 8-byte alignment in this cover letter.
>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>> ---
>>>   docs/interop/qcow2.txt | 26 +++++++++++++++++++++++---
>>>   1 file changed, 23 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/docs/interop/qcow2.txt b/docs/interop/qcow2.txt
>>> index af5711e533..4709f3bb30 100644
>>> --- a/docs/interop/qcow2.txt
>>> +++ b/docs/interop/qcow2.txt
>>> @@ -79,9 +79,9 @@ The first cluster of a qcow2 image contains the file header:
>>>                       Offset into the image file at which the snapshot table
>>>                       starts. Must be aligned to a cluster boundary.
>>> -If the version is 3 or higher, the header has the following additional fields.
>>> -For version 2, the values are assumed to be zero, unless specified otherwise
>>> -in the description of a field.
>>> +For version 2, header is always 72 bytes length and finishes here.
>>> +For version 3 or higher the header length is at least 104 bytes and has at
>>> +least next five fields, up to the @header_length field.
>>
>> For version 2, the header is exactly 72 bytes in length, and finishes here.
>> For version 3 or higher, the header length is at least 104 bytes, including the next fields through header_length.
>>
>>>            72 -  79:  incompatible_features
>>>                       Bitmask of incompatible features. An implementation must
>>> @@ -164,6 +164,26 @@ in the description of a field.
>>>           100 - 103:  header_length
>>>                       Length of the header structure in bytes. For version 2
>>>                       images, the length is always assumed to be 72 bytes.
>>> +                    For version 3 it's at least 104 bytes.
>>
>> I'd also add a sentence that this field must be a multiple of 8.
>>
>>> +
>>> +Additional fields (version 3 and higher)
>>> +
>>> +The following fields of the header are optional: if software doesn't know how
>>> +to interpret the field, it may be safely ignored, other than preserving the
>>> +field unchanged when rewriting the image header.
>>
>> Maybe:
>>
>> if software doesn't know how to interpret the field, it may be safely ignored unless a corresponding incompatible feature flag bit is set; however, the field should be preserved unchanged when rewriting the image header.
>>
>>> +
>>> +For all additional fields zero value equals to absence of field (absence is
>>> +when field.offset + field.size > @header_length). This implies
>>> +that if software want's to set fields up to some field not aligned to multiply
>>> +of 8 it must align header up by zeroes. And on the other hand, if software
>>> +need some optional field which is absent it should assume that it's value is
>>> +zero.
>>
>> Maybe:
>>
>> Each optional field that does not have a corresponding incompatible feature bit must support the value 0 that gives the same default behavior as when the optional field is omitted.
> 
> Hmmm. That doesn't work, as "corresponding" is something not actually defined. Consider our zstd extension.
> 
> It has corresponding incompatible bit, therefore, this sentence doesn't apply to it. But still, if incompatible bit is unset we can have this field. And it's zero value must correspond
> to the absence of the field.
> 
> So, additional field may use incomaptible bit only for subset of its values.
> 
> But, I see, that you want to allow 0 value to not match field-absence if incompatible bit is set?
> 
> So, may be
> 
> Additional fields has the following compatible behavior by default:
> 
> 1. If software doesn't know how to interpret the field, it may be safely ignored, other than preserving the field unchanged when rewriting the image header.
> 2. Zeroed additional field gives the same behavior as when this field is omitted.
> 
> This default behavior may be altered with help of incompatible feature bits. So, if, for example, additional field has corresponding incompatible feature bit, and it is set, we are sure that software which opens the image knows how to interpret the field, so,
> 1. The field definitely will not be ignored when corresponding incompatible bit is set.
> 2. The field may define any meaning it wants for zero value for the case when corresponding incompatible bit is set.
> 
>>
>>> +
>>> +It's allowed for the header end to cut some field in the middle (in this case
>>> +the field is considered as absent), but in this case the part of the field
>>> +which is covered by @header_length must be zeroed.
>>> +
>>> +        < ... No additional fields in the header currently ... >
>>
>> Do we even still need this if we require 8-byte alignment?  We'd never be able to cut a single field in the middle
> 
> hmm, for example:
> 105: compression byte
> 106-113: some other 8-bytes field, unalinged
> 113-119: padding to multiply of 8
> 
> - bad example, for sure. But to prevent it, we should also define some field alignment requirements..
> 
> 
>> , but I suppose you are worried about cutting a 2-field 16-byte addition tied to a single feature in the middle.
> 
> and this too.
> 
>>   But that's not going to happen in practice.
> 
> why not?
> 
> 4 bytes: feature 1
> 
> 4 bytes: feature 2
> 8 bytes: feature 2
> 
> so, last 12 bytes may be considered as one field.. And software which don't know about feature2, will pad header to the middle of feature2
> 
>> The only time the header will be longer than 104 bytes is if at least one documented optional feature has been implemented/backported, and that feature will be implemented in its entirety.  If you backport a later feature but not the earlier, you're still going to set header_length to the boundary of the feature that you ARE backporting.
> 
> That's true, of course.
> 
>>   Thus, I argue that blindly setting header_length to 120 prior to the standard ever defining optional field(s) at 112-120 is premature, and that if we ever add a feature requiring bytes 112-128 for a new feature, you will never see a valid qcow2 file with a header length of 120.
> 
> consider my example above.
> 
> 
> 

Eric what do you think?

-- 
Best regards,
Vladimir

Re: [PATCH v8 1/3] docs: improve qcow2 spec about extending image header

[Eric Blake]

On 10/18/19 9:36 AM, Vladimir Sementsov-Ogievskiy wrote:

>> Maybe:
>>
>> if software doesn't know how to interpret the field, it may be safely ignored unless a corresponding incompatible feature flag bit is set; however, the field should be preserved unchanged when rewriting the image header.
>>
>>> +
>>> +For all additional fields zero value equals to absence of field (absence is
>>> +when field.offset + field.size > @header_length). This implies
>>> +that if software want's to set fields up to some field not aligned to multiply
>>> +of 8 it must align header up by zeroes. And on the other hand, if software
>>> +need some optional field which is absent it should assume that it's value is
>>> +zero.
>>
>> Maybe:
>>
>> Each optional field that does not have a corresponding incompatible feature bit must support the value 0 that gives the same default behavior as when the optional field is omitted.
> 
> Hmmm. That doesn't work, as "corresponding" is something not actually defined. Consider our zstd extension.
> 
> It has corresponding incompatible bit, therefore, this sentence doesn't apply to it. But still, if incompatible bit is unset we can have this field. And it's zero value must correspond
> to the absence of the field.
> 
> So, additional field may use incomaptible bit only for subset of its values.
> 
> But, I see, that you want to allow 0 value to not match field-absence if incompatible bit is set?

Not necessarily.  Rather, if the value of an unknown field can be safely 
ignored, then it should default to 0.  If it cannot be safely ignored, 
then that field will not be set to a non-zero value without also setting 
an incompatible feature flag, so that software that does not know how to 
interpret the field will fail to load the image because it also fails to 
recognize the associated new incompatible feature bit.

But I'd really like Kevin's opinion on how much wording is worth adding.

> 
> So, may be
> 
> Additional fields has the following compatible behavior by default:

s/has/have/

> 
> 1. If software doesn't know how to interpret the field, it may be safely ignored, other than preserving the field unchanged when rewriting the image header.
> 2. Zeroed additional field gives the same behavior as when this field is omitted.

Both good.

> 
> This default behavior may be altered with help of incompatible feature bits. So, if, for example, additional field has corresponding incompatible feature bit, and it is set, we are sure that software which opens the image knows how to interpret the field, so,
> 1. The field definitely will not be ignored when corresponding incompatible bit is set.
> 2. The field may define any meaning it wants for zero value for the case when corresponding incompatible bit is set.

Rather wordy but seems accurate.  Perhaps compress it to:

3. Any additional field whose value must not be ignored for correct 
handling of the file will be accompanied by a corresponding incompatible 
feature bit.

and maybe even reorder it to list the points as:

Additional fields have the following compatibility rules:
1. Any additional field whose value must not be ignored for correct 
handling of the file will be accompanied by a corresponding incompatible 
feature bit.
2. If there are no unrecognized incompatible feature bits set, an 
additional field may be safely ignored other than preserving its value 
when rewriting the image header.
3. An explicit value of 0 will have the same behavior as when the field 
is not present.


>>> +It's allowed for the header end to cut some field in the middle (in this case
>>> +the field is considered as absent), but in this case the part of the field
>>> +which is covered by @header_length must be zeroed.
>>> +
>>> +        < ... No additional fields in the header currently ... >
>>
>> Do we even still need this if we require 8-byte alignment?  We'd never be able to cut a single field in the middle
> 
> hmm, for example:
> 105: compression byte
> 106-113: some other 8-bytes field, unalinged
> 113-119: padding to multiply of 8
> 
> - bad example, for sure. But to prevent it, we should also define some field alignment requirements..
> 
> 
>> , but I suppose you are worried about cutting a 2-field 16-byte addition tied to a single feature in the middle.
> 
> and this too.
> 
>>    But that's not going to happen in practice.
> 
> why not?
> 
> 4 bytes: feature 1
> 
> 4 bytes: feature 2
> 8 bytes: feature 2
> 
> so, last 12 bytes may be considered as one field.. And software which don't know about feature2, will pad header to the middle of feature2
> 
>> The only time the header will be longer than 104 bytes is if at least one documented optional feature has been implemented/backported, and that feature will be implemented in its entirety.  If you backport a later feature but not the earlier, you're still going to set header_length to the boundary of the feature that you ARE backporting.
> 
> That's true, of course.
> 
>>    Thus, I argue that blindly setting header_length to 120 prior to the standard ever defining optional field(s) at 112-120 is premature, and that if we ever add a feature requiring bytes 112-128 for a new feature, you will never see a valid qcow2 file with a header length of 120.
> 
> consider my example above.

As long as we never add new fields that are not 8-byte aligned 
(including any explicit padding), then we will never have the case of 
dividing fields in the middle by keeping the header length a multiple of 8.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH v8 1/3] docs: improve qcow2 spec about extending image header

[Vladimir Sementsov-Ogievskiy]

06.11.2019 22:19, Eric Blake wrote:
> On 10/18/19 9:36 AM, Vladimir Sementsov-Ogievskiy wrote:
> 
>>> Maybe:
>>>
>>> if software doesn't know how to interpret the field, it may be safely ignored unless a corresponding incompatible feature flag bit is set; however, the field should be preserved unchanged when rewriting the image header.
>>>
>>>> +
>>>> +For all additional fields zero value equals to absence of field (absence is
>>>> +when field.offset + field.size > @header_length). This implies
>>>> +that if software want's to set fields up to some field not aligned to multiply
>>>> +of 8 it must align header up by zeroes. And on the other hand, if software
>>>> +need some optional field which is absent it should assume that it's value is
>>>> +zero.
>>>
>>> Maybe:
>>>
>>> Each optional field that does not have a corresponding incompatible feature bit must support the value 0 that gives the same default behavior as when the optional field is omitted.
>>
>> Hmmm. That doesn't work, as "corresponding" is something not actually defined. Consider our zstd extension.
>>
>> It has corresponding incompatible bit, therefore, this sentence doesn't apply to it. But still, if incompatible bit is unset we can have this field. And it's zero value must correspond
>> to the absence of the field.
>>
>> So, additional field may use incomaptible bit only for subset of its values.
>>
>> But, I see, that you want to allow 0 value to not match field-absence if incompatible bit is set?
> 
> Not necessarily.  Rather, if the value of an unknown field can be safely ignored, then it should default to 0.  If it cannot be safely ignored, then that field will not be set to a non-zero value without also setting an incompatible feature flag, so that software that does not know how to interpret the field will fail to load the image because it also fails to recognize the associated new incompatible feature bit.
> 
> But I'd really like Kevin's opinion on how much wording is worth adding.
> 
>>
>> So, may be
>>
>> Additional fields has the following compatible behavior by default:
> 
> s/has/have/
> 
>>
>> 1. If software doesn't know how to interpret the field, it may be safely ignored, other than preserving the field unchanged when rewriting the image header.
>> 2. Zeroed additional field gives the same behavior as when this field is omitted.
> 
> Both good.
> 
>>
>> This default behavior may be altered with help of incompatible feature bits. So, if, for example, additional field has corresponding incompatible feature bit, and it is set, we are sure that software which opens the image knows how to interpret the field, so,
>> 1. The field definitely will not be ignored when corresponding incompatible bit is set.
>> 2. The field may define any meaning it wants for zero value for the case when corresponding incompatible bit is set.
> 
> Rather wordy but seems accurate.  Perhaps compress it to:
> 
> 3. Any additional field whose value must not be ignored for correct handling of the file will be accompanied by a corresponding incompatible feature bit.
> 
> and maybe even reorder it to list the points as:
> 
> Additional fields have the following compatibility rules:
> 1. Any additional field whose value must not be ignored for correct handling of the file will be accompanied by a corresponding incompatible feature bit.

I'd like to stress, that incompatible bit is needed for incompatible value, not for the field itself. (So field may be accompanied by incompatible bit for some
it's values and for others - not), So, what about

1. If the value of the additional field must not be ignored for correct handling of the file, it will be accompanied by a corresponding incompatible feature bit.

> 2. If there are no unrecognized incompatible feature bits set, an additional field may be safely ignored other than preserving its value when rewriting the image header.

Sounds like we can ignore the field if we know its meaning and know its incompatible bit..

2. If there are no unrecognized incompatible feature bits set, an unknown additional field may be safely ignored other than preserving its value when rewriting the image header.

> 3. An explicit value of 0 will have the same behavior as when the field is not present.

But it may be changed by incompatible bit..

3. An explicit value of 0 will have the same behavior as when the field is not present, if not altered by specific incompatible bit.

> 
> 
>>>> +It's allowed for the header end to cut some field in the middle (in this case
>>>> +the field is considered as absent), but in this case the part of the field
>>>> +which is covered by @header_length must be zeroed.
>>>> +
>>>> +        < ... No additional fields in the header currently ... >
>>>
>>> Do we even still need this if we require 8-byte alignment?  We'd never be able to cut a single field in the middle
>>
>> hmm, for example:
>> 105: compression byte
>> 106-113: some other 8-bytes field, unalinged
>> 113-119: padding to multiply of 8
>>
>> - bad example, for sure. But to prevent it, we should also define some field alignment requirements..
>>
>>
>>> , but I suppose you are worried about cutting a 2-field 16-byte addition tied to a single feature in the middle.
>>
>> and this too.
>>
>>>    But that's not going to happen in practice.
>>
>> why not?
>>
>> 4 bytes: feature 1
>>
>> 4 bytes: feature 2
>> 8 bytes: feature 2
>>
>> so, last 12 bytes may be considered as one field.. And software which don't know about feature2, will pad header to the middle of feature2
>>
>>> The only time the header will be longer than 104 bytes is if at least one documented optional feature has been implemented/backported, and that feature will be implemented in its entirety.  If you backport a later feature but not the earlier, you're still going to set header_length to the boundary of the feature that you ARE backporting.
>>
>> That's true, of course.
>>
>>>    Thus, I argue that blindly setting header_length to 120 prior to the standard ever defining optional field(s) at 112-120 is premature, and that if we ever add a feature requiring bytes 112-128 for a new feature, you will never see a valid qcow2 file with a header length of 120.
>>
>> consider my example above.
> 
> As long as we never add new fields that are not 8-byte aligned (including any explicit padding), then we will never have the case of dividing fields in the middle by keeping the header length a multiple of 8.
> 

OK.

-- 
Best regards,
Vladimir

Re: [PATCH v8 1/3] docs: improve qcow2 spec about extending image header

[Vladimir Sementsov-Ogievskiy]

07.11.2019 15:26, Vladimir Sementsov-Ogievskiy wrote:
> 06.11.2019 22:19, Eric Blake wrote:
>> On 10/18/19 9:36 AM, Vladimir Sementsov-Ogievskiy wrote:
>>
>>>> Maybe:
>>>>
>>>> if software doesn't know how to interpret the field, it may be safely ignored unless a corresponding incompatible feature flag bit is set; however, the field should be preserved unchanged when rewriting the image header.
>>>>
>>>>> +
>>>>> +For all additional fields zero value equals to absence of field (absence is
>>>>> +when field.offset + field.size > @header_length). This implies
>>>>> +that if software want's to set fields up to some field not aligned to multiply
>>>>> +of 8 it must align header up by zeroes. And on the other hand, if software
>>>>> +need some optional field which is absent it should assume that it's value is
>>>>> +zero.
>>>>
>>>> Maybe:
>>>>
>>>> Each optional field that does not have a corresponding incompatible feature bit must support the value 0 that gives the same default behavior as when the optional field is omitted.
>>>
>>> Hmmm. That doesn't work, as "corresponding" is something not actually defined. Consider our zstd extension.
>>>
>>> It has corresponding incompatible bit, therefore, this sentence doesn't apply to it. But still, if incompatible bit is unset we can have this field. And it's zero value must correspond
>>> to the absence of the field.
>>>
>>> So, additional field may use incomaptible bit only for subset of its values.
>>>
>>> But, I see, that you want to allow 0 value to not match field-absence if incompatible bit is set?
>>
>> Not necessarily.  Rather, if the value of an unknown field can be safely ignored, then it should default to 0.  If it cannot be safely ignored, then that field will not be set to a non-zero value without also setting an incompatible feature flag, so that software that does not know how to interpret the field will fail to load the image because it also fails to recognize the associated new incompatible feature bit.
>>
>> But I'd really like Kevin's opinion on how much wording is worth adding.
>>
>>>
>>> So, may be
>>>
>>> Additional fields has the following compatible behavior by default:
>>
>> s/has/have/
>>
>>>
>>> 1. If software doesn't know how to interpret the field, it may be safely ignored, other than preserving the field unchanged when rewriting the image header.
>>> 2. Zeroed additional field gives the same behavior as when this field is omitted.
>>
>> Both good.
>>
>>>
>>> This default behavior may be altered with help of incompatible feature bits. So, if, for example, additional field has corresponding incompatible feature bit, and it is set, we are sure that software which opens the image knows how to interpret the field, so,
>>> 1. The field definitely will not be ignored when corresponding incompatible bit is set.
>>> 2. The field may define any meaning it wants for zero value for the case when corresponding incompatible bit is set.
>>
>> Rather wordy but seems accurate.  Perhaps compress it to:
>>
>> 3. Any additional field whose value must not be ignored for correct handling of the file will be accompanied by a corresponding incompatible feature bit.
>>
>> and maybe even reorder it to list the points as:
>>
>> Additional fields have the following compatibility rules:
>> 1. Any additional field whose value must not be ignored for correct handling of the file will be accompanied by a corresponding incompatible feature bit.
> 
> I'd like to stress, that incompatible bit is needed for incompatible value, not for the field itself. (So field may be accompanied by incompatible bit for some
> it's values and for others - not), So, what about
> 
> 1. If the value of the additional field must not be ignored for correct handling of the file, it will be accompanied by a corresponding incompatible feature bit.
> 
>> 2. If there are no unrecognized incompatible feature bits set, an additional field may be safely ignored other than preserving its value when rewriting the image header.
> 
> Sounds like we can ignore the field if we know its meaning and know its incompatible bit..
> 
> 2. If there are no unrecognized incompatible feature bits set, an unknown additional field may be safely ignored other than preserving its value when rewriting the image header.
> 
>> 3. An explicit value of 0 will have the same behavior as when the field is not present.
> 
> But it may be changed by incompatible bit..
> 
> 3. An explicit value of 0 will have the same behavior as when the field is not present, if not altered by specific incompatible bit.
> 

Eric, OK for you?

>>
>>
>>>>> +It's allowed for the header end to cut some field in the middle (in this case
>>>>> +the field is considered as absent), but in this case the part of the field
>>>>> +which is covered by @header_length must be zeroed.
>>>>> +
>>>>> +        < ... No additional fields in the header currently ... >
>>>>
>>>> Do we even still need this if we require 8-byte alignment?  We'd never be able to cut a single field in the middle
>>>
>>> hmm, for example:
>>> 105: compression byte
>>> 106-113: some other 8-bytes field, unalinged
>>> 113-119: padding to multiply of 8
>>>
>>> - bad example, for sure. But to prevent it, we should also define some field alignment requirements..
>>>
>>>
>>>> , but I suppose you are worried about cutting a 2-field 16-byte addition tied to a single feature in the middle.
>>>
>>> and this too.
>>>
>>>>    But that's not going to happen in practice.
>>>
>>> why not?
>>>
>>> 4 bytes: feature 1
>>>
>>> 4 bytes: feature 2
>>> 8 bytes: feature 2
>>>
>>> so, last 12 bytes may be considered as one field.. And software which don't know about feature2, will pad header to the middle of feature2
>>>
>>>> The only time the header will be longer than 104 bytes is if at least one documented optional feature has been implemented/backported, and that feature will be implemented in its entirety.  If you backport a later feature but not the earlier, you're still going to set header_length to the boundary of the feature that you ARE backporting.
>>>
>>> That's true, of course.
>>>
>>>>    Thus, I argue that blindly setting header_length to 120 prior to the standard ever defining optional field(s) at 112-120 is premature, and that if we ever add a feature requiring bytes 112-128 for a new feature, you will never see a valid qcow2 file with a header length of 120.
>>>
>>> consider my example above.
>>
>> As long as we never add new fields that are not 8-byte aligned (including any explicit padding), then we will never have the case of dividing fields in the middle by keeping the header length a multiple of 8.
>>
> 
> OK.
> 


-- 
Best regards,
Vladimir

[PATCH v8 2/3] docs: define padding for qcow2 header

[Vladimir Sementsov-Ogievskiy]

Header extensions ends are already defined to be multiply of 8. Let's
gently ask for header length to be a multiply of 8 too, when we have
some additional fields. Requiring this may be considered as an
incompatible change, so the padding is optional. Actually, padding is
allowed before this patch (due to definition of additional fields),
the only actual change is "SHOULD" word.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 docs/interop/qcow2.txt | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/docs/interop/qcow2.txt b/docs/interop/qcow2.txt
index 4709f3bb30..b971e59b1a 100644
--- a/docs/interop/qcow2.txt
+++ b/docs/interop/qcow2.txt
@@ -185,6 +185,11 @@ which is covered by @header_length must be zeroed.
 
         < ... No additional fields in the header currently ... >
 
+Header padding
+        If @header_length is larger than 104, software SHOULD make it a
+        multiply of 8, adding zero-padding after additional fields. Still the
+        padding is optional and may be absent in the image.
+
 Directly after the image header, optional sections called header extensions can
 be stored. Each extension has a structure like the following:
 
-- 
2.21.0

Re: [PATCH v8 2/3] docs: define padding for qcow2 header

[Eric Blake]

On 10/18/19 4:47 AM, Vladimir Sementsov-Ogievskiy wrote:
> Header extensions ends are already defined to be multiply of 8. Let's
> gently ask for header length to be a multiply of 8 too, when we have
> some additional fields. Requiring this may be considered as an
> incompatible change, so the padding is optional. Actually, padding is
> allowed before this patch (due to definition of additional fields),
> the only actual change is "SHOULD" word.

Too weak. I've already argued that this should be mandatory, and that we 
are not breaking backwards compatibility, but merely clarifying what has 
already been implicit by the fact that header extensions are required to 
be 8-byte size multiple (which makes no sense unless they are also 
8-byte aligned).

> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>   docs/interop/qcow2.txt | 5 +++++
>   1 file changed, 5 insertions(+)
> 
> diff --git a/docs/interop/qcow2.txt b/docs/interop/qcow2.txt
> index 4709f3bb30..b971e59b1a 100644
> --- a/docs/interop/qcow2.txt
> +++ b/docs/interop/qcow2.txt
> @@ -185,6 +185,11 @@ which is covered by @header_length must be zeroed.
>   
>           < ... No additional fields in the header currently ... >
>   
> +Header padding
> +        If @header_length is larger than 104, software SHOULD make it a
> +        multiply of 8, adding zero-padding after additional fields. Still the
> +        padding is optional and may be absent in the image.
> +
>   Directly after the image header, optional sections called header extensions can
>   be stored. Each extension has a structure like the following:
>   
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH v8 2/3] docs: define padding for qcow2 header

[Vladimir Sementsov-Ogievskiy]

18.10.2019 17:02, Eric Blake wrote:
> On 10/18/19 4:47 AM, Vladimir Sementsov-Ogievskiy wrote:
>> Header extensions ends are already defined to be multiply of 8. Let's
>> gently ask for header length to be a multiply of 8 too, when we have
>> some additional fields. Requiring this may be considered as an
>> incompatible change, so the padding is optional. Actually, padding is
>> allowed before this patch (due to definition of additional fields),
>> the only actual change is "SHOULD" word.
> 
> Too weak. I've already argued that this should be mandatory, and that we are not breaking backwards compatibility, but merely clarifying what has already been implicit by the fact that header extensions are required to be 8-byte size multiple (which makes no sense unless they are also 8-byte aligned).

OK

> 
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   docs/interop/qcow2.txt | 5 +++++
>>   1 file changed, 5 insertions(+)
>>
>> diff --git a/docs/interop/qcow2.txt b/docs/interop/qcow2.txt
>> index 4709f3bb30..b971e59b1a 100644
>> --- a/docs/interop/qcow2.txt
>> +++ b/docs/interop/qcow2.txt
>> @@ -185,6 +185,11 @@ which is covered by @header_length must be zeroed.
>>           < ... No additional fields in the header currently ... >
>> +Header padding
>> +        If @header_length is larger than 104, software SHOULD make it a
>> +        multiply of 8, adding zero-padding after additional fields. Still the
>> +        padding is optional and may be absent in the image.
>> +
>>   Directly after the image header, optional sections called header extensions can
>>   be stored. Each extension has a structure like the following:
>>
> 


-- 
Best regards,
Vladimir

[PATCH v8 3/3] docs: qcow2: introduce compression type feature

[Vladimir Sementsov-Ogievskiy]

The patch add new additional field to qcow2 header: compression_type,
which specifies compression type. If field is absent or zero, default
compression type is set: ZLIB, which corresponds to current behavior.

New compression type (ZSTD) is to be added in further commit.

Suggested-by: Denis Plotnikov <dplotnikov@virtuozzo.com>
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 docs/interop/qcow2.txt | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/docs/interop/qcow2.txt b/docs/interop/qcow2.txt
index b971e59b1a..4eabd81363 100644
--- a/docs/interop/qcow2.txt
+++ b/docs/interop/qcow2.txt
@@ -109,6 +109,12 @@ least next five fields, up to the @header_length field.
                                 An External Data File Name header extension may
                                 be present if this bit is set.
 
+                    Bit 3:      Compression type bit.  If this bit is set,
+                                non-default compression is used for compressed
+                                clusters. In this case, @header_length must
+                                be at least 105 and @compression_type field
+                                must be non-zero.
+
                     Bits 3-63:  Reserved (set to 0)
 
          80 -  87:  compatible_features
@@ -183,7 +189,18 @@ It's allowed for the header end to cut some field in the middle (in this case
 the field is considered as absent), but in this case the part of the field
 which is covered by @header_length must be zeroed.
 
-        < ... No additional fields in the header currently ... >
+              104:  compression_type
+                    Defines the compression method used for compressed clusters.
+                    A single compression type is applied to all compressed image
+                    clusters.
+                    If incompatible compression type bit is set: the field must
+                    exist (i.e. @header_length >= 105) and must be non-zero (
+                    which means non-zlib compression type)
+                    If incompatible compression type bit is unset: the field
+                    may not exist (if @header_length < 105) or it must be zero
+                    (which means zlib).
+                    Available compression type values:
+                        0: zlib <https://www.zlib.net/>
 
 Header padding
         If @header_length is larger than 104, software SHOULD make it a
-- 
2.21.0

Re: [PATCH v8 3/3] docs: qcow2: introduce compression type feature

[Eric Blake]

On 10/18/19 4:47 AM, Vladimir Sementsov-Ogievskiy wrote:
> The patch add new additional field to qcow2 header: compression_type,
> which specifies compression type. If field is absent or zero, default
> compression type is set: ZLIB, which corresponds to current behavior.
> 
> New compression type (ZSTD) is to be added in further commit.
> 
> Suggested-by: Denis Plotnikov <dplotnikov@virtuozzo.com>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>   docs/interop/qcow2.txt | 19 ++++++++++++++++++-
>   1 file changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/docs/interop/qcow2.txt b/docs/interop/qcow2.txt
> index b971e59b1a..4eabd81363 100644
> --- a/docs/interop/qcow2.txt
> +++ b/docs/interop/qcow2.txt
> @@ -109,6 +109,12 @@ least next five fields, up to the @header_length field.
>                                   An External Data File Name header extension may
>                                   be present if this bit is set.
>   
> +                    Bit 3:      Compression type bit.  If this bit is set,
> +                                non-default compression is used for compressed
> +                                clusters. In this case, @header_length must
> +                                be at least 105 and @compression_type field
> +                                must be non-zero.

I want to insist on 8-byte alignment, so this should be at least 112.

Also, as pointed out in v7, use of the decoration '@header_length' and 
'@compression_type' is not consistent with the rest of the document. 
Drop the @.

> +
>                       Bits 3-63:  Reserved (set to 0)
>   
>            80 -  87:  compatible_features
> @@ -183,7 +189,18 @@ It's allowed for the header end to cut some field in the middle (in this case
>   the field is considered as absent), but in this case the part of the field
>   which is covered by @header_length must be zeroed.
>   
> -        < ... No additional fields in the header currently ... >
> +              104:  compression_type
> +                    Defines the compression method used for compressed clusters.
> +                    A single compression type is applied to all compressed image
> +                    clusters.
> +                    If incompatible compression type bit is set: the field must
> +                    exist (i.e. @header_length >= 105) and must be non-zero (
> +                    which means non-zlib compression type)
> +                    If incompatible compression type bit is unset: the field
> +                    may not exist (if @header_length < 105) or it must be zero
> +                    (which means zlib).
> +                    Available compression type values:
> +                        0: zlib <https://www.zlib.net/>
>   

One byte for the field is fine, but I'd still explicitly document 7 
bytes of unused padding, since I want to insist on an 8-byte multiple.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH v8 3/3] docs: qcow2: introduce compression type feature

[Vladimir Sementsov-Ogievskiy]

18.10.2019 17:04, Eric Blake wrote:
> On 10/18/19 4:47 AM, Vladimir Sementsov-Ogievskiy wrote:
>> The patch add new additional field to qcow2 header: compression_type,
>> which specifies compression type. If field is absent or zero, default
>> compression type is set: ZLIB, which corresponds to current behavior.
>>
>> New compression type (ZSTD) is to be added in further commit.
>>
>> Suggested-by: Denis Plotnikov <dplotnikov@virtuozzo.com>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   docs/interop/qcow2.txt | 19 ++++++++++++++++++-
>>   1 file changed, 18 insertions(+), 1 deletion(-)
>>
>> diff --git a/docs/interop/qcow2.txt b/docs/interop/qcow2.txt
>> index b971e59b1a..4eabd81363 100644
>> --- a/docs/interop/qcow2.txt
>> +++ b/docs/interop/qcow2.txt
>> @@ -109,6 +109,12 @@ least next five fields, up to the @header_length field.
>>                                   An External Data File Name header extension may
>>                                   be present if this bit is set.
>> +                    Bit 3:      Compression type bit.  If this bit is set,
>> +                                non-default compression is used for compressed
>> +                                clusters. In this case, @header_length must
>> +                                be at least 105 and @compression_type field
>> +                                must be non-zero.
> 
> I want to insist on 8-byte alignment, so this should be at least 112.
> 
> Also, as pointed out in v7, use of the decoration '@header_length' and '@compression_type' is not consistent with the rest of the document. Drop the @.

Missed this, sorry(

> 
>> +
>>                       Bits 3-63:  Reserved (set to 0)
>>            80 -  87:  compatible_features
>> @@ -183,7 +189,18 @@ It's allowed for the header end to cut some field in the middle (in this case
>>   the field is considered as absent), but in this case the part of the field
>>   which is covered by @header_length must be zeroed.
>> -        < ... No additional fields in the header currently ... >
>> +              104:  compression_type
>> +                    Defines the compression method used for compressed clusters.
>> +                    A single compression type is applied to all compressed image
>> +                    clusters.
>> +                    If incompatible compression type bit is set: the field must
>> +                    exist (i.e. @header_length >= 105) and must be non-zero (
>> +                    which means non-zlib compression type)
>> +                    If incompatible compression type bit is unset: the field
>> +                    may not exist (if @header_length < 105) or it must be zero
>> +                    (which means zlib).
>> +                    Available compression type values:
>> +                        0: zlib <https://www.zlib.net/>
> 
> One byte for the field is fine, but I'd still explicitly document 7 bytes of unused padding, since I want to insist on an 8-byte multiple.
> 

OK

-- 
Best regards,
Vladimir