[PATCH v11 0/2] docs: qcow2: introduce compression type feature

[PATCH v11 0/2] docs: qcow2: introduce compression type feature

[Vladimir Sementsov-Ogievskiy]

v11:
01: grammar/wording [Eric]
    add Eric's r-b
02: empty lines between paragraphs [Eric]

Vladimir Sementsov-Ogievskiy (2):
  docs: improve qcow2 spec about extending image header
  docs: qcow2: introduce compression type feature

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

-- 
2.21.0

[PATCH v11 1/2] 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 that 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.
2. 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>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 docs/interop/qcow2.txt | 45 +++++++++++++++++++++++++++++++++++++++---
 1 file changed, 42 insertions(+), 3 deletions(-)

diff --git a/docs/interop/qcow2.txt b/docs/interop/qcow2.txt
index af5711e533..823cc266e0 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, 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,45 @@ 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 and must be a multiple
+                    of 8.
+
+
+=== Additional fields (version 3 and higher) ===
+
+In general, these fields are optional and may be safely ignored by the software,
+as well as filled by zeros (which is equal to field absence), if software needs
+to set field B, but does not care about field A which precedes B. More
+formally, additional fields have the following compatibility rules:
+
+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 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*, if not altered by a specific incompatible bit.
+
+*. A field is considered not present when header_length is less than or equal
+to the field's offset. Also, all additional fields are not present for
+version 2.
+
+        < ... No additional fields in the header currently ... >
+
+
+=== Header padding ===
+
+@header_length must be a multiple of 8, which means that if the end of the last
+additional field is not aligned, some padding is needed. This padding must be
+zeroed, so that if some existing (or future) additional field will fall into
+the padding, it will be interpreted accordingly to point [3.] of the previous
+paragraph, i.e.  in the same manner as when this field is not present.
+
+
+=== Header extensions ===
 
 Directly after the image header, optional sections called header extensions can
 be stored. Each extension has a structure like the following:
-- 
2.21.0

[PATCH v11 2/2] docs: qcow2: introduce compression type feature

[Vladimir Sementsov-Ogievskiy]

The patch adds a new additional field to the 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 823cc266e0..3a8d5c0c6e 100644
--- a/docs/interop/qcow2.txt
+++ b/docs/interop/qcow2.txt
@@ -109,6 +109,11 @@ the next fields through header_length.
                                 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,
+                                a non-default compression is used for compressed
+                                clusters. The compression_type field must be
+                                present and not zero.
+
                     Bits 3-63:  Reserved (set to 0)
 
          80 -  87:  compatible_features
@@ -190,7 +195,19 @@ present*, if not altered by a specific incompatible bit.
 to the field's offset. Also, all additional fields are not present for
 version 2.
 
-        < ... No additional fields in the header currently ... >
+              104:  compression_type
+
+                    Defines the compression method used for compressed clusters.
+                    All compressed clusters in an image use the same compression
+                    type.
+
+                    If the incompatible bit "Compression type" is set: the field
+                    must be present and non-zero (which means non-zlib
+                    compression type). Otherwise, this field must not be present
+                    or must be zero (which means zlib).
+
+                    Available compression type values:
+                        0: zlib <https://www.zlib.net/>
 
 
 === Header padding ===
-- 
2.21.0

Re: [PATCH v11 2/2] docs: qcow2: introduce compression type feature

[Eric Blake]

On 1/31/20 8:22 AM, Vladimir Sementsov-Ogievskiy wrote:
> The patch adds a new additional field to the 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 823cc266e0..3a8d5c0c6e 100644
> --- a/docs/interop/qcow2.txt
> +++ b/docs/interop/qcow2.txt
> @@ -109,6 +109,11 @@ the next fields through header_length.
>                                   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,
> +                                a non-default compression is used for compressed
> +                                clusters. The compression_type field must be
> +                                present and not zero.
> +

I'm still not sold on this.  What's wrong with:

            Bit 3:      Compression type bit.  If this bit is set,
                        a non-default compression may be used for
                        compressed clusters, and the compression_type
                        field must be present.

