Re: [PATCH v8 1/3] docs: improve qcow2 spec about extending image header

[Eric Blake]

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.
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.
3. An explicit value of 0 will have the same behavior as when the field 
is not present.


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

--
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org