Re: [PATCH v2] Consider discard option when writing zeros
On Wed, Jun 19, 2024 at 8:40 PM Nir Soffer wrote: > - Need to run all block tests > Stale note, make check pass
Re: [PATCH v2] Consider discard option when writing zeros
Tested using:
$ cat test-unmap.sh
#!/bin/sh
qemu=${1:?Usage: $0 qemu-executable}
img=/tmp/test.raw
echo
echo "defaults - write zeroes"
fallocate -l 1m $img
echo -e 'qemu-io none0 "write -z 0 1m"\nquit' | $qemu -monitor stdio \
-drive if=none,file=$img,format=raw >/dev/null
du -sh $img
echo
echo "defaults - write zeroes unmap"
fallocate -l 1m $img
echo -e 'qemu-io none0 "write -zu 0 1m"\nquit' | $qemu -monitor stdio \
-drive if=none,file=$img,format=raw >/dev/null
du -sh $img
echo
echo "defaults - write actual zeros"
fallocate -l 1m $img
echo -e 'qemu-io none0 "write -P 0 0 1m"\nquit' | $qemu -monitor stdio \
-drive if=none,file=$img,format=raw >/dev/null
du -sh $img
echo
echo "discard=off - write zeroes unmap"
fallocate -l 1m $img
echo -e 'qemu-io none0 "write -zu 0 1m"\nquit' | $qemu -monitor stdio \
-drive if=none,file=$img,format=raw,discard=off >/dev/null
du -sh $img
echo
echo "detect-zeros=on - write actual zeros"
fallocate -l 1m $img
echo -e 'qemu-io none0 "write -P 0 0 1m"\nquit' | $qemu -monitor stdio \
-drive if=none,file=$img,format=raw,detect-zeroes=on >/dev/null
du -sh $img
echo
echo "detect-zeros=unmap,discard=unmap - write actual zeros"
fallocate -l 1m $img
echo -e 'qemu-io none0 "write -P 0 0 1m"\nquit' | $qemu -monitor stdio \
-drive if=none,file=$img,format=raw,detect-zeroes=unmap,discard=unmap
>/dev/null
du -sh $img
echo
echo "discard=unmap - write zeroes"
fallocate -l 1m $img
echo -e 'qemu-io none0 "write -z 0 1m"\nquit' | $qemu -monitor stdio \
-drive if=none,file=$img,format=raw,discard=unmap >/dev/null
du -sh $img
echo
echo "discard=unmap - write zeroes unmap"
fallocate -l 1m $img
echo -e 'qemu-io none0 "write -zu 0 1m"\nquit' | $qemu -monitor stdio \
-drive if=none,file=$img,format=raw,discard=unmap >/dev/null
du -sh $img
rm $img
Before this change:
$ cat before.out
defaults - write zeroes
1.0M /tmp/test.raw
defaults - write zeroes unmap
0 /tmp/test.raw
defaults - write actual zeros
1.0M /tmp/test.raw
discard=off - write zeroes unmap
0 /tmp/test.raw
detect-zeros=on - write actual zeros
1.0M /tmp/test.raw
detect-zeros=unmap,discard=unmap - write actual zeros
0 /tmp/test.raw
discard=unmap - write zeroes
1.0M /tmp/test.raw
discard=unmap - write zeroes unmap
0 /tmp/test.raw
[nsoffer build (consider-discard-option)]$
After this change:
$ cat after.out
defaults - write zeroes
1.0M /tmp/test.raw
defaults - write zeroes unmap
1.0M /tmp/test.raw
defaults - write actual zeros
1.0M /tmp/test.raw
discard=off - write zeroes unmap
1.0M /tmp/test.raw
detect-zeros=on - write actual zeros
1.0M /tmp/test.raw
detect-zeros=unmap,discard=unmap - write actual zeros
0 /tmp/test.raw
discard=unmap - write zeroes
1.0M /tmp/test.raw
discard=unmap - write zeroes unmap
0 /tmp/test.raw
Differences:
$ diff -u before.out after.out
--- before.out 2024-06-19 20:24:09.234083713 +0300
+++ after.out 2024-06-19 20:24:20.526165573 +0300
@@ -3,13 +3,13 @@
1.0M /tmp/test.raw
defaults - write zeroes unmap
-0 /tmp/test.raw
+1.0M /tmp/test.raw
defaults - write actual zeros
1.0M /tmp/test.raw
discard=off - write zeroes unmap
-0 /tmp/test.raw
+1.0M /tmp/test.raw
On Wed, Jun 19, 2024 at 8:40 PM Nir Soffer wrote:
> When opening an image with discard=off, we punch hole in the image when
> writing zeroes, making the image sparse. This breaks users that want to
> ensure that writes cannot fail with ENOSPACE by using fully allocated
> images.
>
> bdrv_co_pwrite_zeroes() correctly disable BDRV_REQ_MAY_UNMAP if we
> opened the child without discard=unmap or discard=on. But we don't go
> through this function when accessing the top node. Move the check down
> to bdrv_co_do_pwrite_zeroes() which seems to be used in all code paths.
>
> Issues:
> - We don't punch hole by default, so images are kept allocated. Before
> this change we punched holes by default. I'm not sure this is a good
> change in behavior.
> - Need to run all block tests
> - Not sure that we have tests covering unmapping, we may need new tests
> - We may need new tests to cover this change
>
> Signed-off-by: Nir Soffer
> ---
>
> Changes since v1:
> - Replace the incorrect has_discard change with the right fix
>
> v1 was here:
> https://lists.nongnu.org/archive/html/qemu-block/2024-06/msg00198.html
>
> block/io.c | 9 +
> 1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/block/io.c b/block/io.c
> index 7217cf811b..301514c880 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -1860,10 +1860,15 @@ bdrv_co_do_pwrite_zeroes(BlockDriverState *bs,
> int64_t offset, int64_t bytes,
> /* By definition there is no user buffer so this flag doesn't make
> sense */
> if (flags & BDRV_REQ_REGISTERED_BUF) {
> return -EINVAL;
> }
>
> +/* If opened with discard=off we should never unmap. */
> +if (!(bs->open_flags & BDRV_O_UNMAP)) {
> +flags &= ~BDRV_REQ_MAY_UNMAP;
> +}
> +
> /* Invalidate the cached block-status data range if this
[PATCH v2] Consider discard option when writing zeros
When opening an image with discard=off, we punch hole in the image when
writing zeroes, making the image sparse. This breaks users that want to
ensure that writes cannot fail with ENOSPACE by using fully allocated
images.
bdrv_co_pwrite_zeroes() correctly disable BDRV_REQ_MAY_UNMAP if we
opened the child without discard=unmap or discard=on. But we don't go
through this function when accessing the top node. Move the check down
to bdrv_co_do_pwrite_zeroes() which seems to be used in all code paths.
Issues:
- We don't punch hole by default, so images are kept allocated. Before
this change we punched holes by default. I'm not sure this is a good
change in behavior.
- Need to run all block tests
- Not sure that we have tests covering unmapping, we may need new tests
- We may need new tests to cover this change
Signed-off-by: Nir Soffer
---
Changes since v1:
- Replace the incorrect has_discard change with the right fix
v1 was here:
https://lists.nongnu.org/archive/html/qemu-block/2024-06/msg00198.html
block/io.c | 9 +
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/block/io.c b/block/io.c
index 7217cf811b..301514c880 100644
--- a/block/io.c
+++ b/block/io.c
@@ -1860,10 +1860,15 @@ bdrv_co_do_pwrite_zeroes(BlockDriverState *bs, int64_t
offset, int64_t bytes,
/* By definition there is no user buffer so this flag doesn't make sense */
if (flags & BDRV_REQ_REGISTERED_BUF) {
return -EINVAL;
}
+/* If opened with discard=off we should never unmap. */
+if (!(bs->open_flags & BDRV_O_UNMAP)) {
+flags &= ~BDRV_REQ_MAY_UNMAP;
+}
+
/* Invalidate the cached block-status data range if this write overlaps */
bdrv_bsc_invalidate_range(bs, offset, bytes);
assert(alignment % bs->bl.request_alignment == 0);
head = offset % alignment;
@@ -2313,14 +2318,10 @@ int coroutine_fn bdrv_co_pwrite_zeroes(BdrvChild
*child, int64_t offset,
{
IO_CODE();
trace_bdrv_co_pwrite_zeroes(child->bs, offset, bytes, flags);
assert_bdrv_graph_readable();
-if (!(child->bs->open_flags & BDRV_O_UNMAP)) {
-flags &= ~BDRV_REQ_MAY_UNMAP;
-}
-
return bdrv_co_pwritev(child, offset, bytes, NULL,
BDRV_REQ_ZERO_WRITE | flags);
}
/*
--
2.45.1
Re: [PATCH 13/13] qapi: convert "Example" sections to rST
On Wed, Jun 19, 2024, 9:20 AM Markus Armbruster wrote:
> John Snow writes:
>
> > Eliminate the "Example" sections in QAPI doc blocks, converting them
> > into QMP example code blocks. This is generally done in this patch by
> > converting "Example:" or "Examples:" lines into ".. code-block:: QMP"
> > lines.
> >
> > The old "Example:" or "Examples:" syntax is now caught as an error; but
> > with the previous commit, "Example::" is still permitted as explicit rST
> > syntax. ('Example' is not special in this case; any sentence that ends
> > with "::" will start an indented code block in rST.)
> >
> > The ".. code-block:: QMP" form explicitly applies the QMP lexer and will
> > loosely validate an example as valid QMP/JSON. The "::" form does not
> > apply any lexer in particular and will not emit any errors.
> >
> > It is possible to choose the QMP lexer with the "::" form by using the
> > Sphinx directive ".. highlight:: QMP" in the document above where the
> > example appears; but this changes the lexer for *all* subsequent "::"
> > style code-blocks in the document thereafter.
> >
> > This patch does not change the default lexer for the legacy qapidoc
> > generator documents; future patches for the new qapidoc generator *may*
> > change this default.
> >
> > This patch has several benefits:
> >
> > 1. Example sections can now be written more arbitrarily, mixing
> >explanatory paragraphs and code blocks however desired.
> >
> > 2. "Example sections" can now use fully arbitrary rST.
>
> Do the double-quotes signify something I'm missing?
>
They aren't *sections* (QAPIDoc terminology) anymore, but was at a loss for
more precise phrasing.
> >
> > 3. All code blocks are now lexed and validated as QMP; increasing
> >usability of the docs and ensuring validity of example snippets.
> >
> >(To some extent - This patch only gaurantees it lexes correctly, not
> >that it's valid under the JSON or QMP grammars. It will catch most
> >small mistakes, however.)
> >
> > 4. Each code-block can be captioned independently without bypassing the
> >QMP lexer/validator.
> >
> >(i.e. code blocks are now for *code* only, so we don't have to
> >sacrifice annotations/captions for having lexicographically correct
> >examples.)
> >
> > For any sections with more than one example, examples are split up into
> > multiple code-block regions. If annotations are present, those
> > annotations are converted into code-block captions instead, e.g.
> >
> > ```
> > Examples:
> >
> >1. Lorem Ipsum
> >
> >-> { "foo": "bar" }
> > ```
> >
> > Is rewritten as:
> >
> > ```
> > .. code-block:: QMP
> >:caption: Example: Lorem Ipsum
> >
> >-> { "foo": "bar" }
> > ```
> >
> > This process was only semi-automated:
> >
> > 1. Replace "Examples?:" sections with sed:
> >
> > sed -i 's|# Example:|# .. code-block:: QMP|' *.json
> > sed -i 's|# Examples:|# .. code-block:: QMP|' *.json
> >
> > 2. Identify sections that no longer parse successfully by attempting the
> >doc build, convert annotations into captions manually.
> >(Tedious, oh well.)
> >
> > 3. Add captions where still needed:
> >
> > sed -zi 's|# .. code-block:: QMP\n#\n|# .. code-block:: QMP\n#
> :caption: Example\n#\n|g' *.json
> >
> > Not fully ideal, but hopefully not something that has to be done very
> > often. (Or ever again.)
> >
> > Signed-off-by: John Snow
> > Acked-by: Stefan Hajnoczi [for block*.json]
>
> [...]
>
> > diff --git a/qapi/pci.json b/qapi/pci.json
> > index f51159a2c4c..9192212661b 100644
> > --- a/qapi/pci.json
> > +++ b/qapi/pci.json
> > @@ -182,7 +182,8 @@
> > #
> > # Since: 0.14
> > #
> > -# Example:
> > +# .. code-block:: QMP
> > +#:caption: Example
> > #
> > # -> { "execute": "query-pci" }
> > # <- { "return": [
> > @@ -311,8 +312,7 @@
> > # ]
> > #}
> > #
> > -# Note: This example has been shortened as the real response is too
> > -# long.
> > +# This example has been shortened as the real response is too long.
>
> Squash into PATCH 09.
>
> > #
> > ##
> > { 'command': 'query-pci', 'returns': ['PciInfo'] }
>
> Otherwise looks good to me except for the somewhat ugly rendering in
> HTML mentioned in the commit message.
>
ACK
> [...]
>
>
Re: [PATCH 03/13] docs/qapidoc: delint a tiny portion of the module
On Wed, Jun 19, 2024, 2:28 AM Markus Armbruster wrote: > John Snow writes: > > > In a forthcoming series that adds a new QMP documentation generator, it > > will be helpful to have a linting baseline. However, there's no need to > > shuffle around the deck chairs too much, because most of this code will > > be removed once that new qapidoc generator (the "transmogrifier") is in > > place. > > > > To ease my pain: just turn off the black auto-formatter for most, but > > not all, of qapidoc.py. This will help ensure that *new* code follows a > > coding standard without bothering too much with cleaning up the existing > > code. > > > > Code that I intend to keep is still subject to the delinting beam. > > > > Signed-off-by: John Snow > > --- > > docs/sphinx/qapidoc.py | 66 +- > > 1 file changed, 40 insertions(+), 26 deletions(-) > > > > diff --git a/docs/sphinx/qapidoc.py b/docs/sphinx/qapidoc.py > > index f270b494f01..e675966defa 100644 > > --- a/docs/sphinx/qapidoc.py > > +++ b/docs/sphinx/qapidoc.py > > @@ -28,33 +28,42 @@ > > import re > > > > from docutils import nodes > > +from docutils.parsers.rst import Directive, directives > > from docutils.statemachine import ViewList > > -from docutils.parsers.rst import directives, Directive > > -from sphinx.errors import ExtensionError > > -from sphinx.util.nodes import nested_parse_with_titles > > -import sphinx > > -from qapi.gen import QAPISchemaVisitor > > from qapi.error import QAPIError, QAPISemError > > +from qapi.gen import QAPISchemaVisitor > > from qapi.schema import QAPISchema > > > > +import sphinx > > +from sphinx.errors import ExtensionError > > +from sphinx.util.nodes import nested_parse_with_titles > > + > > > > # Sphinx up to 1.6 uses AutodocReporter; 1.7 and later > > # use switch_source_input. Check borrowed from kerneldoc.py. > > -Use_SSI = sphinx.__version__[:3] >= '1.7' > > -if Use_SSI: > > +USE_SSI = sphinx.__version__[:3] >= "1.7" > > +if USE_SSI: > > from sphinx.util.docutils import switch_source_input > > else: > > -from sphinx.ext.autodoc import AutodocReporter > > +from sphinx.ext.autodoc import ( # pylint: > disable=no-name-in-module > > +AutodocReporter, > > +) > > > > > > -__version__ = '1.0' > > +__version__ = "1.0" > > > > > > +# Disable black auto-formatter until re-enabled: > > +# fmt: off > > + > > # Function borrowed from pydash, which is under the MIT license > > def intersperse(iterable, separator): > > """Yield the members of *iterable* interspersed with *separator*.""" > > iterable = iter(iterable) > > -yield next(iterable) > > +try: > > +yield next(iterable) > > +except StopIteration: > > +return > > This gets rid of pylint's > > docs/sphinx/qapidoc.py:82:10: R1708: Do not raise StopIteration in > generator, use return statement instead (stop-iteration-return) > > I considered the same change some time ago, and decided against it to > avoid deviating from pydash. StopIteration would be a programming error > here. > > Please *delete* the function instead: commit fd62bff901b removed its > last use. I'd do it in a separate commit, but that's up to you. > Oh! I didn't realize it wasn't being used. That's certainly easier :) > > for item in iterable: > > yield separator > > yield item > > @@ -451,6 +460,10 @@ def get_document_nodes(self): > > return self._top_node.children > > > > > > +# Turn the black formatter on for the rest of the file. > > +# fmt: on > > + > > + > > class QAPISchemaGenDepVisitor(QAPISchemaVisitor): > > """A QAPI schema visitor which adds Sphinx dependencies each module > > > > @@ -458,34 +471,34 @@ class QAPISchemaGenDepVisitor(QAPISchemaVisitor): > > that the generated documentation output depends on the input > > schema file associated with each module in the QAPI input. > > """ > > + > > def __init__(self, env, qapidir): > > self._env = env > > self._qapidir = qapidir > > > > def visit_module(self, name): > > if name != "./builtin": > > -qapifile = self._qapidir + '/' + name > > +qapifile = self._qapidir + "/" + name > > The string literal quote changes are mildly annoying. But since by your > good work you're effectively appointing yourself maintainer of this > file... ;) > Mildly. However, I do think black is "close enough" on most style issues that I have absolutely no regret or hesitation using it for all new python development. (I've been using it a lot in hobby code the last year and I find it to be remarkably helpful for my own consistency in style issues, it's indispensable for me.) So in this series, I start using it because I essentially wind up rewriting this entire file and wanted an autoformatter so I could shut my brain off for stuff like this. A "flag day" as you call it is likely coming soon to python/ where I'll start enforcing black
Re: [PATCH 13/13] qapi: convert "Example" sections to rST
John Snow writes:
> Eliminate the "Example" sections in QAPI doc blocks, converting them
> into QMP example code blocks. This is generally done in this patch by
> converting "Example:" or "Examples:" lines into ".. code-block:: QMP"
> lines.
>
> The old "Example:" or "Examples:" syntax is now caught as an error; but
> with the previous commit, "Example::" is still permitted as explicit rST
> syntax. ('Example' is not special in this case; any sentence that ends
> with "::" will start an indented code block in rST.)
>
> The ".. code-block:: QMP" form explicitly applies the QMP lexer and will
> loosely validate an example as valid QMP/JSON. The "::" form does not
> apply any lexer in particular and will not emit any errors.
>
> It is possible to choose the QMP lexer with the "::" form by using the
> Sphinx directive ".. highlight:: QMP" in the document above where the
> example appears; but this changes the lexer for *all* subsequent "::"
> style code-blocks in the document thereafter.
>
> This patch does not change the default lexer for the legacy qapidoc
> generator documents; future patches for the new qapidoc generator *may*
> change this default.
>
> This patch has several benefits:
>
> 1. Example sections can now be written more arbitrarily, mixing
>explanatory paragraphs and code blocks however desired.
>
> 2. "Example sections" can now use fully arbitrary rST.
Do the double-quotes signify something I'm missing?
>
> 3. All code blocks are now lexed and validated as QMP; increasing
>usability of the docs and ensuring validity of example snippets.
>
>(To some extent - This patch only gaurantees it lexes correctly, not
>that it's valid under the JSON or QMP grammars. It will catch most
>small mistakes, however.)
>
> 4. Each code-block can be captioned independently without bypassing the
>QMP lexer/validator.
>
>(i.e. code blocks are now for *code* only, so we don't have to
>sacrifice annotations/captions for having lexicographically correct
>examples.)
>
> For any sections with more than one example, examples are split up into
> multiple code-block regions. If annotations are present, those
> annotations are converted into code-block captions instead, e.g.
>
> ```
> Examples:
>
>1. Lorem Ipsum
>
>-> { "foo": "bar" }
> ```
>
> Is rewritten as:
>
> ```
> .. code-block:: QMP
>:caption: Example: Lorem Ipsum
>
>-> { "foo": "bar" }
> ```
>
> This process was only semi-automated:
>
> 1. Replace "Examples?:" sections with sed:
>
> sed -i 's|# Example:|# .. code-block:: QMP|' *.json
> sed -i 's|# Examples:|# .. code-block:: QMP|' *.json
>
> 2. Identify sections that no longer parse successfully by attempting the
>doc build, convert annotations into captions manually.
>(Tedious, oh well.)
>
> 3. Add captions where still needed:
>
> sed -zi 's|# .. code-block:: QMP\n#\n|# .. code-block:: QMP\n#:caption:
> Example\n#\n|g' *.json
>
> Not fully ideal, but hopefully not something that has to be done very
> often. (Or ever again.)
>
> Signed-off-by: John Snow
> Acked-by: Stefan Hajnoczi [for block*.json]
[...]
> diff --git a/qapi/pci.json b/qapi/pci.json
> index f51159a2c4c..9192212661b 100644
> --- a/qapi/pci.json
> +++ b/qapi/pci.json
> @@ -182,7 +182,8 @@
> #
> # Since: 0.14
> #
> -# Example:
> +# .. code-block:: QMP
> +#:caption: Example
> #
> # -> { "execute": "query-pci" }
> # <- { "return": [
> @@ -311,8 +312,7 @@
> # ]
> #}
> #
> -# Note: This example has been shortened as the real response is too
> -# long.
> +# This example has been shortened as the real response is too long.
Squash into PATCH 09.
> #
> ##
> { 'command': 'query-pci', 'returns': ['PciInfo'] }
Otherwise looks good to me except for the somewhat ugly rendering in
HTML mentioned in the commit message.
[...]
Re: [PATCH 09/13] qapi: convert "Note" sections to plain rST
John Snow writes:
> We do not need a dedicated section for notes. By eliminating a specially
> parsed section, these notes can be treated as normal rST paragraphs in
> the new QMP reference manual, and can be placed and styled much more
> flexibly.
>
> Convert all existing "Note" and "Notes" sections to pure rST. As part of
> the conversion, capitalize the first letter of each sentence and add
> trailing punctuation where appropriate to ensure notes look sensible and
> consistent in rendered HTML documentation.
>
> Update docs/devel/qapi-code-gen.rst to reflect the new paradigm, and ...
>
> ... Update the QAPI parser to prohibit "Note" sections while suggesting
> a new syntax. The exact formatting to use is a matter of taste, but a
> good candidate is simply:
>
> .. note:: lorem ipsum ...
>
> ... but there are other choices, too. The Sphinx readthedocs theme
> offers theming for the following forms (capitalization unimportant); all
> are adorned with a (!) symbol in the title bar for rendered HTML docs.
>
> See
> https://sphinx-rtd-theme.readthedocs.io/en/stable/demo/demo.html#admonitions
> for examples of each directive/admonition in use.
>
> These are rendered in orange:
>
> .. Attention:: ...
> .. Caution:: ...
> .. WARNING:: ...
>
> These are rendered in red:
>
> .. DANGER:: ...
> .. Error:: ...
>
> These are rendered in green:
>
> .. Hint:: ...
> .. Important:: ...
> .. Tip:: ...
>
> These are rendered in blue:
>
> .. Note:: ...
> .. admonition:: custom title
>
>admonition body text
>
> This patch uses ".. note::" almost everywhere, with just two "caution"
> directives. ".. admonition:: notes" is used in a few places where we had
> an ordered list of multiple notes that would not make sense as
> standalone/separate admonitions.
>
> Signed-off-by: John Snow
> Acked-by: Stefan Hajnoczi [for block*.json]
[...]
> diff --git a/qapi/control.json b/qapi/control.json
> index 10c906fa0e7..59d5e00c151 100644
> --- a/qapi/control.json
> +++ b/qapi/control.json
> @@ -22,14 +22,13 @@
> # "arguments": { "enable": [ "oob" ] } }
> # <- { "return": {} }
> #
> -# Notes: This command is valid exactly when first connecting: it must
> -# be issued before any other command will be accepted, and will
> -# fail once the monitor is accepting other commands. (see qemu
> -# docs/interop/qmp-spec.rst)
> +# .. note:: This command is valid exactly when first connecting: it must
> +#be issued before any other command will be accepted, and will fail
> +#once the monitor is accepting other commands. (see qemu
> +#docs/interop/qmp-spec.rst)
> #
> -# The QMP client needs to explicitly enable QMP capabilities,
> -# otherwise all the QMP capabilities will be turned off by
> -# default.
> +# .. note:: The QMP client needs to explicitly enable QMP capabilities,
> +#otherwise all the QMP capabilities will be turned off by default.
> #
> # Since: 0.13
> ##
> @@ -150,8 +149,8 @@
> # ]
> #}
> #
> -# Note: This example has been shortened as the real response is too
> -# long.
> +# This example has been shortened as the real response is too long.
> +#
Here's one way to transform the elision note, ...
> ##
> { 'command': 'query-commands', 'returns': ['CommandInfo'],
>'allow-preconfig': true }
[...]
> diff --git a/qapi/pci.json b/qapi/pci.json
> index 08bf6958634..f51159a2c4c 100644
> --- a/qapi/pci.json
> +++ b/qapi/pci.json
> @@ -146,8 +146,8 @@
> #
> # @regions: a list of the PCI I/O regions associated with the device
> #
> -# Notes: the contents of @class_info.desc are not stable and should
> -# only be treated as informational.
> +# .. note:: The contents of @class_info.desc are not stable and should
> +#only be treated as informational.
> #
> # Since: 0.14
> ##
> @@ -311,7 +311,8 @@
> # ]
> #}
> #
> -# Note: This example has been shortened as the real response is too
> +# Note: This example has been shortened as the real response is too
> # long.
> +#
... and here's another way. Same way would be better, wouldn't it?
They actually are after PATCH 13:
-# Note: This example has been shortened as the real response is too
-# long.
+# This example has been shortened as the real response is too long.
Move that hunk here, please.
> ##
> { 'command': 'query-pci', 'returns': ['PciInfo'] }
[...]
Re: [PATCH 12/13] qapi/parser: don't parse rST markup as section headers
John Snow writes: > The double-colon synax is rST formatting that precedes a literal code > block. We do not want to capture these as QAPI-specific sections. > > Coerce blocks that start with e.g. "Example::" to be parsed as untagged > paragraphs instead of special tagged sections. > > Signed-off-by: John Snow > --- > scripts/qapi/parser.py | 9 +++-- > 1 file changed, 7 insertions(+), 2 deletions(-) > > diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py > index 53b06a94508..971fdf61a09 100644 > --- a/scripts/qapi/parser.py > +++ b/scripts/qapi/parser.py > @@ -545,10 +545,15 @@ def get_doc(self) -> 'QAPIDoc': > line = self.get_doc_indented(doc) > no_more_args = True > elif match := re.match( > -r'(Returns|Errors|Since|Notes?|Examples?|TODO): *', > -line): > +r'(Returns|Errors|Since|Notes?|Examples?|TODO)' > +r'(?!::): *', > +line, > +): > # tagged section > > +# Note: "sections" with two colons are left alone as > +# rST markup and not interpreted as a section heading. > + > # TODO: Remove this error sometime in 2025 or so > # after we've fully transitioned to the new qapidoc > # generator. Patch looks good, but let's add a positive test to doc-good.json.
Re: [PATCH 09/13] qapi: convert "Note" sections to plain rST
John Snow writes:
> We do not need a dedicated section for notes. By eliminating a specially
> parsed section, these notes can be treated as normal rST paragraphs in
> the new QMP reference manual, and can be placed and styled much more
> flexibly.
>
> Convert all existing "Note" and "Notes" sections to pure rST. As part of
> the conversion, capitalize the first letter of each sentence and add
> trailing punctuation where appropriate to ensure notes look sensible and
> consistent in rendered HTML documentation.
>
> Update docs/devel/qapi-code-gen.rst to reflect the new paradigm, and ...
>
> ... Update the QAPI parser to prohibit "Note" sections while suggesting
Scratch "... ..." and downcase "Update"?
> a new syntax. The exact formatting to use is a matter of taste, but a
> good candidate is simply:
>
> .. note:: lorem ipsum ...
>
> ... but there are other choices, too. The Sphinx readthedocs theme
> offers theming for the following forms (capitalization unimportant); all
> are adorned with a (!) symbol in the title bar for rendered HTML docs.
>
> See
> https://sphinx-rtd-theme.readthedocs.io/en/stable/demo/demo.html#admonitions
> for examples of each directive/admonition in use.
>
> These are rendered in orange:
>
> .. Attention:: ...
> .. Caution:: ...
> .. WARNING:: ...
>
> These are rendered in red:
>
> .. DANGER:: ...
> .. Error:: ...
>
> These are rendered in green:
>
> .. Hint:: ...
> .. Important:: ...
> .. Tip:: ...
>
> These are rendered in blue:
>
> .. Note:: ...
> .. admonition:: custom title
>
>admonition body text
>
> This patch uses ".. note::" almost everywhere, with just two "caution"
> directives. ".. admonition:: notes" is used in a few places where we had
> an ordered list of multiple notes that would not make sense as
> standalone/separate admonitions.
>
> Signed-off-by: John Snow
> Acked-by: Stefan Hajnoczi [for block*.json]
[...]
> diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
> index b3de1fb6b3a..57598331c5c 100644
> --- a/qga/qapi-schema.json
> +++ b/qga/qapi-schema.json
> @@ -422,8 +422,9 @@
> # Returns: GuestFsfreezeStatus ("thawed", "frozen", etc., as defined
> # below)
> #
> -# Note: This may fail to properly report the current state as a result
> -# of some other guest processes having issued an fs freeze/thaw.
> +# .. note:: This may fail to properly report the current state as a
> +#result of some other guest processes having issued an fs
> +#freeze/thaw.
> #
> # Since: 0.15.0
> ##
> @@ -443,9 +444,9 @@
> #
> # Returns: Number of file systems currently frozen.
> #
> -# Note: On Windows, the command is implemented with the help of a
> -# Volume Shadow-copy Service DLL helper. The frozen state is
> -# limited for up to 10 seconds by VSS.
> +# .. note:: On Windows, the command is implemented with the help of a
> +#Volume Shadow-copy Service DLL helper. The frozen state is limited
> +#for up to 10 seconds by VSS.
> #
> # Since: 0.15.0
> ##
> @@ -479,10 +480,10 @@
> #
> # Returns: Number of file systems thawed by this call
> #
> -# Note: if return value does not match the previous call to
> -# guest-fsfreeze-freeze, this likely means some freezable
> -# filesystems were unfrozen before this call, and that the
> -# filesystem state may have changed before issuing this command.
> +# .. note:: If return value does not match the previous call to
> +#guest-fsfreeze-freeze, this likely means some freezable filesystems
> +#were unfrozen before this call, and that the filesystem state may
> +#have changed before issuing this command.
> #
> # Since: 0.15.0
> ##
> @@ -560,8 +561,8 @@
> # Errors:
> # - If suspend to disk is not supported, Unsupported
> #
> -# Notes: It's strongly recommended to issue the guest-sync command
> -# before sending commands when the guest resumes
> +# .. note:: It's strongly recommended to issue the guest-sync command
> +#before sending commands when the guest resumes.
> #
> # Since: 1.1
> ##
> @@ -596,8 +597,8 @@
> # Errors:
> # - If suspend to ram is not supported, Unsupported
> #
> -# Notes: It's strongly recommended to issue the guest-sync command
> -# before sending commands when the guest resumes
> +# .. note:: It's strongly recommended to issue the guest-sync command
> +#before sending commands when the guest resumes.
> #
> # Since: 1.1
> ##
> @@ -631,8 +632,8 @@
> # Errors:
> # - If hybrid suspend is not supported, Unsupported
> #
> -# Notes: It's strongly recommended to issue the guest-sync command
> -# before sending commands when the guest resumes
> +# .. note:: It's strongly recommended to issue the guest-sync command
> +#before sending commands when the guest resumes.
> #
> # Since: 1.1
> ##
> @@ -1461,16 +1462,15 @@
> # * POSIX: as defined by os-release(5)
> # * Windows: contains string "server" or "client"
> #
> -# Notes: On POSIX systems the fields @id, @name, @pretty-name,
> -#
Re: [PATCH 11/13] qapi: add markup to note blocks
John Snow writes: > Generally, surround command-line options with ``literal`` markup to help > it stand out from prose in rendered HTML, and add cross-references to > replace "see also" messages. > > References to types, values, and other QAPI definitions are not yet > adjusted here; they will be converted en masse in a subsequent patch > after the new QAPI doc generator is merged. > > Signed-off-by: John Snow Reviewed-by: Markus Armbruster
Re: [PATCH 10/13] qapi: update prose in note blocks
John Snow writes: > Where I've noticed, rephrase the note to read more fluently. > > Signed-off-by: John Snow > --- > qapi/block-core.json | 4 ++-- > qga/qapi-schema.json | 2 +- > 2 files changed, 3 insertions(+), 3 deletions(-) > > diff --git a/qapi/block-core.json b/qapi/block-core.json > index cacedfb771c..9ef23ec02ae 100644 > --- a/qapi/block-core.json > +++ b/qapi/block-core.json > @@ -6048,9 +6048,9 @@ > # > # @name: the name of the internal snapshot to be created > # > -# .. note:: In transaction, if @name is empty, or any snapshot matching > +# .. note:: In a transaction, if @name is empty or any snapshot matching > #@name exists, the operation will fail. Only some image formats > -#support it, for example, qcow2, and rbd. > +#support it; for example, qcow2, and rbd. > # > # Since: 1.7 > ## > diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json > index 57598331c5c..1273d85bb5f 100644 > --- a/qga/qapi-schema.json > +++ b/qga/qapi-schema.json > @@ -480,7 +480,7 @@ > # > # Returns: Number of file systems thawed by this call > # > -# .. note:: If return value does not match the previous call to > +# .. note:: If the return value does not match the previous call to > #guest-fsfreeze-freeze, this likely means some freezable filesystems > #were unfrozen before this call, and that the filesystem state may > #have changed before issuing this command. Reviewed-by: Markus Armbruster
Re: [PATCH 08/13] qapi: ensure all errors sections are uniformly typset
John Snow writes: > Transactions have the only instance of an Errors section that isn't a > rST list; turn it into one. > > Signed-off-by: John Snow Let;s explain the "why" a bit more clearly. Maybe qapi: Nail down convention that Errors sections are lists By unstated convention, Errors sections are rST lists. Document the convention, and make the one exception conform. > --- > docs/devel/qapi-code-gen.rst | 7 +++ > qapi/transaction.json| 2 +- > 2 files changed, 8 insertions(+), 1 deletion(-) > > diff --git a/docs/devel/qapi-code-gen.rst b/docs/devel/qapi-code-gen.rst > index f453bd35465..cee43222f19 100644 > --- a/docs/devel/qapi-code-gen.rst > +++ b/docs/devel/qapi-code-gen.rst > @@ -1011,6 +1011,13 @@ like this:: > "Returns" and "Errors" sections are only valid for commands. They > document the success and the error response, respectively. > > +"Errors" sections should be formatted as an rST list, each entry > +detailing a relevant error condition. For example:: > + > + # Errors: > + # - If @device does not exist, DeviceNotFound > + # - Any other error returns a GenericError. > + > A "Since: x.y.z" tagged section lists the release that introduced the > definition. > > diff --git a/qapi/transaction.json b/qapi/transaction.json > index 5749c133d4a..07afc269d54 100644 > --- a/qapi/transaction.json > +++ b/qapi/transaction.json > @@ -235,7 +235,7 @@ > # additional detail. > # > # Errors: > -# Any errors from commands in the transaction > +# - Any errors from commands in the transaction > # > # Note: The transaction aborts on the first failure. Therefore, there > # will be information on only one failed operation returned in an Preferably with an improved commit message Reviewed-by: Markus Armbruster
Re: [PATCH 07/13] qapi: fix non-compliant JSON examples
John Snow writes: > The new QMP documentation generator wants to parse all examples as > "QMP". We have an existing QMP lexer in docs/sphinx/qmp_lexer.py (Seen > in-use here: https://qemu-project.gitlab.io/qemu/interop/bitmaps.html) > that allows the use of "->", "<-" and "..." tokens to denote QMP > protocol flow with elisions, but otherwise defers to the JSON lexer. > > To utilize this lexer for the existing QAPI documentation, we need them > to conform to a standard so that they lex and render correctly. Once the > QMP lexer is active for examples, errant QMP/JSON will produce warning > messages and fail the build. > > Fix any invalid JSON found in QAPI documentation (identified by > attempting to lex all examples as QMP; see subsequent commits). Further, > the QMP lexer still supports elisions, but they must be represented as > the value "...", so three examples have been adjusted to support that > format here. > > Signed-off-by: John Snow Reviewed-by: Markus Armbruster
Re: [PATCH 06/13] docs/qapidoc: fix nested parsing under untagged sections
John Snow writes:
> Sphinx does not like sections without titles, because it wants to
> convert every section into a reference. When there is no title, it
> struggles to do this and transforms the tree inproperly.
>
> Depending on the rST used, this may result in an assertion error deep in
> the docutils HTMLWriter.
>
> (Observed when using ".. admonition:: Notes" under such a section - When
> this is transformed with its own element, Sphinx is fooled into
> believing this title belongs to the section and incorrect mutates the
> docutils tree, leading to errors during rendering time.)
>
> When parsing an untagged section (free paragraphs), skip making a hollow
> section and instead append the parse results to the prior section.
>
> Many Bothans died to bring us this information.
>
> Signed-off-by: John Snow
> ---
> docs/sphinx/qapidoc.py | 16 +++-
> 1 file changed, 11 insertions(+), 5 deletions(-)
>
> diff --git a/docs/sphinx/qapidoc.py b/docs/sphinx/qapidoc.py
> index f2f2005dd5f..66cf254a624 100644
> --- a/docs/sphinx/qapidoc.py
> +++ b/docs/sphinx/qapidoc.py
> @@ -286,14 +286,20 @@ def _nodes_for_sections(self, doc):
> if section.tag and section.tag == 'TODO':
> # Hide TODO: sections
> continue
> +
> +if not section.tag:
> +# Sphinx cannot handle sectionless titles;
> +# Instead, just append the results to the prior section.
> +container = nodes.container()
> +self._parse_text_into_node(section.text, container)
> +nodelist += container.children
> +continue
> +
> snode = self._make_section(section.tag)
> -if section.tag and section.tag.startswith('Example'):
> +if section.tag.startswith('Example'):
> snode += self._nodes_for_example(dedent(section.text))
> else:
> -self._parse_text_into_node(
> -dedent(section.text) if section.tag else section.text,
> -snode,
> -)
> +self._parse_text_into_node(dedent(section.text), snode)
> nodelist.append(snode)
> return nodelist
Acked-by: Markus Armbruster
Re: [PATCH 05/13] qapi/parser: fix comment parsing immediately following a doc block
John Snow writes:
> If a comment immediately follows a doc block, the parser doesn't ignore
> that token appropriately. Fix that.
>
> e.g.
>
>> ##
>> # = Hello World!
>> ##
>>
>> # I'm a comment!
>
> will break the parser, because it does not properly ignore the comment
> token if it immediately follows a doc block.
>
> Fixes: 3d035cd2cca6 (qapi: Rewrite doc comment parser)
> Signed-off-by: John Snow
> ---
> scripts/qapi/parser.py | 2 +-
> tests/qapi-schema/doc-good.json | 2 ++
> 2 files changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
> index 43167ef0ab3..dfd6a6c5bee 100644
> --- a/scripts/qapi/parser.py
> +++ b/scripts/qapi/parser.py
> @@ -584,7 +584,7 @@ def get_doc(self) -> 'QAPIDoc':
> line = self.get_doc_line()
> first = False
>
> -self.accept(False)
> +self.accept()
> doc.end()
> return doc
>
> diff --git a/tests/qapi-schema/doc-good.json b/tests/qapi-schema/doc-good.json
> index de38a386e8f..8b39eb946af 100644
> --- a/tests/qapi-schema/doc-good.json
> +++ b/tests/qapi-schema/doc-good.json
> @@ -55,6 +55,8 @@
> # - {braces}
> ##
>
> +# Not a doc comment
> +
> ##
> # @Enum:
> #
Reviewed-by: Markus Armbruster
Re: [PATCH 04/13] qapi/parser: preserve indentation in QAPIDoc sections
John Snow writes:
> Change get_doc_indented() to preserve indentation on all subsequent text
> lines, and create a compatibility dedent() function for qapidoc.py to
> remove that indentation. This is being done for the benefit of a new
Suggest "remove indentation the same way get_doc_indented() did."
> qapidoc generator which requires that indentation in argument and
> features sections are preserved.
>
> Prior to this patch, a section like this:
>
> ```
> @name: lorem ipsum
>dolor sit amet
> consectetur adipiscing elit
> ```
>
> would have its body text be parsed as:
Suggest "parsed into".
> (first and final newline only for presentation)
>
> ```
> lorem ipsum
> dolor sit amet
> consectetur adipiscing elit
> ```
>
> We want to preserve the indentation for even the first body line so that
> the entire block can be parsed directly as rST. This patch would now
> parse that segment as:
If you change "parsed as" to "parsed into" above, then do it here, too.
>
> ```
> lorem ipsum
>dolor sit amet
> consectetur adipiscing elit
> ```
>
> This is helpful for formatting arguments and features as field lists in
> rST, where the new generator will format this information as:
>
> ```
> :arg type name: lorem ipsum
>dolor sit amet
> consectetur apidiscing elit
> ```
>
> ...and can be formed by the simple concatenation of the field list
> construct and the body text. The indents help preserve the continuation
> of a block-level element, and further allow the use of additional rST
> block-level constructs such as code blocks, lists, and other such
> markup. Avoiding reflowing the text conditionally also helps preserve
> source line context for better rST error reporting from sphinx through
> generated source, too.
What do you mean by "reflowing"?
> This understandably breaks the existing qapidoc.py; so a new function is
> added there to dedent the text for compatibility. Once the new generator
> is merged, this function will not be needed any longer and can be
> dropped.
>
> I verified this patch changes absolutely nothing by comparing the
> md5sums of the QMP ref html pages both before and after the change, so
> it's certified inert. QAPI test output has been updated to reflect the
> new strategy of preserving indents for rST.
I think the remainder is unnecessary detail. Drop?
> before:
>
> 69cde3d6f18b0f324badbb447d4381ce manual_before/interop/qemu-ga-ref.html
> 446e9381833def2adc779f1b90f2215f manual_before/interop/qemu-qmp-ref.html
> df0ad6c26cb4c28b85d663fe44609c12
> manual_before/interop/qemu-storage-daemon-qmp-ref.html
>
> after:
>
> 69cde3d6f18b0f324badbb447d4381ce manual/interop/qemu-ga-ref.html
> 446e9381833def2adc779f1b90f2215f manual/interop/qemu-qmp-ref.html
> df0ad6c26cb4c28b85d663fe44609c12
> manual/interop/qemu-storage-daemon-qmp-ref.html
>
> Signed-off-by: John Snow
> ---
> docs/sphinx/qapidoc.py | 29 -
> scripts/qapi/parser.py | 5 +++--
> tests/qapi-schema/doc-good.out | 32
> 3 files changed, 43 insertions(+), 23 deletions(-)
>
> diff --git a/docs/sphinx/qapidoc.py b/docs/sphinx/qapidoc.py
> index e675966defa..f2f2005dd5f 100644
> --- a/docs/sphinx/qapidoc.py
> +++ b/docs/sphinx/qapidoc.py
> @@ -26,6 +26,7 @@
>
> import os
> import re
> +import textwrap
>
> from docutils import nodes
> from docutils.parsers.rst import Directive, directives
> @@ -53,6 +54,21 @@
> __version__ = "1.0"
>
>
> +def dedent(text: str) -> str:
> +# Temporary: In service of the new QAPI Sphinx domain, the QAPI doc
> +# parser now preserves indents in args/members/features text.
> +# QAPIDoc does not handle this well, so undo that change here.
A comment should explain how things are. This one explains how things
have changed. Suggest:
# Adjust indentation to make description text parse as paragraph.
If we planned to keep this, we might want to explain in more detail, as
I did in review of v1. But we don't.
> +
> +lines = text.splitlines(True)
> +if re.match(r"\s+", lines[0]):
> +# First line is indented; description started on the line after
> +# the name. dedent the whole block.
> +return textwrap.dedent(text)
> +
> +# Descr started on same line. Dedent line 2+.
> +return lines[0] + textwrap.dedent("".join(lines[1:]))
> +
> +
> # Disable black auto-formatter until re-enabled:
> # fmt: off
>
> @@ -176,7 +192,7 @@ def _nodes_for_members(self, doc, what, base=None,
> branches=None):
> term = self._nodes_for_one_member(section.member)
> # TODO drop fallbacks when undocumented members are outlawed
> if section.text:
> -defn = section.text
> +defn = dedent(section.text)
> else:
> defn = [nodes.Text('Not documented')]
>
> @@ -214,7 +230,7 @@ def _nodes_for_enum_values(self, doc):
>
>
Re: [PATCH] block/file-posix: Consider discard flag when opening
> On 19 Jun 2024, at 11:16, Kevin Wolf wrote:
>
> Am 18.06.2024 um 23:24 hat Nir Soffer geschrieben:
>> Set has_discard only when BDRV_O_UNMAP is not set. With this users that
>> want to keep their images fully allocated can disable hole punching
>> when writing zeros or discarding using:
>>
>> -drive file=thick.img,discard=off
>>
>> This change is not entirely correct since it changes the default discard
>> behavior. Previously we always allowed punching holes, but now you have
>> must use discard=unmap|on to enable it. We probably need to add the
>> BDDR_O_UNMAP flag by default.
>>
>> make check still works, so maybe we don't have tests for sparsifying
>> images, or maybe you need to run special tests that do not run by
>> default. We needs tests for keeping images non-sparse.
>>
>> Signed-off-by: Nir Soffer
>
> So first of all, I agree with you that this patch is wrong. ;-)
>
> At first, I failed to understand the problem this is trying to solve. I
> put a debug message in handle_aiocb_discard() and tried with which
> options it triggers. [1] To me, this looked exactly like it should be.
> We only try to discard blocks when discard=unmap is given as an option.
>
> That leaves the case of write_zeroes. And while at the first sight, the
> code looked good, we do seem to have a problem there and it tried to
> unmap even with discard=off.
>
>> block/file-posix.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/block/file-posix.c b/block/file-posix.c
>> index be25e35ff6..acac2abadc 100644
>> --- a/block/file-posix.c
>> +++ b/block/file-posix.c
>> @@ -738,11 +738,11 @@ static int raw_open_common(BlockDriverState *bs, QDict
>> *options,
>> ret = -EINVAL;
>> goto fail;
>> }
>> #endif /* !defined(CONFIG_LINUX_IO_URING) */
>>
>> -s->has_discard = true;
>> +s->has_discard = !!(bdrv_flags & BDRV_O_UNMAP);
>> s->has_write_zeroes = true;
>>
>> if (fstat(s->fd, ) < 0) {
>> ret = -errno;
>> error_setg_errno(errp, errno, "Could not stat file");
>
> s->has_discard is about what the host supports, not about the semantics
> of the QEMU block node. So this doesn't feel right to me.
>
> So for the buggy case, write_zeroes, bdrv_co_pwrite_zeroes() has code
> that considers the case and clears the ~BDRV_REQ_MAY_UNMAP flags:
>
>if (!(child->bs->open_flags & BDRV_O_UNMAP)) {
>flags &= ~BDRV_REQ_MAY_UNMAP;
>}
>
> But it turns out that we don't necessarily even go through this function
> for the top node which has discard=off, so it can't take effect:
>
> (gdb) bt
> #0 0x74f2f144 in __pthread_kill_implementation () at /lib64/libc.so
>
Re: [PATCH] block/file-posix: Consider discard flag when opening
Am 18.06.2024 um 23:24 hat Nir Soffer geschrieben:
> Set has_discard only when BDRV_O_UNMAP is not set. With this users that
> want to keep their images fully allocated can disable hole punching
> when writing zeros or discarding using:
>
>-drive file=thick.img,discard=off
>
> This change is not entirely correct since it changes the default discard
> behavior. Previously we always allowed punching holes, but now you have
> must use discard=unmap|on to enable it. We probably need to add the
> BDDR_O_UNMAP flag by default.
>
> make check still works, so maybe we don't have tests for sparsifying
> images, or maybe you need to run special tests that do not run by
> default. We needs tests for keeping images non-sparse.
>
> Signed-off-by: Nir Soffer
So first of all, I agree with you that this patch is wrong. ;-)
At first, I failed to understand the problem this is trying to solve. I
put a debug message in handle_aiocb_discard() and tried with which
options it triggers. [1] To me, this looked exactly like it should be.
We only try to discard blocks when discard=unmap is given as an option.
That leaves the case of write_zeroes. And while at the first sight, the
code looked good, we do seem to have a problem there and it tried to
unmap even with discard=off.
> block/file-posix.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/block/file-posix.c b/block/file-posix.c
> index be25e35ff6..acac2abadc 100644
> --- a/block/file-posix.c
> +++ b/block/file-posix.c
> @@ -738,11 +738,11 @@ static int raw_open_common(BlockDriverState *bs, QDict
> *options,
> ret = -EINVAL;
> goto fail;
> }
> #endif /* !defined(CONFIG_LINUX_IO_URING) */
>
> -s->has_discard = true;
> +s->has_discard = !!(bdrv_flags & BDRV_O_UNMAP);
> s->has_write_zeroes = true;
>
> if (fstat(s->fd, ) < 0) {
> ret = -errno;
> error_setg_errno(errp, errno, "Could not stat file");
s->has_discard is about what the host supports, not about the semantics
of the QEMU block node. So this doesn't feel right to me.
So for the buggy case, write_zeroes, bdrv_co_pwrite_zeroes() has code
that considers the case and clears the ~BDRV_REQ_MAY_UNMAP flags:
if (!(child->bs->open_flags & BDRV_O_UNMAP)) {
flags &= ~BDRV_REQ_MAY_UNMAP;
}
But it turns out that we don't necessarily even go through this function
for the top node which has discard=off, so it can't take effect:
(gdb) bt
#0 0x74f2f144 in __pthread_kill_implementation () at /lib64/libc.so.6
#1 0x74ed765e in raise () at /lib64/libc.so.6
#2 0x74ebf902 in abort () at /lib64/libc.so.6
#3 0x5615aff0 in raw_do_pwrite_zeroes (bs=0x57f4bcf0, offset=0,
bytes=1048576, flags=BDRV_REQ_MAY_UNMAP, blkdev=false) at
../block/file-posix.c:3643
#4 0x5615557e in raw_co_pwrite_zeroes (bs=0x57f4bcf0, offset=0,
bytes=1048576, flags=BDRV_REQ_MAY_UNMAP) at ../block/file-posix.c:3655
#5 0x560cde2a in bdrv_co_do_pwrite_zeroes (bs=0x57f4bcf0,
offset=0, bytes=1048576, flags=6) at ../block/io.c:1901
#6 0x560c72f9 in bdrv_aligned_pwritev (child=0x57f51460,
req=0x7fffed5ff800, offset=0, bytes=1048576, align=1, qiov=0x0, qiov_offset=0,
flags=6) at ../block/io.c:2100
#7 0x560c6b41 in bdrv_co_do_zero_pwritev (child=0x57f51460,
offset=0, bytes=1048576, flags=6, req=0x7fffed5ff800) at ../block/io.c:2183
#8 0x560c6647 in bdrv_co_pwritev_part (child=0x57f51460, offset=0,
bytes=1048576, qiov=0x0, qiov_offset=0, flags=6) at ../block/io.c:2283
#9 0x560c634f in bdrv_co_pwritev (child=0x57f51460, offset=0,
bytes=1048576, qiov=0x0, flags=6) at ../block/io.c:2216
#10 0x560c75b5 in bdrv_co_pwrite_zeroes (child=0x57f51460,
offset=0, bytes=1048576, flags=BDRV_REQ_MAY_UNMAP) at ../block/io.c:2322
#11 0x56117d24 in raw_co_pwrite_zeroes (bs=0x57f44980, offset=0,
bytes=1048576, flags=BDRV_REQ_MAY_UNMAP) at ../block/raw-format.c:307
#12 0x560cde2a in bdrv_co_do_pwrite_zeroes (bs=0x57f44980,
offset=0, bytes=1048576, flags=6) at ../block/io.c:1901
#13 0x560c72f9 in bdrv_aligned_pwritev (child=0x57f513f0,
req=0x7fffed5ffd90, offset=0, bytes=1048576, align=1, qiov=0x0, qiov_offset=0,
flags=6) at ../block/io.c:2100
#14 0x560c6b41 in bdrv_co_do_zero_pwritev (child=0x57f513f0,
offset=0, bytes=1048576, flags=6, req=0x7fffed5ffd90) at ../block/io.c:2183
#15 0x560c6647 in bdrv_co_pwritev_part (child=0x57f513f0, offset=0,
bytes=1048576, qiov=0x0, qiov_offset=0, flags=6) at ../block/io.c:2283
#16 0x560ad741 in blk_co_do_pwritev_part (blk=0x57f51660, offset=0,
bytes=1048576, qiov=0x0, qiov_offset=0, flags=6) at
../block/block-backend.c:1425
#17 0x560ad5f2 in blk_co_pwritev_part (blk=0x57f51660, offset=0,
bytes=1048576, qiov=0x0, qiov_offset=0, flags=6) at
../block/block-backend.c:1440
#18 0x560ad8cf in blk_co_pwritev
Re: [PATCH] block: m25p80: Fix heap-buffer-overflow in flash_erase function
Hi Philippe,
On Tue, Jun 18, 2024 at 10:34 PM Philippe Mathieu-Daudé
wrote:
> On 18/6/24 21:11, Zheyu Ma wrote:
> > Thanks for your useful advice!
> >
> > So how about report the issue and return:
>
> We might report the issue to the user, but there should
> be a way the hardware report the issue to the guest software
> running. Usually signaled as error condition, irq, ...
> We need to figure out what check wasn't properly done.
> The spec is the source of truth.
>
Sure.
Although I'm unfamiliar with the device, I'll try to figure it out.
Zheyu
>
> > diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
> > index 8dec134832..2121b43708 100644
> > --- a/hw/block/m25p80.c
> > +++ b/hw/block/m25p80.c
> > @@ -617,6 +617,12 @@ static void flash_erase(Flash *s, int offset,
> > FlashCMD cmd)
> > abort();
> > }
> >
> > +if (offset + len > s->size) {
> > +qemu_log_mask(LOG_GUEST_ERROR,
> > + "M25P80: Erase operation exceeds storage size\n");
> > +return;
> > +}
> > +
> > trace_m25p80_flash_erase(s, offset, len);
> >
> > if ((s->pi->flags & capa_to_assert) != capa_to_assert) {
> >
> > regards,
> > Zheyu
> >
> > On Tue, Jun 18, 2024 at 5:35 PM Philippe Mathieu-Daudé
> > mailto:phi...@linaro.org>> wrote:
> >
> > Hi Zheyu,
> >
> > On 18/6/24 17:23, Zheyu Ma wrote:
> > > This patch fixes a heap-buffer-overflow issue in the flash_erase
> > function
> > > of the m25p80 flash memory emulation. The overflow occurs when the
> > > combination of offset and length exceeds the allocated memory for
> the
> > > storage. The patch adds a check to ensure that the erase length
> > does not
> > > exceed the storage size and adjusts the length accordingly if
> > necessary.
> > >
> > > Reproducer:
> > > cat << EOF | qemu-system-aarch64 -display none \
> > > -machine accel=qtest, -m 512M -machine kudo-bmc -qtest stdio
> > > writeq 0xc010 0x6
> > > writel 0xc00c 0x9
> > > writeq 0xc010 0xf27f9412
> > > writeq 0xc00f 0x2b5cdc26
> > > writeq 0xc00c 0x
> > > writeq 0xc00c 0x
> > > writeq 0xc00c 0x
> > > writel 0xc00c 0x9
> > > writeq 0xc00c 0x9
> > > EOF
> > >
> > > ASan log:
> > > ==2614308==ERROR: AddressSanitizer: heap-buffer-overflow on
> > address 0x7fd3fb7fc000 at pc 0x55aa77a442dc bp 0x7fffaa155900 sp
> > 0x7fffaa1550c8
> > > WRITE of size 65536 at 0x7fd3fb7fc000 thread T0
> > > #0 0x55aa77a442db in __asan_memset
> > llvm/compiler-rt/lib/asan/asan_interceptors_memintrinsics.cpp:26:3
> > > #1 0x55aa77e7e6b3 in flash_erase hw/block/m25p80.c:631:5
> > > #2 0x55aa77e6f8b1 in complete_collecting_data
> > hw/block/m25p80.c:773:9
> > > #3 0x55aa77e6aaa9 in m25p80_transfer8
> hw/block/m25p80.c:1550:13
> > > #4 0x55aa78e9a691 in ssi_transfer_raw_default
> hw/ssi/ssi.c:92:16
> > > #5 0x55aa78e996c0 in ssi_transfer hw/ssi/ssi.c:165:14
> > > #6 0x55aa78e8d76a in npcm7xx_fiu_uma_transaction
> > hw/ssi/npcm7xx_fiu.c:336:9
> > > #7 0x55aa78e8be4b in npcm7xx_fiu_ctrl_write
> > hw/ssi/npcm7xx_fiu.c:428:13
> > >
> > > Signed-off-by: Zheyu Ma > >
> > > ---
> > > hw/block/m25p80.c | 6 ++
> > > 1 file changed, 6 insertions(+)
> > >
> > > diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
> > > index 8dec134832..e9a59f6616 100644
> > > --- a/hw/block/m25p80.c
> > > +++ b/hw/block/m25p80.c
> > > @@ -617,6 +617,12 @@ static void flash_erase(Flash *s, int
> > offset, FlashCMD cmd)
> > > abort();
> > > }
> > >
> > > +if (offset + len > s->size) {
> > > +qemu_log_mask(LOG_GUEST_ERROR,
> > > + "M25P80: Erase exceeds storage size,
> > adjusting length\n");
> >
> > Usually hardware doesn't "adjust" but report error earlier.
> >
> > > +len = s->size - offset;
> > > +}
> > > +
> > > trace_m25p80_flash_erase(s, offset, len);
> > >
> > > if ((s->pi->flags & capa_to_assert) != capa_to_assert) {
> >
>
>
Re: [PATCH 03/13] docs/qapidoc: delint a tiny portion of the module
John Snow writes:
> In a forthcoming series that adds a new QMP documentation generator, it
> will be helpful to have a linting baseline. However, there's no need to
> shuffle around the deck chairs too much, because most of this code will
> be removed once that new qapidoc generator (the "transmogrifier") is in
> place.
>
> To ease my pain: just turn off the black auto-formatter for most, but
> not all, of qapidoc.py. This will help ensure that *new* code follows a
> coding standard without bothering too much with cleaning up the existing
> code.
>
> Code that I intend to keep is still subject to the delinting beam.
>
> Signed-off-by: John Snow
> ---
> docs/sphinx/qapidoc.py | 66 +-
> 1 file changed, 40 insertions(+), 26 deletions(-)
>
> diff --git a/docs/sphinx/qapidoc.py b/docs/sphinx/qapidoc.py
> index f270b494f01..e675966defa 100644
> --- a/docs/sphinx/qapidoc.py
> +++ b/docs/sphinx/qapidoc.py
> @@ -28,33 +28,42 @@
> import re
>
> from docutils import nodes
> +from docutils.parsers.rst import Directive, directives
> from docutils.statemachine import ViewList
> -from docutils.parsers.rst import directives, Directive
> -from sphinx.errors import ExtensionError
> -from sphinx.util.nodes import nested_parse_with_titles
> -import sphinx
> -from qapi.gen import QAPISchemaVisitor
> from qapi.error import QAPIError, QAPISemError
> +from qapi.gen import QAPISchemaVisitor
> from qapi.schema import QAPISchema
>
> +import sphinx
> +from sphinx.errors import ExtensionError
> +from sphinx.util.nodes import nested_parse_with_titles
> +
>
> # Sphinx up to 1.6 uses AutodocReporter; 1.7 and later
> # use switch_source_input. Check borrowed from kerneldoc.py.
> -Use_SSI = sphinx.__version__[:3] >= '1.7'
> -if Use_SSI:
> +USE_SSI = sphinx.__version__[:3] >= "1.7"
> +if USE_SSI:
> from sphinx.util.docutils import switch_source_input
> else:
> -from sphinx.ext.autodoc import AutodocReporter
> +from sphinx.ext.autodoc import ( # pylint: disable=no-name-in-module
> +AutodocReporter,
> +)
>
>
> -__version__ = '1.0'
> +__version__ = "1.0"
>
>
> +# Disable black auto-formatter until re-enabled:
> +# fmt: off
> +
> # Function borrowed from pydash, which is under the MIT license
> def intersperse(iterable, separator):
> """Yield the members of *iterable* interspersed with *separator*."""
> iterable = iter(iterable)
> -yield next(iterable)
> +try:
> +yield next(iterable)
> +except StopIteration:
> +return
This gets rid of pylint's
docs/sphinx/qapidoc.py:82:10: R1708: Do not raise StopIteration in
generator, use return statement instead (stop-iteration-return)
I considered the same change some time ago, and decided against it to
avoid deviating from pydash. StopIteration would be a programming error
here.
Please *delete* the function instead: commit fd62bff901b removed its
last use. I'd do it in a separate commit, but that's up to you.
> for item in iterable:
> yield separator
> yield item
> @@ -451,6 +460,10 @@ def get_document_nodes(self):
> return self._top_node.children
>
>
> +# Turn the black formatter on for the rest of the file.
> +# fmt: on
> +
> +
> class QAPISchemaGenDepVisitor(QAPISchemaVisitor):
> """A QAPI schema visitor which adds Sphinx dependencies each module
>
> @@ -458,34 +471,34 @@ class QAPISchemaGenDepVisitor(QAPISchemaVisitor):
> that the generated documentation output depends on the input
> schema file associated with each module in the QAPI input.
> """
> +
> def __init__(self, env, qapidir):
> self._env = env
> self._qapidir = qapidir
>
> def visit_module(self, name):
> if name != "./builtin":
> -qapifile = self._qapidir + '/' + name
> +qapifile = self._qapidir + "/" + name
The string literal quote changes are mildly annoying. But since by your
good work you're effectively appointing yourself maintainer of this
file... ;)
> self._env.note_dependency(os.path.abspath(qapifile))
> super().visit_module(name)
>
>
> class QAPIDocDirective(Directive):
> """Extract documentation from the specified QAPI .json file"""
> +
> required_argument = 1
> optional_arguments = 1
> -option_spec = {
> -'qapifile': directives.unchanged_required
> -}
> +option_spec = {"qapifile": directives.unchanged_required}
> has_content = False
>
> def new_serialno(self):
> """Return a unique new ID string suitable for use as a node's ID"""
> env = self.state.document.settings.env
> -return 'qapidoc-%d' % env.new_serialno('qapidoc')
> +return "qapidoc-%d" % env.new_serialno("qapidoc")
>
> def run(self):
> env = self.state.document.settings.env
> -qapifile = env.config.qapidoc_srctree + '/' + self.arguments[0]
> +qapifile =