>                       Bits 3-63:  Reserved (set to 0)
>   
>            80 -  87:  compatible_features
> @@ -190,7 +195,19 @@ present*, if not altered by a specific incompatible bit.
>   to the field's offset. Also, all additional fields are not present for
>   version 2.
>   
> -        < ... No additional fields in the header currently ... >
> +              104:  compression_type
> +
> +                    Defines the compression method used for compressed clusters.
> +                    All compressed clusters in an image use the same compression
> +                    type.
> +
> +                    If the incompatible bit "Compression type" is set: the field
> +                    must be present and non-zero (which means non-zlib
> +                    compression type). Otherwise, this field must not be present
> +                    or must be zero (which means zlib).

            If the incompatible bit "Compression type" is set: the field
            must be present. Otherwise, this field must not be present
            or must be zero (which means zlib).  It is recommended but
            not required that the "Compression type" bit be clear if this
            field is present but zero.

> +
> +                    Available compression type values:
> +                        0: zlib <https://www.zlib.net/>
>   
>   
>   === Header padding ===
> 

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

Re: [PATCH v11 1/2] docs: improve qcow2 spec about extending image header

[Eric Blake]

On 1/31/20 8:22 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 that 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.
> 2. If we want to add incompatible field (or a field, for which some its

s/some/some of/

>     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>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
>   docs/interop/qcow2.txt | 45 +++++++++++++++++++++++++++++++++++++++---
>   1 file changed, 42 insertions(+), 3 deletions(-)
-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH v11 1/2] docs: improve qcow2 spec about extending image header

[Alberto Garcia]

On Fri 31 Jan 2020 03:22:18 PM CET, 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 that 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.
> 2. 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>
> Reviewed-by: Eric Blake <eblake@redhat.com>

Reviewed-by: Alberto Garcia <berto@igalia.com>

Berto

Re: [PATCH v11 2/2] docs: qcow2: introduce compression type feature

[Alberto Garcia]

On Fri 31 Jan 2020 03:46:12 PM CET, Eric Blake wrote:
>> +                    If the incompatible bit "Compression type" is set: the field
>> +                    must be present and non-zero (which means non-zlib
>> +                    compression type). Otherwise, this field must not be present
>> +                    or must be zero (which means zlib).
>
>             If the incompatible bit "Compression type" is set: the field
>             must be present. Otherwise, this field must not be present
>             or must be zero (which means zlib).

But "not being present" and "being zero" is equivalent (as described in
the previous commit).

And if the incompatible bit is not present then the field can be safely
ignored (i.e. whether it is zero or not is irrelevant).

Let's try again:

   Defines the compression method used for compressed clusters. All
   compressed clusters in an image use the same type.

   The value of this field should only be used when the incompatible bit
   "Compression type" is set. If that bit is unset then this field is
   not used and the compression method is zlib.

Berto

Re: [PATCH v11 2/2] docs: qcow2: introduce compression type feature

[Eric Blake]

On 1/31/20 11:34 AM, Alberto Garcia wrote:
> On Fri 31 Jan 2020 03:46:12 PM CET, Eric Blake wrote:
>>> +                    If the incompatible bit "Compression type" is set: the field
>>> +                    must be present and non-zero (which means non-zlib
>>> +                    compression type). Otherwise, this field must not be present
>>> +                    or must be zero (which means zlib).
>>
>>              If the incompatible bit "Compression type" is set: the field
>>              must be present. Otherwise, this field must not be present
>>              or must be zero (which means zlib).
> 
> But "not being present" and "being zero" is equivalent (as described in
> the previous commit).
> 
> And if the incompatible bit is not present then the field can be safely
> ignored (i.e. whether it is zero or not is irrelevant).
> 
> Let's try again:
> 
>     Defines the compression method used for compressed clusters. All
>     compressed clusters in an image use the same type.
> 
>     The value of this field should only be used when the incompatible bit
>     "Compression type" is set. If that bit is unset then this field is
>     not used and the compression method is zlib.

I like that wording.

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

Re: [PATCH v11 2/2] docs: qcow2: introduce compression type feature

[Vladimir Sementsov-Ogievskiy]

