* [PATCH v10 0/2] qcow2: add zstd cluster compression
@ 2020-01-20 17:13 Vladimir Sementsov-Ogievskiy
2020-01-20 17:13 ` [PATCH v10 1/2] docs: improve qcow2 spec about extending image header Vladimir Sementsov-Ogievskiy
2020-01-20 17:13 ` [PATCH v10 2/2] docs: qcow2: introduce compression type feature Vladimir Sementsov-Ogievskiy
0 siblings, 2 replies; 10+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-01-20 17:13 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, vsementsov, armbru, qemu-devel, dplotnikov, den, mreitz
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.
v10: Spelling/structure improvements by Max
v9: Merge 01 and 02
Change wordings
Require header alignment
Vladimir Sementsov-Ogievskiy (2):
docs: improve qcow2 spec about extending image header
docs: qcow2: introduce compression type feature
docs/interop/qcow2.txt | 58 +++++++++++++++++++++++++++++++++++++++---
1 file changed, 55 insertions(+), 3 deletions(-)
--
2.21.0
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v10 1/2] docs: improve qcow2 spec about extending image header
2020-01-20 17:13 [PATCH v10 0/2] qcow2: add zstd cluster compression Vladimir Sementsov-Ogievskiy
@ 2020-01-20 17:13 ` Vladimir Sementsov-Ogievskiy
2020-01-20 19:42 ` Eric Blake
2020-01-20 17:13 ` [PATCH v10 2/2] docs: qcow2: introduce compression type feature Vladimir Sementsov-Ogievskiy
1 sibling, 1 reply; 10+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-01-20 17:13 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, vsementsov, armbru, qemu-devel, dplotnikov, den, mreitz
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.
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>
---
docs/interop/qcow2.txt | 44 +++++++++++++++++++++++++++++++++++++++---
1 file changed, 41 insertions(+), 3 deletions(-)
diff --git a/docs/interop/qcow2.txt b/docs/interop/qcow2.txt
index af5711e533..355925c35e 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,44 @@ 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 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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH v10 2/2] docs: qcow2: introduce compression type feature
2020-01-20 17:13 [PATCH v10 0/2] qcow2: add zstd cluster compression Vladimir Sementsov-Ogievskiy
2020-01-20 17:13 ` [PATCH v10 1/2] docs: improve qcow2 spec about extending image header Vladimir Sementsov-Ogievskiy
@ 2020-01-20 17:13 ` Vladimir Sementsov-Ogievskiy
2020-01-20 19:46 ` Eric Blake
1 sibling, 1 reply; 10+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-01-20 17:13 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, vsementsov, armbru, qemu-devel, dplotnikov, den, mreitz
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 | 16 +++++++++++++++-
1 file changed, 15 insertions(+), 1 deletion(-)
diff --git a/docs/interop/qcow2.txt b/docs/interop/qcow2.txt
index 355925c35e..4569f0dba3 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
@@ -189,7 +194,16 @@ present*, if not altered by a specific incompatible bit.
*. A field is considered not present when header_length is less or equal 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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v10 1/2] docs: improve qcow2 spec about extending image header
2020-01-20 17:13 ` [PATCH v10 1/2] docs: improve qcow2 spec about extending image header Vladimir Sementsov-Ogievskiy
@ 2020-01-20 19:42 ` Eric Blake
2020-01-21 16:18 ` Max Reitz
2020-01-31 13:55 ` Vladimir Sementsov-Ogievskiy
0 siblings, 2 replies; 10+ messages in thread
From: Eric Blake @ 2020-01-20 19:42 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-block
Cc: kwolf, armbru, qemu-devel, dplotnikov, den, mreitz
On 1/20/20 11:13 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:
s/so for/so that for/
>
> 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>
> ---
> docs/interop/qcow2.txt | 44 +++++++++++++++++++++++++++++++++++++++---
> 1 file changed, 41 insertions(+), 3 deletions(-)
>
> diff --git a/docs/interop/qcow2.txt b/docs/interop/qcow2.txt
> index af5711e533..355925c35e 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,44 @@ 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
We're inconsistent on 'zeros' (git grep has 201 hits) vs. 'zeroes' (688
hits); I prefer the latter, but won't object if you don't tweak it since
this is the first use of either spelling in qcow2.txt.
> +to set field B, but does not care about field A, which precedes B. More
s/A, which/A which/
> +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 or equal to the
s/less/less than/
> +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
s/that, if/that if/
> +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:
>
We're down to few enough grammar nits that I'm happy with:
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v10 2/2] docs: qcow2: introduce compression type feature
2020-01-20 17:13 ` [PATCH v10 2/2] docs: qcow2: introduce compression type feature Vladimir Sementsov-Ogievskiy
@ 2020-01-20 19:46 ` Eric Blake
2020-01-21 9:06 ` Vladimir Sementsov-Ogievskiy
2020-01-21 16:20 ` Max Reitz
0 siblings, 2 replies; 10+ messages in thread
From: Eric Blake @ 2020-01-20 19:46 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-block
Cc: kwolf, armbru, qemu-devel, dplotnikov, den, mreitz
On 1/20/20 11:13 AM, Vladimir Sementsov-Ogievskiy wrote:
> The patch add new additional field to qcow2 header: compression_type,
s/add/adds a/
s/to/to the/
> 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.
It would be nice to have that patch as part of the same series, but it
has already been posted to the list separately, so I'm okay with this
series as just doc word-smithing while we get that patch series in soon.
>
> Suggested-by: Denis Plotnikov <dplotnikov@virtuozzo.com>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
> docs/interop/qcow2.txt | 16 +++++++++++++++-
> 1 file changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/docs/interop/qcow2.txt b/docs/interop/qcow2.txt
> index 355925c35e..4569f0dba3 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.
Why must the compression_type field be non-zero? If this bit is set, an
older qemu cannot use the image, regardless of the contents of the
compression_type field, so for maximum back-compat, a sane app will
never set this bit when compression_type is zero. But there is nothing
technically wrong with setting this bit even when compression_type is 0,
and newer qemu would still manage to use the image correctly with zlib
compression if it were not for this arbitrary restriction.
> +
> Bits 3-63: Reserved (set to 0)
>
> 80 - 87: compatible_features
> @@ -189,7 +194,16 @@ present*, if not altered by a specific incompatible bit.
> *. A field is considered not present when header_length is less or equal 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
Ragged edge formatting looks awkward. Either this is one paragraph
("type. If") or there should be a blank line to make it obvious there
are two paragraphs.
> + 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/>
I'm still not sure I agree with the mandate that the field must be
non-zero when the incompatible feature bit is set.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v10 2/2] docs: qcow2: introduce compression type feature
2020-01-20 19:46 ` Eric Blake
@ 2020-01-21 9:06 ` Vladimir Sementsov-Ogievskiy
2020-01-21 16:20 ` Max Reitz
1 sibling, 0 replies; 10+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-01-21 9:06 UTC (permalink / raw)
To: Eric Blake, qemu-block
Cc: kwolf, Denis Lunev, armbru, qemu-devel, Denis Plotnikov, mreitz
20.01.2020 22:46, Eric Blake wrote:
> On 1/20/20 11:13 AM, Vladimir Sementsov-Ogievskiy wrote:
>> The patch add new additional field to qcow2 header: compression_type,
>
> s/add/adds a/
> s/to/to the/
>
>> 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.
>
> It would be nice to have that patch as part of the same series, but it has already been posted to the list separately, so I'm okay with this series as just doc word-smithing while we get that patch series in soon.
>
>>
>> Suggested-by: Denis Plotnikov <dplotnikov@virtuozzo.com>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>> docs/interop/qcow2.txt | 16 +++++++++++++++-
>> 1 file changed, 15 insertions(+), 1 deletion(-)
>>
>> diff --git a/docs/interop/qcow2.txt b/docs/interop/qcow2.txt
>> index 355925c35e..4569f0dba3 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.
>
> Why must the compression_type field be non-zero? If this bit is set, an older qemu cannot use the image, regardless of the contents of the compression_type field, so for maximum back-compat, a sane app will never set this bit when compression_type is zero. But there is nothing technically wrong with setting this bit even when compression_type is 0, and newer qemu would still manage to use the image correctly with zlib compression if it were not for this arbitrary restriction.
OK, I just made it stricter, no actual reason for it. Then:
If this bit is set, the compression type of the image is defined by compression_type additional field (which must present in this case).
>
>> +
>> Bits 3-63: Reserved (set to 0)
>> 80 - 87: compatible_features
>> @@ -189,7 +194,16 @@ present*, if not altered by a specific incompatible bit.
>> *. A field is considered not present when header_length is less or equal 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
>
> Ragged edge formatting looks awkward. Either this is one paragraph ("type. If") or there should be a blank line to make it obvious there are two paragraphs.
OK, let it be additional empty line. Then we need one before "Defines" too?
>
>> + 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/>
>
> I'm still not sure I agree with the mandate that the field must be non-zero when the incompatible feature bit is set.
>
I don't care, so let's make it less strict.
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v10 1/2] docs: improve qcow2 spec about extending image header
2020-01-20 19:42 ` Eric Blake
@ 2020-01-21 16:18 ` Max Reitz
2020-01-31 14:05 ` Vladimir Sementsov-Ogievskiy
2020-01-31 13:55 ` Vladimir Sementsov-Ogievskiy
1 sibling, 1 reply; 10+ messages in thread
From: Max Reitz @ 2020-01-21 16:18 UTC (permalink / raw)
To: Eric Blake, Vladimir Sementsov-Ogievskiy, qemu-block
Cc: kwolf, den, dplotnikov, qemu-devel, armbru
[-- Attachment #1.1: Type: text/plain, Size: 4199 bytes --]
On 20.01.20 20:42, Eric Blake wrote:
> On 1/20/20 11:13 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:
>
> s/so for/so that for/
>
>>
>> 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>
>> ---
>> docs/interop/qcow2.txt | 44 +++++++++++++++++++++++++++++++++++++++---
>> 1 file changed, 41 insertions(+), 3 deletions(-)
>>
>> diff --git a/docs/interop/qcow2.txt b/docs/interop/qcow2.txt
>> index af5711e533..355925c35e 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,44 @@ 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
>
> We're inconsistent on 'zeros' (git grep has 201 hits) vs. 'zeroes' (688
> hits); I prefer the latter, but won't object if you don't tweak it since
> this is the first use of either spelling in qcow2.txt.
>
>> +to set field B, but does not care about field A, which precedes B. More
>
> s/A, which/A which/
I’ve heard before that one should always use a comma before “which” and
never before “that” (in that a subordinate clause opened by “that” is a
mandatory description, whereas those that start with “why” are not
necessary for understanding).
So if this is a mandatory description (which I suppose it is), shouldn’t
it also be s/which/that/?
I suppose “field A that precedes B” sounds a bit weird because “A”
hasn’t been introduced before. That is, “the field that precedes B”
would sound more natural. Or is that precisely the kind of instance
where one would use “which” without comma? :-)
All in all, I was wondering whether there isn’t a more natural way to
rephrase the whole paragraph. (No, I don’t have an excuse why I didn’t
say so in the last revision.)
Maybe:
In general, these fields are optional and may be safely ignored when
read and filled with zeroes when written. For example, say software
wants to set field B but does not care about its preceding field A. It
may then set A to zero, B to its desired value, and adjust header_length
to include A and B.
?
Max
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v10 2/2] docs: qcow2: introduce compression type feature
2020-01-20 19:46 ` Eric Blake
2020-01-21 9:06 ` Vladimir Sementsov-Ogievskiy
@ 2020-01-21 16:20 ` Max Reitz
1 sibling, 0 replies; 10+ messages in thread
From: Max Reitz @ 2020-01-21 16:20 UTC (permalink / raw)
To: Eric Blake, Vladimir Sementsov-Ogievskiy, qemu-block
Cc: kwolf, den, dplotnikov, qemu-devel, armbru
[-- Attachment #1.1: Type: text/plain, Size: 2444 bytes --]
On 20.01.20 20:46, Eric Blake wrote:
> On 1/20/20 11:13 AM, Vladimir Sementsov-Ogievskiy wrote:
>> The patch add new additional field to qcow2 header: compression_type,
>
> s/add/adds a/
> s/to/to the/
>
>> 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.
>
> It would be nice to have that patch as part of the same series, but it
> has already been posted to the list separately, so I'm okay with this
> series as just doc word-smithing while we get that patch series in soon.
>
>>
>> Suggested-by: Denis Plotnikov <dplotnikov@virtuozzo.com>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>> docs/interop/qcow2.txt | 16 +++++++++++++++-
>> 1 file changed, 15 insertions(+), 1 deletion(-)
>>
>> diff --git a/docs/interop/qcow2.txt b/docs/interop/qcow2.txt
>> index 355925c35e..4569f0dba3 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.
>
> Why must the compression_type field be non-zero? If this bit is set, an
> older qemu cannot use the image, regardless of the contents of the
> compression_type field, so for maximum back-compat, a sane app will
> never set this bit when compression_type is zero. But there is nothing
> technically wrong with setting this bit even when compression_type is 0,
> and newer qemu would still manage to use the image correctly with zlib
> compression if it were not for this arbitrary restriction.
There is something technically wrong if the specification demands otherwise.
As you yourself say, no sane software would deviate from this behavior
anyway. If that is so, I don’t see why we shouldn’t specify it this
way. I see no benefit in allowing this bit be set for zlib images.
Max
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v10 1/2] docs: improve qcow2 spec about extending image header
2020-01-20 19:42 ` Eric Blake
2020-01-21 16:18 ` Max Reitz
@ 2020-01-31 13:55 ` Vladimir Sementsov-Ogievskiy
1 sibling, 0 replies; 10+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-01-31 13:55 UTC (permalink / raw)
To: Eric Blake, qemu-block; +Cc: kwolf, armbru, qemu-devel, dplotnikov, den, mreitz
20.01.2020 22:42, Eric Blake wrote:
> On 1/20/20 11:13 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:
>
> s/so for/so that for/
>
>>
>> 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>
>> ---
>> docs/interop/qcow2.txt | 44 +++++++++++++++++++++++++++++++++++++++---
>> 1 file changed, 41 insertions(+), 3 deletions(-)
>>
>> diff --git a/docs/interop/qcow2.txt b/docs/interop/qcow2.txt
>> index af5711e533..355925c35e 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,44 @@ 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
>
> We're inconsistent on 'zeros' (git grep has 201 hits) vs. 'zeroes' (688 hits); I prefer the latter, but won't object if you don't tweak it since this is the first use of either spelling in qcow2.txt.
No, qcow2.txt already contains 4 invocations of 'zeros' (and no one 'zeroes'), so, I'll keep 'zeros' for consitency.
>
>> +to set field B, but does not care about field A, which precedes B. More
>
> s/A, which/A which/
>
>> +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 or equal to the
>
> s/less/less than/
>
>> +field's offset. Also, all additional fields are not present for version 2.
>> +jkjkkjjkjkj
>> + < ... 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
>
> s/that, if/that if/
>
>> +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:
>>
>
> We're down to few enough grammar nits that I'm happy with:
>
> Reviewed-by: Eric Blake <eblake@redhat.com>
>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v10 1/2] docs: improve qcow2 spec about extending image header
2020-01-21 16:18 ` Max Reitz
@ 2020-01-31 14:05 ` Vladimir Sementsov-Ogievskiy
0 siblings, 0 replies; 10+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-01-31 14:05 UTC (permalink / raw)
To: Max Reitz, Eric Blake, qemu-block
Cc: kwolf, den, dplotnikov, qemu-devel, armbru
21.01.2020 19:18, Max Reitz wrote:
> On 20.01.20 20:42, Eric Blake wrote:
>> On 1/20/20 11:13 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:
>>
>> s/so for/so that for/
>>
>>>
>>> 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>
>>> ---
>>> docs/interop/qcow2.txt | 44 +++++++++++++++++++++++++++++++++++++++---
>>> 1 file changed, 41 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/docs/interop/qcow2.txt b/docs/interop/qcow2.txt
>>> index af5711e533..355925c35e 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,44 @@ 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
>>
>> We're inconsistent on 'zeros' (git grep has 201 hits) vs. 'zeroes' (688
>> hits); I prefer the latter, but won't object if you don't tweak it since
>> this is the first use of either spelling in qcow2.txt.
>>
>>> +to set field B, but does not care about field A, which precedes B. More
>>
>> s/A, which/A which/
>
> I’ve heard before that one should always use a comma before “which” and
> never before “that” (in that a subordinate clause opened by “that” is a
> mandatory description, whereas those that start with “why” are not
> necessary for understanding).
>
> So if this is a mandatory description (which I suppose it is), shouldn’t
> it also be s/which/that/?
>
> I suppose “field A that precedes B” sounds a bit weird because “A”
> hasn’t been introduced before. That is, “the field that precedes B”
> would sound more natural. Or is that precisely the kind of instance
> where one would use “which” without comma? :-)
>
> All in all, I was wondering whether there isn’t a more natural way to
> rephrase the whole paragraph. (No, I don’t have an excuse why I didn’t
> say so in the last revision.)
>
> Maybe:
>
> In general, these fields are optional and may be safely ignored when
> read and filled with zeroes when written.
The end of sentence is a bit in conflict with following "... other than preserving its value when rewriting the image header ..."
(yes, here we say about creating new fields, not rewriting the existing ones, but this is not clear from the context)
So, for v10 I tend to keep it as is (with Eric's corrections).
For example, say software
> wants to set field B but does not care about its preceding field A. It
> may then set A to zero, B to its desired value, and adjust header_length
> to include A and B.
>
> ?
>
> Max
>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2020-01-31 14:06 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-20 17:13 [PATCH v10 0/2] qcow2: add zstd cluster compression Vladimir Sementsov-Ogievskiy
2020-01-20 17:13 ` [PATCH v10 1/2] docs: improve qcow2 spec about extending image header Vladimir Sementsov-Ogievskiy
2020-01-20 19:42 ` Eric Blake
2020-01-21 16:18 ` Max Reitz
2020-01-31 14:05 ` Vladimir Sementsov-Ogievskiy
2020-01-31 13:55 ` Vladimir Sementsov-Ogievskiy
2020-01-20 17:13 ` [PATCH v10 2/2] docs: qcow2: introduce compression type feature Vladimir Sementsov-Ogievskiy
2020-01-20 19:46 ` Eric Blake
2020-01-21 9:06 ` Vladimir Sementsov-Ogievskiy
2020-01-21 16:20 ` Max Reitz
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).