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 <dplotni...@virtuozzo.com>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsement...@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
