Re: [PATCH v2 1/1] docs/interop: convert text files to restructuredText

[Peter Maydell <peter.maydell@linaro.org>]

On Fri, 16 May 2025 at 16:01, <conte.souleymane@gmail.com> wrote:
>
> From: Souleymane Conte <conte.souleymane@gmail.com>
>
> buglink: https://gitlab.com/qemu-project/qemu/-/issues/527
>
> Signed-off-by: Souleymane Conte <conte.souleymane@gmail.com>

Thanks for this patch. It looks good, so I only have
some minor changes to suggest below.

A minor thing: for a single patch, please don't send a
cover letter -- it confuses some of our automated tooling.
Cover letters are only needed for multi-patch series.

For a single patch, if you want to add notes about e.g.
changes between v1 and v2, you can put them below the '---'
line...

> ---

...here. Text here won't go into the final git commit
message.

>  docs/interop/index.rst                |   1 +
>  docs/interop/{qcow2.txt => qcow2.rst} | 218 +++++++++++++++-----------
>  2 files changed, 128 insertions(+), 91 deletions(-)
>  rename docs/interop/{qcow2.txt => qcow2.rst} (88%)
>
> diff --git a/docs/interop/index.rst b/docs/interop/index.rst
> index 999e44eae1..bfe3cf0beb 100644
> --- a/docs/interop/index.rst
> +++ b/docs/interop/index.rst
> @@ -23,6 +23,7 @@ are useful for making QEMU interoperate with other software.
>     qemu-ga-ref
>     qemu-qmp-ref
>     qemu-storage-daemon-qmp-ref
> +   qcow2

I think I would put this a little further up in the
index, just below "parallels". That way it goes together
with the other image-format specs in the index list.

>     vhost-user
>     vhost-user-gpu
>     vhost-vdpa
> diff --git a/docs/interop/qcow2.txt b/docs/interop/qcow2.rst
> similarity index 88%
> rename from docs/interop/qcow2.txt
> rename to docs/interop/qcow2.rst
> index 2c4618375a..2295dd4ae6 100644
> --- a/docs/interop/qcow2.txt
> +++ b/docs/interop/qcow2.rst
> @@ -1,6 +1,8 @@
> -== General ==
> +================
> +Qcow2 Image File
> +================
>
> -A qcow2 image file is organized in units of constant size, which are called
> +A *qcow2* image file is organized in units of constant size, which are called
>  (host) clusters. A cluster is the unit in which all allocations are done,
>  both for actual guest data and for image metadata.
>
> @@ -9,10 +11,10 @@ clusters of the same size.
>
>  All numbers in qcow2 are stored in Big Endian byte order.
>
> +Header
> +------
>
> -== Header ==
> -
> -The first cluster of a qcow2 image contains the file header:
> +The first cluster of a qcow2 image contains the file header::
>
>      Byte  0 -  3:   magic
>                      QCOW magic string ("QFI\xfb")
> @@ -38,7 +40,7 @@ The first cluster of a qcow2 image contains the file header:
>                      within a cluster (1 << cluster_bits is the cluster size).
>                      Must not be less than 9 (i.e. 512 byte clusters).
>
> -                    Note: qemu as of today has an implementation limit of 2 MB
> +                    Note: Qemu as of today has an implementation limit of 2 MB

If we're going to change this kind of thing: the correct spelling
is all-caps "QEMU".

>                      as the maximum cluster size and won't be able to open images
>                      with larger cluster sizes.
>
> @@ -48,7 +50,7 @@ The first cluster of a qcow2 image contains the file header:
>           24 - 31:   size
>                      Virtual disk size in bytes.
>
> -                    Note: qemu has an implementation limit of 32 MB as
> +                    Note: Qemu has an implementation limit of 32 MB as
>                      the maximum L1 table size.  With a 2 MB cluster
>                      size, it is unable to populate a virtual cluster
>                      beyond 2 EB (61 bits); with a 512 byte cluster
> @@ -88,6 +90,7 @@ The first cluster of a qcow2 image contains the file header:
>  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
> @@ -185,7 +188,8 @@ the next fields through header_length.
>                      of 8.
>
>
> -=== Additional fields (version 3 and higher) ===
> +Additional fields (version 3 and higher)
> +----------------------------------------
>
>  In general, these fields are optional and may be safely ignored by the software,
>  as well as filled by zeros (which is equal to field absence), if software needs
> @@ -193,21 +197,25 @@ to set field B, but does not care about field A which precedes B. More
>  formally, additional fields have the following compatibility rules:
>
>  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.
> +   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 unknown
> -additional field may be safely ignored other than preserving its value when
> -rewriting the image header.
> +   additional field may be safely ignored other than preserving its value when
> +   rewriting the image header.
> +
> +.. _ref_rules_3:
>
>  3. An explicit value of 0 will have the same behavior as when the field is not
> -present*, if not altered by a specific incompatible bit.
> +   present*, if not altered by a specific incompatible bit.
>
> -*. A field is considered not present when header_length is less than or equal
> +(*) A field is considered not present when *header_length* is less than or equal
>  to the field's offset. Also, all additional fields are not present for
>  version 2.

