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

[Max Reitz]

On 31.01.20 19:15, Vladimir Sementsov-Ogievskiy wrote:
> 31.01.2020 20:49, Eric Blake wrote:
>> On 1/31/20 11:34 AM, Alberto Garcia wrote:
>>> On Fri 31 Jan 2020 03:46:12 PM CET, Eric Blake wrote:
>>>>> +                    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).
>>>>
>>>>              If the incompatible bit "Compression type" is set: the
>>>> field
>>>>              must be present. Otherwise, this field must not be present
>>>>              or must be zero (which means zlib).
>>>
>>> But "not being present" and "being zero" is equivalent (as described in
>>> the previous commit).
>>>
>>> And if the incompatible bit is not present then the field can be safely
>>> ignored (i.e. whether it is zero or not is irrelevant).
>>>
>>> Let's try again:
>>>
>>>     Defines the compression method used for compressed clusters. All
>>>     compressed clusters in an image use the same type.
>>>
>>>     The value of this field should only be used when the incompatible
>>> bit
>>>     "Compression type" is set. If that bit is unset then this field is
>>>     not used and the compression method is zlib.

This doesn’t fully make sense to me.  Maybe with s/should/must/, because
as it is it means that for non-zlib compression methods, you *should*
set the compression type bit and add this header extension; but you may
also just add the extension and not set the compression bit.

All in all, I didn’t see anyone disagreeing on the fact that there are
only two cases that make any sense:
(1) Have the bit unset and the extension not present or zero: zlib.
(2) Have the bit set and an extension present and non-zero: not zlib.

If those are the only sensible choices, I don’t see any practical
argument for allowing anything else[1].  (However, I do see an argument
for forbidding anything else, namely to ensure that everyone follows
these sensible guidelines.)

>> I like that wording.
>>
> 
> I'm OK with it too, as well as I'm OK with the stricter variant, when we
> don't allow incompatible images with zlib set. I don't see any serious
> difference.
> 
> But I need this to land somehow. Max likes stricter variant and he is
> maintainer of qcow2..
> 
> Max, will you merge it as is, or did you change your mind, or should we
> ask Kevin for his opinion?

I’m currently preparing a pull request (without this series), but after
that I’m planning to merge the stricter variant.

As far as I’ve seen, the argument for making it less strict was still
accompanied by “Sure, nobody would set this flag for zlib-compressed
images because that doesn’t make sense”.  So if nobody would do that, we
might as well just forbid it and thus ensure that everyone indeed does
the sensible thing.

Max


[1] Besides “The specification would be one restriction shorter”, which
I don’t think is a very good argument.  Because without that sentence,
anyone who implements qcow2 has to think about the problem anyway and
figure out that quasi-restriction by themselves.  If they don’t, it’d be
a bit bad because they’d produce incompatible zlib-compressed images for
no reason.  Hence why I don’t see the restriction as a burden to the
reader but as a helpful guideline (that must be followed).

signature.asc
Description: OpenPGP digital signature