Re: [PATCH v8 1/3] docs: improve qcow2 spec about extending image header

[Vladimir Sementsov-Ogievskiy]

07.11.2019 15:26, Vladimir Sementsov-Ogievskiy wrote:
> 06.11.2019 22:19, Eric Blake wrote:
>> On 10/18/19 9:36 AM, Vladimir Sementsov-Ogievskiy wrote:
>>
>>>> Maybe:
>>>>
>>>> if software doesn't know how to interpret the field, it may be safely 
>>>> ignored unless a corresponding incompatible feature flag bit is set; 
>>>> however, the field should be preserved unchanged when rewriting the image 
>>>> header.
>>>>
>>>>> +
>>>>> +For all additional fields zero value equals to absence of field (absence 
>>>>> is
>>>>> +when field.offset + field.size > @header_length). This implies
>>>>> +that if software want's to set fields up to some field not aligned to 
>>>>> multiply
>>>>> +of 8 it must align header up by zeroes. And on the other hand, if 
>>>>> software
>>>>> +need some optional field which is absent it should assume that it's 
>>>>> value is
>>>>> +zero.
>>>>
>>>> Maybe:
>>>>
>>>> Each optional field that does not have a corresponding incompatible 
>>>> feature bit must support the value 0 that gives the same default behavior 
>>>> as when the optional field is omitted.
>>>
>>> Hmmm. That doesn't work, as "corresponding" is something not actually 
>>> defined. Consider our zstd extension.
>>>
>>> It has corresponding incompatible bit, therefore, this sentence doesn't 
>>> apply to it. But still, if incompatible bit is unset we can have this 
>>> field. And it's zero value must correspond
>>> to the absence of the field.
>>>
>>> So, additional field may use incomaptible bit only for subset of its values.
>>>
>>> But, I see, that you want to allow 0 value to not match field-absence if 
>>> incompatible bit is set?
>>
>> Not necessarily.  Rather, if the value of an unknown field can be safely 
>> ignored, then it should default to 0.  If it cannot be safely ignored, then 
>> that field will not be set to a non-zero value without also setting an 
>> incompatible feature flag, so that software that does not know how to 
>> interpret the field will fail to load the image because it also fails to 
>> recognize the associated new incompatible feature bit.
>>
>> But I'd really like Kevin's opinion on how much wording is worth adding.
>>
>>>
>>> So, may be
>>>
>>> Additional fields has the following compatible behavior by default:
>>
>> s/has/have/
>>
>>>
>>> 1. If software doesn't know how to interpret the field, it may be safely 
>>> ignored, other than preserving the field unchanged when rewriting the image 
>>> header.
>>> 2. Zeroed additional field gives the same behavior as when this field is 
>>> omitted.
>>
>> Both good.
>>
>>>
>>> This default behavior may be altered with help of incompatible feature 
>>> bits. So, if, for example, additional field has corresponding incompatible 
>>> feature bit, and it is set, we are sure that software which opens the image 
>>> knows how to interpret the field, so,
>>> 1. The field definitely will not be ignored when corresponding incompatible 
>>> bit is set.
>>> 2. The field may define any meaning it wants for zero value for the case 
>>> when corresponding incompatible bit is set.
>>
>> Rather wordy but seems accurate.  Perhaps compress it to:
>>
>> 3. Any additional field whose value must not be ignored for correct handling 
>> of the file will be accompanied by a corresponding incompatible feature bit.
>>
>> and maybe even reorder it to list the points as:
>>
>> Additional fields have the following compatibility rules:
>> 1. Any additional field whose value must not be ignored for correct handling 
>> of the file will be accompanied by a corresponding incompatible feature bit.
> 
> I'd like to stress, that incompatible bit is needed for incompatible value, 
> not for the field itself. (So field may be accompanied by incompatible bit 
> for some
> it's values and for others - not), So, what about
> 
> 1. If the value of the additional field must not be ignored for correct 
> handling of the file, it will be accompanied by a corresponding incompatible 
> feature bit.
> 
>> 2. If there are no unrecognized incompatible feature bits set, an additional 
>> field may be safely ignored other than preserving its value when rewriting 
>> the image header.
> 
> Sounds like we can ignore the field if we know its meaning and know its 
> incompatible bit..
> 
> 2. If there are no unrecognized incompatible feature bits set, an unknown 
> additional field may be safely ignored other than preserving its value when 
> rewriting the image header.
> 
>> 3. An explicit value of 0 will have the same behavior as when the field is 
>> not present.
> 
> But it may be changed by incompatible bit..
> 
> 3. An explicit value of 0 will have the same behavior as when the field is 
> not present, if not altered by specific incompatible bit.
> 

Eric, OK for you?

>>
>>
>>>>> +It's allowed for the header end to cut some field in the middle (in this 
>>>>> case
>>>>> +the field is considered as absent), but in this case the part of the 
>>>>> field
>>>>> +which is covered by @header_length must be zeroed.
>>>>> +
>>>>> +        < ... No additional fields in the header currently ... >
>>>>
>>>> Do we even still need this if we require 8-byte alignment?  We'd never be 
>>>> able to cut a single field in the middle
>>>
>>> hmm, for example:
>>> 105: compression byte
>>> 106-113: some other 8-bytes field, unalinged
>>> 113-119: padding to multiply of 8
>>>
>>> - bad example, for sure. But to prevent it, we should also define some 
>>> field alignment requirements..
>>>
>>>
>>>> , but I suppose you are worried about cutting a 2-field 16-byte addition 
>>>> tied to a single feature in the middle.
>>>
>>> and this too.
>>>
>>>>    But that's not going to happen in practice.
>>>
>>> why not?
>>>
>>> 4 bytes: feature 1
>>>
>>> 4 bytes: feature 2
>>> 8 bytes: feature 2
>>>
>>> so, last 12 bytes may be considered as one field.. And software which don't 
>>> know about feature2, will pad header to the middle of feature2
>>>
>>>> The only time the header will be longer than 104 bytes is if at least one 
>>>> documented optional feature has been implemented/backported, and that 
>>>> feature will be implemented in its entirety.  If you backport a later 
>>>> feature but not the earlier, you're still going to set header_length to 
>>>> the boundary of the feature that you ARE backporting.
>>>
>>> That's true, of course.
>>>
>>>>    Thus, I argue that blindly setting header_length to 120 prior to the 
>>>> standard ever defining optional field(s) at 112-120 is premature, and that 
>>>> if we ever add a feature requiring bytes 112-128 for a new feature, you 
>>>> will never see a valid qcow2 file with a header length of 120.
>>>
>>> consider my example above.
>>
>> As long as we never add new fields that are not 8-byte aligned (including 
>> any explicit padding), then we will never have the case of dividing fields 
>> in the middle by keeping the header length a multiple of 8.
>>
> 
> OK.
> 


-- 
Best regards,
Vladimir