Re: [PATCH v7 1/2] docs: improve qcow2 spec about extending image header

[Eric Blake]

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 <address@hidden>
> ---
>   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.

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

> +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.  Ideally, we add fields so 
infrequently that backporting doesn't have to worry about backporting 
field 2 while skipping field 1.

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

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

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