[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v8 1/3] docs: improve qcow2 spec about extending image header
From: |
Vladimir Sementsov-Ogievskiy |
Subject: |
Re: [PATCH v8 1/3] docs: improve qcow2 spec about extending image header |
Date: |
Mon, 2 Dec 2019 14:14:40 +0000 |
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
[Prev in Thread] |
Current Thread |
[Next in Thread] |
- Re: [PATCH v8 1/3] docs: improve qcow2 spec about extending image header,
Vladimir Sementsov-Ogievskiy <=