Re: [PATCH v7 1/2] docs: improve qcow2 spec about extending image header

[Vladimir Sementsov-Ogievskiy]

18.10.2019 11:29, Vladimir Sementsov-Ogievskiy wrote:
> 08.10.2019 12:05, Vladimir Sementsov-Ogievskiy wrote:
>> 07.10.2019 23:21, Eric Blake wrote:
>>> On 10/7/19 11:04 AM, Vladimir Sementsov-Ogievskiy wrote:
>>>> Make it more obvious how to add new fields to the version 3 header and
>>>> how to interpret them.
>>>>
>>>> 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..3f2855593f 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.
>>>
>>> This hunk seems okay.
>>>
>>>>            72 -  79:  incompatible_features
>>>>                       Bitmask of incompatible features. An implementation 
>>>> must
>>>> @@ -165,6 +165,26 @@ in the description of a field.
>>>>                       Length of the header structure in bytes. For version 
>>>> 2
>>>>                       images, the length is always assumed to be 72 bytes.
>>>> +Additional fields (version 3 and higher)
>>>> +
>>>> +The following fields of the header are optional: if software don't know 
>>>> how to
>>>> +interpret the field, it may safely ignore it. Still the field must be 
>>>> kept as is
>>>> +when rewriting the image.
>>>
>>> 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.
>>>
>>> Missing:
>>>
>>> If header_length excludes an optional field, the value of 0 should be used 
>>> for that field.
>>
>> This is what I dislike in old wording. Why do we need this default-zero 
>> thing[*]? What is the default?
>>
>> Default is absence of the feature, we don't have these future features now 
>> and don't care of them.
>> What is this default 0 for us now? Nothing.
>>
>> Consider some future version: if it sees that header_length excludes some 
>> fields, it understands,
>> that there is no such feature here. That's all. Work without it. The feature 
>> itself should declare
>> behavior without this feature, which should correspond to behavior before 
>> this feature introduction..
>>
>> So at least, I don't like "the value of 0 should be used for that field", as 
>> instances of Qemu which
>> don't know about the feature will ignore this requirement, as they don't 
>> need any value of that
>> field at all.
>>
>> What you actually mean, IMHO, is: for all optional field 0 value must be 
>> equal to absence of the feature,
>> like when header_length excludes this field. I don't see, do we really need 
>> this requirement, but
>> seems it was mentioned before this patch and we'd better keep it.. I just 
>> don't like concept of
>> "default" value keeping in mind valid Qemu instances which don't know about 
>> field at all.
>>
>>>
>>>> @header_length must be bound to the end of one of
>>>> +these fields (or to @header_length field end itself, to be 104 bytes).
>>>
>>> We don't use the @header_length markup anywhere else in this file, starting 
>>> to do so here is odd.
>>>
>>> I would suggest a stronger requirement:
>>>
>>> header_length must be a multiple of 4, and must not land in the middle of 
>>> any optional 8-byte field.
>>>
>>> Or maybe even add our compression type extension with 4 bytes of padding, 
>>> so that we could go even stronger:
>>>
>>> header_length must be a multiple of 8.
>>
>> Hmm, if we imply that software will have to add some padding, than 
>> requirement above about zero === feature-absence
>> becomes necessary. [*]
>>
>> Still I have two questions:
>> 1. Do we really need all fields to be 4 or 8 bytes? Why not use 1 byte for 
>> compression?
>> 2. What is the benefit of padding, which you propose?
> 
> Hmm, now I think, that we should align header to multiply of 8, as header 
> extensions are already have
> """
> Directly after the image header, optional sections called header extensions 
> can
> be stored. Each extension has a structure like the following:
> 
> [...]
> 
>            n -  m:   Padding to round up the header extension size to the next
>                      multiple of 8.
> """
> 
> So, it looks inconsistent, if we pad all header extensions to  8 bytes except 
> for the start of the first extension.
> 
> I'll resend with padding soon.


Still, we have to make an exception at least for header_length = 104, which is 
not a multiply of 8.

Also, is requiring alignment is an incompatible change of specification?

> 
>>
>>>
>>>> +This definition implies the following:
>>>> +1. Software may support some of these optional fields and ignore the 
>>>> others,
>>>> +   which means that features may be backported to downstream Qemu 
>>>> independently.
>>>
>>> I don't think this belongs in the spec.
>>
>> Me too. But at least I noted what I try to achieve, so consider it a bit 
>> like RFC. And of course I hoped for your rewordings )
>>
>>>   Ideally, we add fields so infrequently that backporting doesn't have to 
>>> worry about backporting field 2 while skipping field 1.
>>
>> Who knows.. Even having only two fields A and B, when we need B which 
>> actually needs 10 patches to backport and A needs 100 would be
>> a problem, if we can't backport B in separate.
>>
>> I remember similar thing about NBD: I needed BLOCK_STATUS, but because of 
>> specification I had to implement
>> structured read first, which wasn't interesting to me at that moment.
>>
>>>
>>>> +2. Software may check @header_length, if it knows optional fields 
>>>> specification
>>>> +   enough (knows about the field which exceeds @header_length).
>>>
>>> Again, I don't think this adds anything.  Since we already documented 
>>> fields are optional, and that if header_length is too short, the missing 
>>> field is treated as 0, software that knows about a longer header_length 
>>> will already handle it correctly.
>>
>> I think, I'll move these points to commit message, to keep them somehow.
>>
>>>
>>>> +3. If @header_length is higher than the highest field end that software 
>>>> knows,
>>>> +   it should assume that additional fields are correct, @header_length is
>>>> +   correct and keep @header_length and 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.
>>>> +
>>>> +        < ... No additional fields in the header currently ... >
>>>> +
>>>
>>> I'm still not seeing the value in adding any of this paragraph to the spec. 
>>>  Maybe in the commit message that accompanies the spec change, but the spec 
>>> is clear enough if it documents how optional header fields are to be 
>>> managed (treat as 0 if missing, preserve on write if unknown, and with a 
>>> mandated alignment to avoid having to worry about other issues).
>>>
>>>>   Directly after the image header, optional sections called header 
>>>> extensions can
>>>>   be stored. Each extension has a structure like the following:
>>>>
>>>
>>
>>
> 
> 


-- 
Best regards,
Vladimir