31.01.2020 20:49, Eric Blake wrote:
> On 1/31/20 11:34 AM, Alberto Garcia wrote:
>> On Fri 31 Jan 2020 03:46:12 PM CET, Eric Blake wrote:
>>>> +                    If the incompatible bit "Compression type" is set: the field
>>>> +                    must be present and non-zero (which means non-zlib
>>>> +                    compression type). Otherwise, this field must not be present
>>>> +                    or must be zero (which means zlib).
>>>
>>>              If the incompatible bit "Compression type" is set: the field
>>>              must be present. Otherwise, this field must not be present
>>>              or must be zero (which means zlib).
>>
>> But "not being present" and "being zero" is equivalent (as described in
>> the previous commit).
>>
>> And if the incompatible bit is not present then the field can be safely
>> ignored (i.e. whether it is zero or not is irrelevant).
>>
>> Let's try again:
>>
>>     Defines the compression method used for compressed clusters. All
>>     compressed clusters in an image use the same type.
>>
>>     The value of this field should only be used when the incompatible bit
>>     "Compression type" is set. If that bit is unset then this field is
>>     not used and the compression method is zlib.
> 
> I like that wording.
> 

I'm OK with it too, as well as I'm OK with the stricter variant, when we don't allow incompatible images with zlib set. I don't see any serious difference.

But I need this to land somehow. Max likes stricter variant and he is maintainer of qcow2..

Max, will you merge it as is, or did you change your mind, or should we ask Kevin for his opinion?

-- 
Best regards,
Vladimir

Re: [PATCH v11 2/2] docs: qcow2: introduce compression type feature

[Alberto Garcia]

On Fri 31 Jan 2020 07:15:33 PM CET, Vladimir Sementsov-Ogievskiy wrote:

> I'm OK with it too, as well as I'm OK with the stricter variant, when
> we don't allow incompatible images with zlib set. I don't see any
> serious difference.

I also think both options are fine.

Berto

Re: [PATCH v11 2/2] docs: qcow2: introduce compression type feature

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 3532 bytes --]

On 31.01.20 19:15, Vladimir Sementsov-Ogievskiy wrote:
> 31.01.2020 20:49, Eric Blake wrote:
>> On 1/31/20 11:34 AM, Alberto Garcia wrote:
>>> On Fri 31 Jan 2020 03:46:12 PM CET, Eric Blake wrote:
>>>>> +                    If the incompatible bit "Compression type" is
>>>>> set: the field
>>>>> +                    must be present and non-zero (which means
>>>>> non-zlib
>>>>> +                    compression type). Otherwise, this field must
>>>>> not be present
>>>>> +                    or must be zero (which means zlib).
>>>>
>>>>              If the incompatible bit "Compression type" is set: the
>>>> field
>>>>              must be present. Otherwise, this field must not be present
>>>>              or must be zero (which means zlib).
>>>
>>> But "not being present" and "being zero" is equivalent (as described in
>>> the previous commit).
>>>
>>> And if the incompatible bit is not present then the field can be safely
>>> ignored (i.e. whether it is zero or not is irrelevant).
>>>
>>> Let's try again:
>>>
>>>     Defines the compression method used for compressed clusters. All
>>>     compressed clusters in an image use the same type.
>>>
>>>     The value of this field should only be used when the incompatible
>>> bit
>>>     "Compression type" is set. If that bit is unset then this field is
>>>     not used and the compression method is zlib.

This doesn’t fully make sense to me.  Maybe with s/should/must/, because
as it is it means that for non-zlib compression methods, you *should*
set the compression type bit and add this header extension; but you may
also just add the extension and not set the compression bit.

All in all, I didn’t see anyone disagreeing on the fact that there are
only two cases that make any sense:
(1) Have the bit unset and the extension not present or zero: zlib.
(2) Have the bit set and an extension present and non-zero: not zlib.

If those are the only sensible choices, I don’t see any practical
argument for allowing anything else[1].  (However, I do see an argument
for forbidding anything else, namely to ensure that everyone follows
these sensible guidelines.)

