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

[Vladimir Sementsov-Ogievskiy]

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?

> 
>> +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