Hi, Thank you for the review and the advices also. I will make some corrections based on the review for v3. Here are the main changes: - Nested table rending in code blocks - monospace usage for all filenames, fields name and function references - rendering of "\0" character using ``
Thanks, Souleymane Conte Le lun. 19 mai 2025 à 15:37, Peter Maydell <peter.mayd...@linaro.org> a écrit : > 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 >
