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