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

[Vladimir Sementsov-Ogievskiy]

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 <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 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