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

[Max Reitz]

On 20.01.20 20:42, Eric Blake wrote:
> On 1/20/20 11:13 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:
> 
> s/so for/so that for/
> 
>>
>> 1. Software may support some of these optional fields and ignore the
>>     others, which means that features may be backported to downstream
>>     Qemu independently.
>> 2. 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.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
>> ---
>>   docs/interop/qcow2.txt | 44 +++++++++++++++++++++++++++++++++++++++---
>>   1 file changed, 41 insertions(+), 3 deletions(-)
>>
>> diff --git a/docs/interop/qcow2.txt b/docs/interop/qcow2.txt
>> index af5711e533..355925c35e 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, 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,44 @@ 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 and must be
>> a multiple
>> +                    of 8.
>> +
>> +
>> +=== Additional fields (version 3 and higher) ===
>> +
>> +In general, these fields are optional and may be safely ignored by
>> the software,
>> +as well as filled by zeros (which is equal to field absence), if
>> software needs
> 
> We're inconsistent on 'zeros' (git grep has 201 hits) vs. 'zeroes' (688
> hits); I prefer the latter, but won't object if you don't tweak it since
> this is the first use of either spelling in qcow2.txt.
> 
>> +to set field B, but does not care about field A, which precedes B. More
> 
> s/A, which/A which/

I’ve heard before that one should always use a comma before “which” and
never before “that” (in that a subordinate clause opened by “that” is a
mandatory description, whereas those that start with “why” are not
necessary for understanding).

So if this is a mandatory description (which I suppose it is), shouldn’t
it also be s/which/that/?

I suppose “field A that precedes B” sounds a bit weird because “A”
hasn’t been introduced before.  That is, “the field that precedes B”
would sound more natural.  Or is that precisely the kind of instance
where one would use “which” without comma? :-)

All in all, I was wondering whether there isn’t a more natural way to
rephrase the whole paragraph.  (No, I don’t have an excuse why I didn’t
say so in the last revision.)

Maybe:

In general, these fields are optional and may be safely ignored when
read and filled with zeroes when written.  For example, say software
wants to set field B but does not care about its preceding field A.  It
may then set A to zero, B to its desired value, and adjust header_length
to include A and B.

?

Max

signature.asc
Description: OpenPGP digital signature