For references in formatted text to things like field names, function
names, filenames and similar fixed-strings, I recommend ``header_length``
(which gives monospaced) rather than *header_length* (which is italics)
or **header_length** (which is bold).

> -              104:  compression_type
> +::
> +
> +        104:        compression_type
>
>                      Defines the compression method used for compressed clusters.
>                      All compressed clusters in an image use the same compression
> @@ -219,28 +227,30 @@ version 2.
>                      or must be zero (which means deflate).
>
>                      Available compression type values:
> -                        0: deflate <https://www.ietf.org/rfc/rfc1951.txt>
> -                        1: zstd <http://github.com/facebook/zstd>
> +                       - 0: deflate https://www.ietf.org/rfc/rfc1951.txt
> +                       - 1: zstd <http://github.com/facebook/zstd>

We should be consistent about whether we have the angle brackets
or not.

This is all in a fixed-width text block, so the URL isn't going
to be formatted anyway. Might as well leave the angleb rackets in,
for consistency with e.g. below.

>
> -                    The deflate compression type is called "zlib"
> +                    The deflate compression type is called zlib

Any reason to drop the quotes here ?

>                      <https://www.zlib.net/> in QEMU. However, clusters with the
>                      deflate compression type do not have zlib headers.
>
>          105 - 111:  Padding, contents defined below.
>
> -=== Header padding ===
> +Header padding
> +--------------
>
> -@header_length must be a multiple of 8, which means that if the end of the last
> +*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
> +the padding, it will be interpreted accordingly to point `[3.] <#ref_rules_3>`_ of the previous
>  paragraph, i.e.  in the same manner as when this field is not present.
>
>
> -=== Header extensions ===
> +Header extensions
> +-----------------
>
>  Directly after the image header, optional sections called header extensions can
> -be stored. Each extension has a structure like the following:
> +be stored. Each extension has a structure like the following::
>
>      Byte  0 -  3:   Header extension type:
>                          0x00000000 - End of the header extension area
> @@ -270,17 +280,19 @@ data of compatible features that it doesn't support. Compatible features that
>  need space for additional data can use a header extension.
>
>
> -== String header extensions ==
> +String header extensions
> +------------------------
>
>  Some header extensions (such as the backing file format name and the external
>  data file name) are just a single string. In this case, the header extension
>  length is the string length and the string is not '\0' terminated. (The header
> -extension padding can make it look like a string is '\0' terminated, but
> +extension padding can make it looks like a string is '\0' terminated, but

The original text is correct: this should be "make it look like".

Sphinx will interpret the '\' in '\0' as an escape, so it renders this
as "'0' terminated". If you write this with the ``...`` for fixed-width
text:
    make it look like a string is ``\0`` terminated
then Sphinx will output the backslash as a literal in the output.


> @@ -377,35 +392,42 @@ Logically the layout looks like
>
>    +-----------------------------+
>    | QCow2 header                |
> +  +-----------------------------+
>    | QCow2 header extension X    |
> +  +-----------------------------+
>    | QCow2 header extension FDE  |
> +  +-----------------------------+
>    | QCow2 header extension ...  |
> +  +-----------------------------+
>    | QCow2 header extension Z    |
>    +-----------------------------+
> +  | ...                         |
> +  +-----------------------------+
> +  | ...                         |
> +  +-----------------------------+
>    | ....other QCow2 tables....  |
> -  .                             .
> -  .                             .
>    +-----------------------------+
> -  | +-------------------------+ |
> -  | | LUKS partition header   | |
> -  | +-------------------------+ |
> -  | | LUKS key material 1     | |
> -  | +-------------------------+ |
> -  | | LUKS key material 2     | |
> -  | +-------------------------+ |
> -  | | LUKS key material ...   | |
> -  | +-------------------------+ |
> -  | | LUKS key material 8     | |
> -  | +-------------------------+ |
> +  | +------------------------+  |
> +  | | LUKS partition header  |  |
> +  | +------------------------+  |
> +  | | LUKS key material 1    |  |
> +  | +------------------------+  |
> +  | | LUKS key material 2    |  |
> +  | +------------------------+  |
> +  | | LUKS key material ...  |  |
> +  | +------------------------+  |
> +  | | LUKS key material 8    |  |
> +  | +------------------------+  |
> +  +-----------------------------+

This does not seem to render as a nested table.

Probably the simplest thing is to use the "::" syntax to
keep this as ASCII art, similar to the way you've handled
the other fixed-width tables in this document.

> +  |   QCow2 cluster payload     |
> +  +-----------------------------+
> +  |                             |
>    +-----------------------------+
> -  | QCow2 cluster payload       |
> -  .                             .
> -  .                             .
> -  .                             .
>    |                             |
>    +-----------------------------+
>

>
> -Refcount block entry (x = refcount_bits - 1):
> +Refcount block entry (x = refcount_bits - 1)::

You could put ``(x = refcount_bits - 1)`` to make the code fragment
render in monospace.


> -Compressed Clusters Descriptor (x = 62 - (cluster_bits - 8)):
> +Compressed Clusters Descriptor (x = 62 - (cluster_bits - 8))::

and similarly here.

thanks
-- PMM