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

[Max Reitz]

On 16.12.19 13:17, 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 <address@hidden>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> ---
>  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 d92c827763..77146b5169 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,
> +                                non-default compression is used for 
> compressed

s/non-default/a non-default/

> +                                clusters. compression_type field must be
> +                                present and not zero.

s/compression_type field/The compression_type field/

> +
>                      Bits 3-63:  Reserved (set to 0)
>  
>           80 -  87:  compatible_features
> @@ -188,7 +193,16 @@ present*, if not altered by specific incompatible bit.
>  *. Field is not present when header_length is less or equal to 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.
> +                    A single compression type is applied to all compressed 
> image
> +                    clusters.

Sounds a bit too complicated.  Maybe „All compressed clusters in an
image use the same compression type.” instead?  (Or s/an/the same/)

> +                    If incompatible compression type bit is set: the field 
> must

Hmm, this sounds like there was an “incompatible compression type” bit,
instead of an incompatible bit called “compression type”.  So maybe “If
the incompatible bit "compression type" is set, this field must...”?

> +                    be present and non-zero (which means non-zlib 
> compression type)

s/$/./

> +                    If incompatible compression type bit is unset: the field

I’d just make this “Otherwise, this field...”

> +                    may not exist or it must be zero (which means zlib).

“must not be present or must be zero”?

(“exist” sounds a bit weird; the spec only defined “not present” so far.
 As for the “may not”, that isn’t in RFC 2119. :-))

Max

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

signature.asc
Description: OpenPGP digital signature