>> I like that wording.
>>
> 
> I'm OK with it too, as well as I'm OK with the stricter variant, when we
> don't allow incompatible images with zlib set. I don't see any serious
> difference.
> 
> But I need this to land somehow. Max likes stricter variant and he is
> maintainer of qcow2..
> 
> Max, will you merge it as is, or did you change your mind, or should we
> ask Kevin for his opinion?

I’m currently preparing a pull request (without this series), but after
that I’m planning to merge the stricter variant.

As far as I’ve seen, the argument for making it less strict was still
accompanied by “Sure, nobody would set this flag for zlib-compressed
images because that doesn’t make sense”.  So if nobody would do that, we
might as well just forbid it and thus ensure that everyone indeed does
the sensible thing.

Max


[1] Besides “The specification would be one restriction shorter”, which
I don’t think is a very good argument.  Because without that sentence,
anyone who implements qcow2 has to think about the problem anyway and
figure out that quasi-restriction by themselves.  If they don’t, it’d be
a bit bad because they’d produce incompatible zlib-compressed images for
no reason.  Hence why I don’t see the restriction as a burden to the
reader but as a helpful guideline (that must be followed).


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v11 0/2] docs: qcow2: introduce compression type feature

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 607 bytes --]

On 31.01.20 15:22, Vladimir Sementsov-Ogievskiy wrote:
> v11:
> 01: grammar/wording [Eric]
>     add Eric's r-b
> 02: empty lines between paragraphs [Eric]
> 
> Vladimir Sementsov-Ogievskiy (2):
>   docs: improve qcow2 spec about extending image header
>   docs: qcow2: introduce compression type feature
> 
>  docs/interop/qcow2.txt | 62 ++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 59 insertions(+), 3 deletions(-)

Thanks, I’ve fixed patch 1’s commit message and applied the series to my
block branch:

https://git.xanclic.moe/XanClic/qemu/commits/branch/block

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v11 2/2] docs: qcow2: introduce compression type feature

[Eric Blake]

On 2/6/20 6:38 AM, Max Reitz wrote:

>> I'm OK with it too, as well as I'm OK with the stricter variant, when we
>> don't allow incompatible images with zlib set. I don't see any serious
>> difference.
>>
>> But I need this to land somehow. Max likes stricter variant and he is
>> maintainer of qcow2..
>>
>> Max, will you merge it as is, or did you change your mind, or should we
>> ask Kevin for his opinion?
> 
> I’m currently preparing a pull request (without this series), but after
> that I’m planning to merge the stricter variant.
> 
> As far as I’ve seen, the argument for making it less strict was still
> accompanied by “Sure, nobody would set this flag for zlib-compressed
> images because that doesn’t make sense”.  So if nobody would do that, we
> might as well just forbid it and thus ensure that everyone indeed does
> the sensible thing.

Fair enough; I'm happy to live with your decision as maintainer's 
prerogative.

I _do_ hope that the actual implementation series gets merged soon, 
though, and that as part of that series, you remember to tweak the 
optional 'Feature Name' extension header to name the new incompatible 
bit introduced in this patch.  And there's the simultaneous patches from 
my qcow2 autoclear-all-zeroes bit that touch the same files, so we may 
have some rebasing fun ahead of us...

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

Re: [PATCH v11 0/2] docs: qcow2: introduce compression type feature

[Vladimir Sementsov-Ogievskiy]

06.02.2020 16:51, Max Reitz wrote:
> On 31.01.20 15:22, Vladimir Sementsov-Ogievskiy wrote:
>> v11:
>> 01: grammar/wording [Eric]
>>      add Eric's r-b
>> 02: empty lines between paragraphs [Eric]
>>
>> Vladimir Sementsov-Ogievskiy (2):
>>    docs: improve qcow2 spec about extending image header
>>    docs: qcow2: introduce compression type feature
>>
>>   docs/interop/qcow2.txt | 62 ++++++++++++++++++++++++++++++++++++++++--
>>   1 file changed, 59 insertions(+), 3 deletions(-)
> 
> Thanks, I’ve fixed patch 1’s commit message and applied the series to my
> block branch:
> 
> https://git.xanclic.moe/XanClic/qemu/commits/branch/block
> 

Thanks!


-- 
Best regards,
Vladimir