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

[Vladimir Sementsov-Ogievskiy]

18.10.2019 17:36, Vladimir Sementsov-Ogievskiy wrote:
> 18.10.2019 17:00, Eric Blake wrote:
>> On 10/18/19 4:47 AM, Vladimir Sementsov-Ogievskiy wrote:
>>> Make it more obvious how to add new fields to the version 3 header and
>>> how to interpret them.
>>>
>>> The specification is adjusted so for new defined optional fields:
>>
>> The specification is adjusted to make it clear that future fields are 
>> optional:
>>
>>>
>>> 1. Software may support some of these optional fields and ignore the
>>>     others, which means that features may be backported to downstream
>>>     Qemu independently.
>>> 3. If @header_length is higher than the highest field end that software
>>>     knows, it should assume that topmost unknown additional fields are
>>>     correct, and keep additional unknown fields as is on rewriting the
>>>     image.
>>> 3. If we want to add incompatible field (or a field, for which some its
>>>     values would be incompatible), it must be accompanied by
>>>     incompatible feature bit.
>>>
>>> Also the concept of "default is zero" is clarified, as it's strange to
>>> say that the value of the field is assumed to be zero for the software
>>> version which don't know about the field at all and don't know how to
>>> treat it be it zero or not.
>>>
>>
>> I'd also mention that we want to enforce 8-byte alignment in this cover 
>> letter.
>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
>>> ---
>>>   docs/interop/qcow2.txt | 26 +++++++++++++++++++++++---
>>>   1 file changed, 23 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/docs/interop/qcow2.txt b/docs/interop/qcow2.txt
>>> index af5711e533..4709f3bb30 100644
>>> --- a/docs/interop/qcow2.txt
>>> +++ b/docs/interop/qcow2.txt
>>> @@ -79,9 +79,9 @@ The first cluster of a qcow2 image contains the file 
>>> header:
>>>                       Offset into the image file at which the snapshot table
>>>                       starts. Must be aligned to a cluster boundary.
>>> -If the version is 3 or higher, the header has the following additional 
>>> fields.
>>> -For version 2, the values are assumed to be zero, unless specified 
>>> otherwise
>>> -in the description of a field.
>>> +For version 2, header is always 72 bytes length and finishes here.
>>> +For version 3 or higher the header length is at least 104 bytes and has at
>>> +least next five fields, up to the @header_length field.
>>
>> For version 2, the header is exactly 72 bytes in length, and finishes here.
>> For version 3 or higher, the header length is at least 104 bytes, including 
>> the next fields through header_length.
>>
>>>            72 -  79:  incompatible_features
>>>                       Bitmask of incompatible features. An implementation 
>>> must
>>> @@ -164,6 +164,26 @@ in the description of a field.
>>>           100 - 103:  header_length
>>>                       Length of the header structure in bytes. For version 2
>>>                       images, the length is always assumed to be 72 bytes.
>>> +                    For version 3 it's at least 104 bytes.
>>
>> I'd also add a sentence that this field must be a multiple of 8.
>>
>>> +
>>> +Additional fields (version 3 and higher)
>>> +
>>> +The following fields of the header are optional: 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.
>>
>> 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?
> 
> So, may be
> 
> Additional fields has the following compatible behavior by default:
> 
> 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.
> 
> 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.
> 
>>
>>> +
>>> +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.
> 
> 
> 

Eric what do you think?

-- 
Best regards,
Vladimir