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

[Vladimir Sementsov-Ogievskiy]

20.01.2020 19:14, Max Reitz wrote:
> 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/)

Sounds better, yes.

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

OK)

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

Agreed, thanks.

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


-- 
Best regards,
Vladimir