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 <vsement...@virtuozzo.com> >>>> --- >>>> 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