Re: [PATCH v1 1/8] qcow2: introduce compression type feature

[Vladimir Sementsov-Ogievskiy]

27.02.2020 17:13, Eric Blake wrote:
> On 2/27/20 7:59 AM, Vladimir Sementsov-Ogievskiy wrote:
>
> > > Hmm - I think it may be worth a tweak to qcow2.txt to call out:
> > >
> > > 104: compression_type
> > > 105 - 111: padding, must be 0
> > >
> > > or else call out:
> > >
> > > 104-111: compression type
> > >
> > > and just blindly use all 8 bytes for the value even though really only 1 or two 
> > > values will ever be defined.  Of course, that moves the byte in question from 
> > > 104 to 111, thanks to our big endian encoding, but as this series is the first 
> > > one installing a non-zero value in those 8 bytes, and as we just finished 
> > > documenting that the header length must be a multiple of 8, there is no real 
> > > impact - we can make such tweaks up until the 5.0 release.
> >
> > But what is the benefit? We have already documented padding in the spec, and 
> > discussed it so much time... What is the problem with padding? And why to add 8 
> > bytes for every new feature which needs only one byte?
>
> Okay, so requiring an 8-byte field is not necessary.  But still, at least 
> mentioning padding bytes (that may be assigned meanings later) is consistent 
> with the rest of the document (for example, we have padding bits documented for 
> the compatible/incompatible/autoclear feature bits), and reminds implementers 
> to keep size rounded to a multiple of 8.

Yes, we can add something about it.. But I'm not sure we need, and I can't 
imaging correct short wording.


We have section about padding:

"
=== Header padding ===

@header_length must be a multiple of 8, which means that if the end of the last
additional field is not aligned, some padding is needed. This padding must be
zeroed, so that if some existing (or future) additional field will fall into
the padding, it will be interpreted accordingly to point [3.] of the previous
paragraph, i.e.  in the same manner as when this field is not present.
"


So, if we want to add something about 104-111, it should be added to this section, not to 
previous "=== Additional fields (version 3 and higher) ===".

And, if we want, to add something, we should consider both cases when 
compression type field exists and when not... What to write?

"105 - 111: These bytes are padding, if header length > 104. May be turned into new 
additional fields in future."

Sounds a bit strange... Keeping in mind that different versions of qemu may 
consider the same bytes as additional field or as padding, and it is correct.


--
Best regards,
Vladimir