[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: |
Wed, 6 Nov 2019 16:17:24 +0000 |
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
- Re: [PATCH v8 1/3] docs: improve qcow2 spec about extending image header,
Vladimir Sementsov-Ogievskiy <=