07.11.2019 15:26, Vladimir Sementsov-Ogievskiy wrote: > 06.11.2019 22:19, Eric Blake wrote: >> On 10/18/19 9:36 AM, Vladimir Sementsov-Ogievskiy wrote: >> >>>> 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? >> >> Not necessarily. Rather, if the value of an unknown field can be safely >> ignored, then it should default to 0. If it cannot be safely ignored, then >> that field will not be set to a non-zero value without also setting an >> incompatible feature flag, so that software that does not know how to >> interpret the field will fail to load the image because it also fails to >> recognize the associated new incompatible feature bit. >> >> But I'd really like Kevin's opinion on how much wording is worth adding. >> >>> >>> So, may be >>> >>> Additional fields has the following compatible behavior by default: >> >> s/has/have/ >> >>> >>> 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. >> >> Both good. >> >>> >>> 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. >> >> Rather wordy but seems accurate. Perhaps compress it to: >> >> 3. Any additional field whose value must not be ignored for correct handling >> of the file will be accompanied by a corresponding incompatible feature bit. >> >> and maybe even reorder it to list the points as: >> >> Additional fields have the following compatibility rules: >> 1. Any additional field whose value must not be ignored for correct handling >> of the file will be accompanied by a corresponding incompatible feature bit. > > I'd like to stress, that incompatible bit is needed for incompatible value, > not for the field itself. (So field may be accompanied by incompatible bit > for some > it's values and for others - not), So, what about > > 1. If the value of the additional field must not be ignored for correct > handling of the file, it will be accompanied by a corresponding incompatible > feature bit. > >> 2. If there are no unrecognized incompatible feature bits set, an additional >> field may be safely ignored other than preserving its value when rewriting >> the image header. > > Sounds like we can ignore the field if we know its meaning and know its > incompatible bit.. > > 2. If there are no unrecognized incompatible feature bits set, an unknown > additional field may be safely ignored other than preserving its value when > rewriting the image header. > >> 3. An explicit value of 0 will have the same behavior as when the field is >> not present. > > But it may be changed by incompatible bit.. > > 3. An explicit value of 0 will have the same behavior as when the field is > not present, if not altered by specific incompatible bit. >
Eric, OK for you? >> >> >>>>> +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. >> >> As long as we never add new fields that are not 8-byte aligned (including >> any explicit padding), then we will never have the case of dividing fields >> in the middle by keeping the header length a multiple of 8. >> > > OK. > -- Best regards, Vladimir