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