Re: [PATCH 02/20] qapi: linter fixups
John Snow writes:
> Fix minor irritants to pylint/flake8 et al.
>
> (Yes, these need to be guarded by the Python tests. That's a work in
> progress, a series that's quite likely to follow once I finish this
> Sphinx project. Please pardon the temporary irritation.)
No worries; one step at a time.
> Signed-off-by: John Snow
> ---
> scripts/qapi/introspect.py | 8
> scripts/qapi/schema.py | 6 +++---
> scripts/qapi/visit.py | 5 +++--
> 3 files changed, 10 insertions(+), 9 deletions(-)
>
> diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
> index 86c075a6ad2..ac14b20f308 100644
> --- a/scripts/qapi/introspect.py
> +++ b/scripts/qapi/introspect.py
> @@ -27,8 +27,8 @@
> from .schema import (
> QAPISchema,
> QAPISchemaAlternatives,
> -QAPISchemaBranches,
> QAPISchemaArrayType,
> +QAPISchemaBranches,
> QAPISchemaBuiltinType,
> QAPISchemaEntity,
> QAPISchemaEnumMember,
> @@ -233,9 +233,9 @@ def _use_type(self, typ: QAPISchemaType) -> str:
> typ = type_int
> elif (isinstance(typ, QAPISchemaArrayType) and
>typ.element_type.json_type() == 'int'):
> -type_intList = self._schema.lookup_type('intList')
> -assert type_intList
> -typ = type_intList
> +type_intlist = self._schema.lookup_type('intList')
> +assert type_intlist
> +typ = type_intlist
I named the variable after the type. Suppressing the linter complaint
just to keep that name isn't worthwhile. I might have picked
type_int_list instead. No need to change it now.
> # Add type to work queue if new
> if typ not in self._used_types:
> self._used_types.append(typ)
> diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
> index 721c470d2b8..d65c35f6ee6 100644
> --- a/scripts/qapi/schema.py
> +++ b/scripts/qapi/schema.py
> @@ -730,6 +730,7 @@ def set_defined_in(self, name: str) -> None:
> for v in self.variants:
> v.set_defined_in(name)
>
> +# pylint: disable=unused-argument
> def check(
> self, schema: QAPISchema, seen: Dict[str, QAPISchemaMember]
> ) -> None:
> @@ -1166,7 +1167,7 @@ def _def_definition(self, defn: QAPISchemaDefinition)
> -> None:
> defn.info, "%s is already defined" % other_defn.describe())
> self._entity_dict[defn.name] = defn
>
> -def lookup_entity(self,name: str) -> Optional[QAPISchemaEntity]:
> +def lookup_entity(self, name: str) -> Optional[QAPISchemaEntity]:
> return self._entity_dict.get(name)
>
> def lookup_type(self, name: str) -> Optional[QAPISchemaType]:
> @@ -1302,11 +1303,10 @@ def _make_implicit_object_type(
> name = 'q_obj_%s-%s' % (name, role)
> typ = self.lookup_entity(name)
> if typ:
> -assert(isinstance(typ, QAPISchemaObjectType))
> +assert isinstance(typ, QAPISchemaObjectType)
> # The implicit object type has multiple users. This can
> # only be a duplicate definition, which will be flagged
> # later.
> -pass
> else:
> self._def_definition(QAPISchemaObjectType(
> name, info, None, ifcond, None, None, members, None))
> diff --git a/scripts/qapi/visit.py b/scripts/qapi/visit.py
> index e766acaac92..12f92e429f6 100644
> --- a/scripts/qapi/visit.py
> +++ b/scripts/qapi/visit.py
> @@ -280,8 +280,9 @@ def gen_visit_alternate(name: str,
> abort();
> default:
> assert(visit_is_input(v));
> -error_setg(errp, "Invalid parameter type for '%%s', expected:
> %(name)s",
> - name ? name : "null");
> + error_setg(errp,
> + "Invalid parameter type for '%%s', expected: %(name)s",
> + name ? name : "null");
> /* Avoid passing invalid *obj to qapi_free_%(c_name)s() */
> g_free(*obj);
> *obj = NULL;
This is mostly lint I neglected to pick off in my recent work. Thanks
for taking care of it!
Reviewed-by: Markus Armbruster
Re: [PATCH 03/20] docs/qapidoc: delint a tiny portion of the module
John Snow writes: > In the coming patches, it's 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 the 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. > > For manual checking for now, try "black --check qapidoc.py" from the > docs/sphinx directory. "pip install black" (without root permissions) if > you do not have it installed otherwise. > > Signed-off-by: John Snow > --- > docs/sphinx/qapidoc.py | 16 +--- > 1 file changed, 9 insertions(+), 7 deletions(-) > > diff --git a/docs/sphinx/qapidoc.py b/docs/sphinx/qapidoc.py > index f270b494f01..1655682d4c7 100644 > --- a/docs/sphinx/qapidoc.py > +++ b/docs/sphinx/qapidoc.py > @@ -28,28 +28,30 @@ > 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 > + Exchanges old pylint gripe docs/sphinx/qapidoc.py:45:4: C0412: Imports from package sphinx are not grouped (ungrouped-imports) for new gripes docs/sphinx/qapidoc.py:37:0: C0411: third party import "import sphinx" should be placed before "from qapi.error import QAPIError, QAPISemError" (wrong-import-order) docs/sphinx/qapidoc.py:38:0: C0411: third party import "from sphinx.errors import ExtensionError" should be placed before "from qapi.error import QAPIError, QAPISemError" (wrong-import-order) docs/sphinx/qapidoc.py:39:0: C0411: third party import "from sphinx.util.nodes import nested_parse_with_titles" should be placed before "from qapi.error import QAPIError, QAPISemError" (wrong-import-order) Easy enough to fix. > > # 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' > +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 > > > -__version__ = '1.0' > +__version__ = "1.0" > > > +# fmt: off I figure this tells black to keep quiet for the remainder of the file. Worth a comment, I think. > # Function borrowed from pydash, which is under the MIT license > def intersperse(iterable, separator): > """Yield the members of *iterable* interspersed with *separator*.""" With my comments addressed Reviewed-by: Markus Armbruster
Re: [PATCH 04/20] qapi/parser: preserve indentation in QAPIDoc sections
John Snow writes:
> Prior to this patch, a section like this:
>
> @name: lorem ipsum
>dolor sit amet
> consectetur adipiscing elit
>
> would be parsed as:
>
> "lorem ipsum\ndolor sit amet\n 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:
>
> "lorem ipsum\n dolor sit amet\n consectetur adipiscing elit"
I'm afraid it's less than clear *why* we want to parse the entire block
directly as rST. I have just enough insight into what you've built on
top of this series to hazard a guess. Bear with me while I try to
explain it.
We first parse the doc comment with parser.py into an internal
representation. The structural parts become objects, and the remainder
becomes text attributes of these objects. Currently, parser.py
carefully adjusts indentation for these text attributes. Why? I'll get
to that.
For your example, parser.py creates an ArgSection object, and sets its
@text member to
"lorem ipsum\ndolor sit amet\n consectetur adipiscing elit"
Printing this string gives us
lorem ipsum
dolor sit amet
consectetur adipiscing elit
qapidoc.py then transforms parser.py's IR into Sphinx IR. The objects
become (more complicated) Sphinx objects, and their text attributes get
re-parsed as rST text into still more Sphinx objects.
This re-parse rejects your example with "Unexpected indentation."
Let me use a slightly different one:
# @name: lorem ipsum
#dolor sit amet
#consectetur adipiscing elit
Results in this @text member
lorem ipsum
dolor sit amet
consectetur adipiscing elit
which is re-parsed as paragraph, i.e. exactly what we want.
> This understandably breaks qapidoc.py;
Without indentation adjustment, we'd get
lorem ipsum
dolor sit amet
consectetur adipiscing elit
which would be re-parsed as a definition list, I guess. This isn't what
we want.
>so a new function is added there
> to re-dedent the text.
Your patch moves the indentation adjustment to another place. No
functional change.
You move it so you can branch off your new rendering pipeline before the
indentation adjustment, because ...
>Once the new generator is merged, this function
> will not be needed any longer and can be dropped.
... yours doesn't want it.
I believe it doesn't want it, because it generates rST (with a QAPI
extension) instead of Sphinx objects. For your example, something like
:arg name: lorem ipsum
dolor sit amet
consectetur adipiscing elit
For mine:
:arg name: lorem ipsum
dolor sit amet
consectetur adipiscing elit
Fair?
The transition from the doc comment to (extended) rST is straightforward
for these examples.
I hope it'll be as straightforward (and thus predictable) in other
cases, too.
> (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.)
>
> Signed-off-by: John Snow
> ---
> docs/sphinx/qapidoc.py | 36 +-
> scripts/qapi/parser.py | 8 ++--
> tests/qapi-schema/doc-good.out | 32 +++---
> 3 files changed, 53 insertions(+), 23 deletions(-)
>
> diff --git a/docs/sphinx/qapidoc.py b/docs/sphinx/qapidoc.py
> index 1655682d4c7..2e3ffcbafb7 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
> @@ -51,6 +52,28 @@
> __version__ = "1.0"
>
>
> +def dedent(text: str) -> str:
> +# Temporary: In service of the new QAPI domain, the QAPI doc parser
> +# now preserves indents in args/members/features text. QAPIDoc does
> +# not handle this well, so undo that change here.
> +
> +# QAPIDoc is being rewritten and will be replaced soon,
> +# but this function is here in the interim as transition glue.
I'm not sure we need the comment.
> +
> +lines = text.splitlines(True)
> +if len(lines) > 1:
> +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)
> +else:
pylint gripes
docs/sphinx/qapidoc.py:65:8: R1705: Unnecessary "else" after "return",
remove the "else" and de-indent the code inside it (no-else-return)
> +# Descr started on same line. Dedent line 2+.
> +return lines[0] + textwrap.dedent("".join(lines[1:]))
> +else:
> +# Descr was a single line; dedent entire line.
> +return t
Re: [PATCH 04/20] qapi/parser: preserve indentation in QAPIDoc sections
John Snow writes: > On Wed, May 15, 2024, 7:50 AM Markus Armbruster wrote: > >> John Snow writes: >> >> > Prior to this patch, a section like this: >> > >> > @name: lorem ipsum >> >dolor sit amet >> > consectetur adipiscing elit >> > >> > would be parsed as: >> > >> > "lorem ipsum\ndolor sit amet\n 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: >> > >> > "lorem ipsum\n dolor sit amet\n consectetur adipiscing elit" >> >> I'm afraid it's less than clear *why* we want to parse the entire block >> directly as rST. I have just enough insight into what you've built on >> top of this series to hazard a guess. Bear with me while I try to >> explain it. >> > > My own summary: qapidoc expects a paragraph, the new generator expects a > block. > > >> We first parse the doc comment with parser.py into an internal >> representation. The structural parts become objects, and the remainder >> becomes text attributes of these objects. Currently, parser.py >> carefully adjusts indentation for these text attributes. Why? I'll get >> to that. >> >> For your example, parser.py creates an ArgSection object, and sets its >> @text member to >> >> "lorem ipsum\ndolor sit amet\n consectetur adipiscing elit" >> >> Printing this string gives us >> >> lorem ipsum >> dolor sit amet >> consectetur adipiscing elit >> >> qapidoc.py then transforms parser.py's IR into Sphinx IR. The objects >> become (more complicated) Sphinx objects, and their text attributes get >> re-parsed as rST text into still more Sphinx objects. >> >> This re-parse rejects your example with "Unexpected indentation." >> > > Specifically, it'd be an unexpected *unindent*; the indent lacking on the > first *two* lines is the problem. > > >> Let me use a slightly different one: >> >> # @name: lorem ipsum >> #dolor sit amet >> #consectetur adipiscing elit >> >> Results in this @text member >> >> lorem ipsum >> dolor sit amet >> consectetur adipiscing elit >> >> which is re-parsed as paragraph, i.e. exactly what we want. >> > > It's what we used to want, anyway. Yes, I'm describing the current state here. >> > This understandably breaks qapidoc.py; >> >> Without indentation adjustment, we'd get >> >> lorem ipsum >>dolor sit amet >>consectetur adipiscing elit >> >> which would be re-parsed as a definition list, I guess. This isn't what >> we want. >> >> >so a new function is added there >> > to re-dedent the text. >> >> Your patch moves the indentation adjustment to another place. No >> functional change. >> >> You move it so you can branch off your new rendering pipeline before the >> indentation adjustment, because ... >> >> >Once the new generator is merged, this function >> > will not be needed any longer and can be dropped. >> >> ... yours doesn't want it. >> >> I believe it doesn't want it, because it generates rST (with a QAPI >> extension) instead of Sphinx objects. For your example, something like >> >> :arg name: lorem ipsum >>dolor sit amet >> consectetur adipiscing elit >> >> For mine: >> >> :arg name: lorem ipsum >>dolor sit amet >>consectetur adipiscing elit >> >> Fair? >> > > Not quite; > > Old parsing, new generator: > > :arg type name: lorem ipsum > dolor sit amet > consectetur apidiscing elit > > This is wrong - continuations of a field list must be indented. Unlike > paragraphs, we want indents to "keep the block". > > New parsing, new generator: > > :arg type name: lorem ipsum >dolor sit amet > consectetur apidiscing elit > > indent is preserved, maintaining the block-level element. > > I don't have to re-add indents and any nested block elements will be > preserved correctly. i.e. you can use code examples, nested lists, etc. in > argument definitions. > > The goal here was "Do not treat this
Re: [PATCH 03/20] docs/qapidoc: delint a tiny portion of the module
John Snow writes: > On Wed, May 15, 2024 at 5:17 AM Markus Armbruster wrote: > >> John Snow writes: >> >> > In the coming patches, it's 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 the 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. >> > >> > For manual checking for now, try "black --check qapidoc.py" from the >> > docs/sphinx directory. "pip install black" (without root permissions) if >> > you do not have it installed otherwise. >> > >> > Signed-off-by: John Snow >> > --- >> > docs/sphinx/qapidoc.py | 16 +--- >> > 1 file changed, 9 insertions(+), 7 deletions(-) >> > >> > diff --git a/docs/sphinx/qapidoc.py b/docs/sphinx/qapidoc.py >> > index f270b494f01..1655682d4c7 100644 >> > --- a/docs/sphinx/qapidoc.py >> > +++ b/docs/sphinx/qapidoc.py >> > @@ -28,28 +28,30 @@ >> > 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 >> > + >> >> Exchanges old pylint gripe >> >> docs/sphinx/qapidoc.py:45:4: C0412: Imports from package sphinx are >> not grouped (ungrouped-imports) >> >> for new gripes >> >> docs/sphinx/qapidoc.py:37:0: C0411: third party import "import sphinx" >> should be placed before "from qapi.error import QAPIError, QAPISemError" >> (wrong-import-order) >> docs/sphinx/qapidoc.py:38:0: C0411: third party import "from >> sphinx.errors import ExtensionError" should be placed before "from >> qapi.error import QAPIError, QAPISemError" (wrong-import-order) >> docs/sphinx/qapidoc.py:39:0: C0411: third party import "from >> sphinx.util.nodes import nested_parse_with_titles" should be placed before >> "from qapi.error import QAPIError, QAPISemError" (wrong-import-order) >> >> Easy enough to fix. >> > > I believe these errors are caused by the fact that the tools are confused > about the "sphinx" namespace - some interpret them as being the local > "module", the docs/sphinx/ directory, and others believe them to be the > third party external package. > > I have not been using pylint on docs/sphinx/ files because of the > difficulty of managing imports - this environment is generally beyond the > reaches of my python borgcube and at present I don't have plans to > integrate it. > > At the moment, I am using black, isort and flake8 for qapidoc.py and > they're happy with it. I am not using mypy because I never did the typing > boogaloo with qapidoc.py and I won't be bothering - except for any new code > I write, which *will* bother. By the end of the new transmogrifier, > qapidoc.py *will* strictly typecheck. > > pylint may prove to be an issue with the imports, though. isort also seems > to misunderstand "sphinx, the stuff in this folder" and "sphinx, the stuff > in a third party package" and so I'm afraid I don't have any good ability > to get pylint to play along, here. > > Pleading for "Sorry, this sucks and I can't figure out how to solve it > quickly". Maybe a future project, apologies. Is this pain we inflict on ourselves by naming the directory "sphinx"? >> > >> > # 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' >> > +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 >> > >> > >> > -__version__ = '1.0' >> > +__version__ = "1.0" >> > >> > >> > +# fmt: off >> >> I figure this tells black to keep quiet for the remainder of the file. >> Worth a comment, I think. >> >> > # Function borrowed from pydash, which is under the MIT license >> > def intersperse(iterable, separator): >> > """Yield the members of *iterable* interspersed with *separator*.""" >> >> With my comments addressed >> Reviewed-by: Markus Armbruster >> > > ^ Dropping this unless you're okay with the weird import orders owing to > the strange import paradigm in the sphinx folder.r Feel free to keep it.
Re: [PATCH 05/20] qapi/parser: adjust info location for doc body section
John Snow writes:
> Instead of using the info object for the doc block as a whole, update
> the info pointer for each call to ensure_untagged_section when the
> existing section is otherwise empty. This way, Sphinx error information
> will match precisely to where the text actually starts.
>
> 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 8cdd5334ec6..41b9319e5cb 100644
> --- a/scripts/qapi/parser.py
> +++ b/scripts/qapi/parser.py
> @@ -662,8 +662,13 @@ def end(self) -> None:
>
> def ensure_untagged_section(self, info: QAPISourceInfo) -> None:
> if self.all_sections and not self.all_sections[-1].tag:
> -# extend current section
> -self.all_sections[-1].text += '\n'
Before, we always append a newline.
> +section = self.all_sections[-1]
> +# Section is empty so far; update info to start *here*.
> +if not section.text:
> +section.info = info
> +else:
> +# extend current section
> +self.all_sections[-1].text += '\n'
Afterwards, we append it only when the section already has some text.
The commit message claims the patch only adjusts section.info. That's a
lie :)
I believe the change makes no difference because .end() strips leading
and trailing newline.
> return
> # start new section
> section = self.Section(info)
You could fix the commit message, but I think backing out the
no-difference change is easier. The appended patch works in my testing.
Next one. Your patch changes the meaning of section.info. Here's its
initialization:
class Section:
# pylint: disable=too-few-public-methods
def __init__(self, info: QAPISourceInfo,
tag: Optional[str] = None):
---># section source info, i.e. where it begins
self.info = info
# section tag, if any ('Returns', '@name', ...)
self.tag = tag
# section text without tag
self.text = ''
The comment is now wrong. Calls for a thorough review of .info's uses.
The alternative to changing .info's meaning is to add another member
with the meaning you need. Then we have to review .info's uses to find
out which ones to switch to the new one.
Left for later.
diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
index 8cdd5334ec..abeae1ca77 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -663,7 +663,10 @@ def end(self) -> None:
def ensure_untagged_section(self, info: QAPISourceInfo) -> None:
if self.all_sections and not self.all_sections[-1].tag:
# extend current section
-self.all_sections[-1].text += '\n'
+section = self.all_sections[-1]
+if not section.text:
+section.info = info
+section.text += '\n'
return
# start new section
section = self.Section(info)
Re: [PATCH 06/20] 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. Reproducer? > > Signed-off-by: John Snow > --- > scripts/qapi/parser.py | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py > index 41b9319e5cb..161768b8b96 100644 > --- a/scripts/qapi/parser.py > +++ b/scripts/qapi/parser.py > @@ -587,7 +587,7 @@ def get_doc(self) -> 'QAPIDoc': > line = self.get_doc_line() > first = False > > -self.accept(False) > +self.accept() > doc.end() > return doc Can't judge the fix without understanding the problem, and the problem will be easier to understand for me with a reproducer.
Re: [PATCH 07/20] qapi/parser: add semantic 'kind' parameter to QAPIDoc.Section
John Snow writes:
> When iterating all_sections, this is helpful to be able to distinguish
> "members" from "features"; the only other way to do so is to
> cross-reference these sections against QAPIDoc.args or QAPIDoc.features,
> but if the desired end goal for QAPIDoc is to remove everything except
> all_sections, we need *something* accessible to distinguish them.
>
> To keep types simple, add this semantic parameter to the base Section
> and not just ArgSection; we can use this to filter out paragraphs and
> tagged sections, too.
>
> Signed-off-by: John Snow
> ---
> scripts/qapi/parser.py | 25 -
> 1 file changed, 16 insertions(+), 9 deletions(-)
>
> diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
> index 161768b8b96..cf4cbca1c1f 100644
> --- a/scripts/qapi/parser.py
> +++ b/scripts/qapi/parser.py
> @@ -613,21 +613,27 @@ class QAPIDoc:
>
> class Section:
> # pylint: disable=too-few-public-methods
> -def __init__(self, info: QAPISourceInfo,
> - tag: Optional[str] = None):
> +def __init__(
> +self,
> +info: QAPISourceInfo,
> +tag: Optional[str] = None,
> +kind: str = 'paragraph',
> +):
> # section source info, i.e. where it begins
> self.info = info
> # section tag, if any ('Returns', '@name', ...)
> self.tag = tag
> # section text without tag
> self.text = ''
> +# section type - {paragraph, feature, member, tagged}
> +self.kind = kind
Hmm. .kind is almost redundant with .tag.
Untagged section:.kind is 'paragraph', .tag is None
Member description: .kind is 'member', .tag matches @NAME
Feature description: .kind is 'feature', .tag matches @NAME
Tagged section: .kind is 'tagged', .tag matches
r'Returns|Errors|Since|Notes?|Examples?|TODO'
.kind can directly be derived from .tag except for member and feature
descriptions. And you want to tell these two apart in a straightforward
manner in later patches, as you explain in your commit message.
If .kind is 'member' or 'feature', then self must be an ArgSection.
Worth a comment? An assertion?
Some time back, I considered changing .tag for member and feature
descriptions to suitable strings, like your 'member' and 'feature', and
move the member / feature name into ArgSection. I didn't, because the
benefit wasn't worth the churn at the time. Perhaps it's worth it now.
Would it result in simpler code than your solution?
Terminology nit: the section you call 'paragraph' isn't actually a
paragraph: it could be several paragraphs. Best to call it 'untagged',
as in .ensure_untagged_section().
>
> def append_line(self, line: str) -> None:
> self.text += line + '\n'
>
> class ArgSection(Section):
> -def __init__(self, info: QAPISourceInfo, tag: str):
> -super().__init__(info, tag)
> +def __init__(self, info: QAPISourceInfo, tag: str, kind: str):
> +super().__init__(info, tag, kind)
> self.member: Optional['QAPISchemaMember'] = None
>
> def connect(self, member: 'QAPISchemaMember') -> None:
[...]
Re: [PATCH 08/20] qapi/parser: differentiate intro and outro paragraphs
John Snow writes:
> Add a semantic tag to paragraphs that appear *before* tagged
> sections/members/features and those that appear after. This will control
> how they are inlined when doc sections are merged and flattened.
This future use is not obvious to me now. I guess the effective way to
help me see it is actual patches, which will come in due time.
> Signed-off-by: John Snow
> ---
> scripts/qapi/parser.py | 22 +-
> 1 file changed, 17 insertions(+), 5 deletions(-)
>
> diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
> index cf4cbca1c1f..b1794f71e12 100644
> --- a/scripts/qapi/parser.py
> +++ b/scripts/qapi/parser.py
> @@ -503,6 +503,10 @@ def get_doc(self) -> 'QAPIDoc':
> self.accept(False)
> line = self.get_doc_line()
> no_more_args = False
> +# Paragraphs before members/features/tagged are "intro"
> paragraphs.
> +# Any appearing subsequently are "outro" paragraphs.
> +# This is only semantic metadata for the doc generator.
Not sure about the last sentence. Isn't it true for almost everything
around here?
Also, long line.
> +intro = True
>
> while line is not None:
> # Blank lines
> @@ -532,6 +536,7 @@ def get_doc(self) -> 'QAPIDoc':
> raise QAPIParseError(
> self, 'feature descriptions expected')
> no_more_args = True
> +intro = False
After feature descriptions.
> elif match := self._match_at_name_colon(line):
> # description
> if no_more_args:
> @@ -547,6 +552,7 @@ def get_doc(self) -> 'QAPIDoc':
> doc.append_line(text)
> line = self.get_doc_indented(doc)
> no_more_args = True
> +intro = False
Or after member descriptions.
> elif match := re.match(
> r'(Returns|Errors|Since|Notes?|Examples?|TODO): *',
> line):
> @@ -557,13 +563,14 @@ def get_doc(self) -> 'QAPIDoc':
> doc.append_line(text)
> line = self.get_doc_indented(doc)
> no_more_args = True
> +intro = False
Or after the first tagged section.
Okay, it does what it says on the tin.
> elif line.startswith('='):
> raise QAPIParseError(
> self,
> "unexpected '=' markup in definition documentation")
> else:
> # tag-less paragraph
> -doc.ensure_untagged_section(self.info)
> +doc.ensure_untagged_section(self.info, intro)
> doc.append_line(line)
> line = self.get_doc_paragraph(doc)
> else:
> @@ -617,7 +624,7 @@ def __init__(
> self,
> info: QAPISourceInfo,
> tag: Optional[str] = None,
> -kind: str = 'paragraph',
> +kind: str = 'intro-paragraph',
The question "why is this optional?" crossed my mind when reviewing the
previous patch. I left it unasked, because I felt challenging the
overlap between @kind and @tag was more useful. However, the new
default value 'intro-paragraph' feels more arbitrary to me than the old
one 'paragraph', and that makes the question pop right back into my
mind.
Unless I'm mistaken, all calls but one @tag and @kind. Making that one
pass it too feels simpler to me.
Moot if we fuse @tag and @kind, of course.
> ):
> # section source info, i.e. where it begins
> self.info = info
> @@ -625,7 +632,7 @@ def __init__(
> self.tag = tag
> # section text without tag
> self.text = ''
> -# section type - {paragraph, feature, member, tagged}
> +# section type - {-paragraph, feature, member,
> tagged}
Long line.
> self.kind = kind
>
> def append_line(self, line: str) -> None:
> @@ -666,7 +673,11 @@ def end(self) -> None:
> raise QAPISemError(
> section.info, "text required after '%s:'" % section.tag)
>
> -def ensure_untagged_section(self, info: QAPISourceInfo) -> None:
> +def ensure_untagged_section(
> +self,
> +info: QAPISourceInfo,
> +intro: bool = True,
Two callers, one passes @info, one doesn't. Passing it always might be
simpler.
> +) -> None:
> if self.all_sections and not self.all_sections[-1].tag:
> section = self.all_sections[-1]
> # Section is empty so far; update info to start *here*.
> @@ -677,7 +688,8 @@ def ensure_untagged_section(self, info: QAPISourceInfo)
> -> None:
> self.all_sections[-1].text += '\n'
> return
>
Re: [PATCH 4/6] cpus: Improve error messages on memsave, pmemsave write error
Philippe Mathieu-Daudé writes:
> On 13/5/24 16:45, Markus Armbruster wrote:
>> Philippe Mathieu-Daudé writes:
>>
>>> On 13/5/24 16:17, Markus Armbruster wrote:
>>>> qmp_memsave() and qmp_pmemsave() report fwrite() error as
>>>>
>>>> An IO error has occurred
>>>>
>>>> Improve this to
>>>>
>>>> writing memory to '' failed
>>>>
>>>> Signed-off-by: Markus Armbruster
>>>> ---
>>>>system/cpus.c | 6 --
>>>>1 file changed, 4 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/system/cpus.c b/system/cpus.c
>>>> index 68d161d96b..f8fa78f33d 100644
>>>> --- a/system/cpus.c
>>>> +++ b/system/cpus.c
>>>> @@ -813,7 +813,8 @@ void qmp_memsave(int64_t addr, int64_t size, const
>>>> char *filename,
>>>> goto exit;
>>>> }
>>>> if (fwrite(buf, 1, l, f) != l) {
>>>> -error_setg(errp, QERR_IO_ERROR);
>>>> +error_setg(errp, "writing memory to '%s' failed",
>>>> + filename);
>>>> goto exit;
>>>> }
>>>> addr += l;
>>>> @@ -843,7 +844,8 @@ void qmp_pmemsave(int64_t addr, int64_t size, const
>>>> char *filename,
>>>> l = size;
>>>> cpu_physical_memory_read(addr, buf, l);
>>>> if (fwrite(buf, 1, l, f) != l) {
>>>> -error_setg(errp, QERR_IO_ERROR);
>>>> +error_setg(errp, "writing memory to '%s' failed",
>>>> + filename);
>>>
>>> What about including errno with error_setg_errno()?
>>
>> Sure fwrite() fails with errno reliably set? The manual page doesn't
>> mention it...
>
> Indeed. I can see some uses in the code base:
>
> qemu-io-cmds.c:409:if (ferror(f)) {
> qemu-io-cmds.c-410-perror(file_name);
This is after fread(), which isn't specified to set errno, either.
> qga/commands-posix.c-632-write_count = fwrite(buf, 1, count, fh);
> qga/commands-posix.c:633:if (ferror(fh)) {
> qga/commands-posix.c-634-error_setg_errno(errp, errno, "failed to
> write to file");
This one is after fwrite(), like the code I'm changing.
> util/qemu-config.c:152:if (ferror(fp)) {
> util/qemu-config.c-153-loc_pop(&loc);
> util/qemu-config.c-154-error_setg_errno(errp, errno, "Cannot read
> config file");
This is after fgets(), which isn't specified to set errno, either.
All three uses feel iffy to me. They work if the stream's error
indicator is clear before fread() / fwrite() / fgets(), and it is set
there, and the reason for it being set is something that sets errno
(such as a failed system call, which seems likely), and errno remains
untouched until after ferror(). Too much "if", "seems likely" for my
taste.
> Regardless,
>
> Reviewed-by: Philippe Mathieu-Daudé
Thanks!
Re: [PATCH 5/6] migration: Rephrase message on failure to save / load Xen device state
Peter Xu writes: > On Mon, May 13, 2024 at 04:17:02PM +0200, Markus Armbruster wrote: >> Functions that use an Error **errp parameter to return errors should >> not also report them to the user, because reporting is the caller's >> job. When the caller does, the error is reported twice. When it >> doesn't (because it recovered from the error), there is no error to >> report, i.e. the report is bogus. >> >> qmp_xen_save_devices_state() and qmp_xen_load_devices_state() violate >> this principle: they call qemu_save_device_state() and >> qemu_loadvm_state(), which call error_report_err(). >> >> I wish I could clean this up now, but migration's error reporting is >> too complicated (confused?) for me to mess with it. > > :-( If I understood how it's *supposed* to work, I might have a chance... I can see a mixture of reporting errors directly (with error_report() & friends), passing them to callers (via Error **errp), and storing them in / retrieving them from MigrationState member @error. This can't be right. I think a necessary first step towards getting it right is a shared understanding how errors are to be handled in migration code. This includes how error data should flow from error source to error sink, and what the possible sinks are. >> Instead, I'm merely improving the error reported by >> qmp_xen_load_devices_state() and qmp_xen_load_devices_state() to the >> QMP core from >> >> An IO error has occurred >> >> to >> saving Xen device state failed >> >> and >> >> loading Xen device state failed >> >> respectively. >> >> Signed-off-by: Markus Armbruster > > Acked-by: Peter Xu Thanks!
Re: [PATCH 05/20] qapi/parser: adjust info location for doc body section
John Snow writes:
> On Thu, May 16, 2024, 1:58 AM Markus Armbruster wrote:
>
>> John Snow writes:
>>
>> > Instead of using the info object for the doc block as a whole, update
>> > the info pointer for each call to ensure_untagged_section when the
>> > existing section is otherwise empty. This way, Sphinx error information
>> > will match precisely to where the text actually starts.
>> >
>> > 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 8cdd5334ec6..41b9319e5cb 100644
>> > --- a/scripts/qapi/parser.py
>> > +++ b/scripts/qapi/parser.py
>> > @@ -662,8 +662,13 @@ def end(self) -> None:
>> >
>> > def ensure_untagged_section(self, info: QAPISourceInfo) -> None:
>> > if self.all_sections and not self.all_sections[-1].tag:
>> > -# extend current section
>> > -self.all_sections[-1].text += '\n'
>>
>> Before, we always append a newline.
>>
>> > +section = self.all_sections[-1]
>> > +# Section is empty so far; update info to start *here*.
>> > +if not section.text:
>> > +section.info = info
>> > +else:
>> > +# extend current section
>> > +self.all_sections[-1].text += '\n'
>>
>> Afterwards, we append it only when the section already has some text.
>>
>> The commit message claims the patch only adjusts section.info. That's a
>> lie :)
>>
>
> Well. It wasn't intentional, so it wasn't a lie... it was just wrong :)
>
>
>> I believe the change makes no difference because .end() strips leading
>> and trailing newline.
>>
>> > return
>> > # start new section
>> > section = self.Section(info)
>>
>> You could fix the commit message, but I think backing out the
>> no-difference change is easier. The appended patch works in my testing.
>>
>> Next one. Your patch changes the meaning of section.info. Here's its
>> initialization:
>>
>> class Section:
>> # pylint: disable=too-few-public-methods
>> def __init__(self, info: QAPISourceInfo,
>> tag: Optional[str] = None):
>> ---># section source info, i.e. where it begins
>> self.info = info
>> # section tag, if any ('Returns', '@name', ...)
>> self.tag = tag
>> # section text without tag
>> self.text = ''
>>
>> The comment is now wrong. Calls for a thorough review of .info's uses.
>>
>
> Hmm... Did I really change its meaning? I guess it's debatable what "where
> it begins" means. Does the tagless section start...
>
> ## <-- Here?
> # Hello! <-- Or here?
> ##
>
> I assert the *section* starts wherever the first line of text it contains
> starts. Nothing else makes any sense.
>
> There is value in recording where the doc block starts, but that's not a
> task for the *section* info.
>
> I don't think I understand your feedback.
This was before my vacation, and my memory is foggy, ... I may have
gotten confused back then. Let me have a fresh look now.
self.info gets initialized in Section.__init__() to whatever info it
gets passed.
Your patch makes .ensure_untagged_section() overwrite this Section.info
when it extends an untagged section that is still empty. Hmmm. I'd
prefer .info to remain constant after initialization.
I figure this overwrite can happen only when extenting the body section
QAPIDoc.__init__() creates. In that case, it adjusts .info from
beginning of doc comment to first non-blank line.
Thoughts?
>> The alternative to changing .info's meaning is to add another member
>> with the meaning you need. Then we have to review .info's uses to find
>> out which ones to switch to the new one.
>
>
>> Left for later.
>>
>>
>> diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
>> index 8cdd5334ec..abeae1ca77 100644
>> --- a/scripts/qapi/parser.py
>> +++ b/scripts/qapi/parser.py
>> @@ -663,7 +663,10 @@ def end(self) -> None:
>> def ensure_untagged_section(self, info: QAPISourceInfo) -> None:
>> if self.all_sections and not self.all_sections[-1].tag:
>> # extend current section
>> -self.all_sections[-1].text += '\n'
>> +section = self.all_sections[-1]
>> +if not section.text:
>> +section.info = info
>> +section.text += '\n'
>> return
>> # start new section
>> section = self.Section(info)
>>
>>
Re: [PATCH v6 10/12] hostmem: add a new memory backend based on POSIX shm_open()
Stefano Garzarella writes:
> shm_open() creates and opens a new POSIX shared memory object.
> A POSIX shared memory object allows creating memory backend with an
> associated file descriptor that can be shared with external processes
> (e.g. vhost-user).
>
> The new `memory-backend-shm` can be used as an alternative when
> `memory-backend-memfd` is not available (Linux only), since shm_open()
> should be provided by any POSIX-compliant operating system.
>
> This backend mimics memfd, allocating memory that is practically
> anonymous. In theory shm_open() requires a name, but this is allocated
> for a short time interval and shm_unlink() is called right after
> shm_open(). After that, only fd is shared with external processes
> (e.g., vhost-user) as if it were associated with anonymous memory.
>
> In the future we may also allow the user to specify the name to be
> passed to shm_open(), but for now we keep the backend simple, mimicking
> anonymous memory such as memfd.
>
> Acked-by: David Hildenbrand
> Acked-by: Stefan Hajnoczi
> Signed-off-by: Stefano Garzarella
> ---
> v5
> - fixed documentation in qapi/qom.json and qemu-options.hx [Markus]
> v4
> - fail if we find "share=off" in shm_backend_memory_alloc() [David]
> v3
> - enriched commit message and documentation to highlight that we
> want to mimic memfd (David)
> ---
> docs/system/devices/vhost-user.rst | 5 +-
> qapi/qom.json | 19 +
> backends/hostmem-shm.c | 123 +
> backends/meson.build | 1 +
> qemu-options.hx| 16
> 5 files changed, 162 insertions(+), 2 deletions(-)
> create mode 100644 backends/hostmem-shm.c
>
> diff --git a/docs/system/devices/vhost-user.rst
> b/docs/system/devices/vhost-user.rst
> index 9b2da106ce..35259d8ec7 100644
> --- a/docs/system/devices/vhost-user.rst
> +++ b/docs/system/devices/vhost-user.rst
> @@ -98,8 +98,9 @@ Shared memory object
>
> In order for the daemon to access the VirtIO queues to process the
> requests it needs access to the guest's address space. This is
> -achieved via the ``memory-backend-file`` or ``memory-backend-memfd``
> -objects. A reference to a file-descriptor which can access this object
> +achieved via the ``memory-backend-file``, ``memory-backend-memfd``, or
> +``memory-backend-shm`` objects.
> +A reference to a file-descriptor which can access this object
> will be passed via the socket as part of the protocol negotiation.
>
> Currently the shared memory object needs to match the size of the main
> diff --git a/qapi/qom.json b/qapi/qom.json
> index 38dde6d785..d40592d863 100644
> --- a/qapi/qom.json
> +++ b/qapi/qom.json
> @@ -721,6 +721,21 @@
> '*hugetlbsize': 'size',
> '*seal': 'bool' } }
>
> +##
> +# @MemoryBackendShmProperties:
> +#
> +# Properties for memory-backend-shm objects.
> +#
> +# The @share boolean option is true by default with shm. Setting it to false
> +# will cause a failure during allocation because it is not supported by this
> +# backend.
docs/devel/qapi-code-gen.rst:
For legibility, wrap text paragraphs so every line is at most 70
characters long.
Separate sentences with two spaces.
Result:
# Properties for memory-backend-shm objects.
#
# The @share boolean option is true by default with shm. Setting it
# to false will cause a failure during allocation because it is not
# supported by this backend.
However, this contradicts the doc comment for @share:
# @share: if false, the memory is private to QEMU; if true, it is
# shared (default: false)
Your intention is to override that text. But that's less than clear.
Moreover, the documentation of @share is pretty far from this override.
John Snow is working on patches that'll pull it closer.
Hmm, MemoryBackendMemfdProperties has the same override.
I think we should change the doc comment for @share to something like
# @share: if false, the memory is private to QEMU; if true, it is
# shared (default depends on the backend type)
and then document the actual default with each backend type.
> +#
> +# Since: 9.1
> +##
> +{ 'struct': 'MemoryBackendShmProperties',
> + 'base': 'MemoryBackendProperties',
> + 'data': { } }
Let's add 'if': 'CONFIG_POSIX' here.
> +
> ##
> # @MemoryBackendEpcProperties:
> #
> @@ -985,6 +1000,8 @@
> { 'name': 'memory-backend-memfd',
>'if': 'CONFIG_LINUX' },
> 'memory-backend-ram',
> +{ 'name': 'memory-backend-shm',
> + 'if': 'CONFIG_POSIX' },
> 'pef-guest',
> { 'name': 'pr-manager-helper',
>'if': 'CONFIG_LINUX' },
> @@ -1056,6 +1073,8 @@
>'memory-backend-memfd': { 'type': 'MemoryBackendMemfdProperties',
>'if': 'CONFIG_LINUX' },
>'memory-backend-ram': 'MemoryBackendProperties',
> + 'memory-backend-shm': { 'type': 'MemoryBackendShmProperties',
> +
Re: [PATCH v6 10/12] hostmem: add a new memory backend based on POSIX shm_open()
Stefano Garzarella writes: > On Wed, May 29, 2024 at 04:50:20PM GMT, Markus Armbruster wrote: >>Stefano Garzarella writes: >> >>> shm_open() creates and opens a new POSIX shared memory object. >>> A POSIX shared memory object allows creating memory backend with an >>> associated file descriptor that can be shared with external processes >>> (e.g. vhost-user). >>> >>> The new `memory-backend-shm` can be used as an alternative when >>> `memory-backend-memfd` is not available (Linux only), since shm_open() >>> should be provided by any POSIX-compliant operating system. >>> >>> This backend mimics memfd, allocating memory that is practically >>> anonymous. In theory shm_open() requires a name, but this is allocated >>> for a short time interval and shm_unlink() is called right after >>> shm_open(). After that, only fd is shared with external processes >>> (e.g., vhost-user) as if it were associated with anonymous memory. >>> >>> In the future we may also allow the user to specify the name to be >>> passed to shm_open(), but for now we keep the backend simple, mimicking >>> anonymous memory such as memfd. >>> >>> Acked-by: David Hildenbrand >>> Acked-by: Stefan Hajnoczi >>> Signed-off-by: Stefano Garzarella >>> --- >>> v5 >>> - fixed documentation in qapi/qom.json and qemu-options.hx [Markus] >>> v4 >>> - fail if we find "share=off" in shm_backend_memory_alloc() [David] >>> v3 >>> - enriched commit message and documentation to highlight that we >>> want to mimic memfd (David) >>> --- >>> docs/system/devices/vhost-user.rst | 5 +- >>> qapi/qom.json | 19 + >>> backends/hostmem-shm.c | 123 + >>> backends/meson.build | 1 + >>> qemu-options.hx| 16 >>> 5 files changed, 162 insertions(+), 2 deletions(-) >>> create mode 100644 backends/hostmem-shm.c >>> >>> diff --git a/docs/system/devices/vhost-user.rst >>> b/docs/system/devices/vhost-user.rst >>> index 9b2da106ce..35259d8ec7 100644 >>> --- a/docs/system/devices/vhost-user.rst >>> +++ b/docs/system/devices/vhost-user.rst >>> @@ -98,8 +98,9 @@ Shared memory object >>> >>> In order for the daemon to access the VirtIO queues to process the >>> requests it needs access to the guest's address space. This is >>> -achieved via the ``memory-backend-file`` or ``memory-backend-memfd`` >>> -objects. A reference to a file-descriptor which can access this object >>> +achieved via the ``memory-backend-file``, ``memory-backend-memfd``, or >>> +``memory-backend-shm`` objects. >>> +A reference to a file-descriptor which can access this object >>> will be passed via the socket as part of the protocol negotiation. >>> >>> Currently the shared memory object needs to match the size of the main >>> diff --git a/qapi/qom.json b/qapi/qom.json >>> index 38dde6d785..d40592d863 100644 >>> --- a/qapi/qom.json >>> +++ b/qapi/qom.json >>> @@ -721,6 +721,21 @@ >>> '*hugetlbsize': 'size', >>> '*seal': 'bool' } } >>> >>> +## >>> +# @MemoryBackendShmProperties: >>> +# >>> +# Properties for memory-backend-shm objects. >>> +# >>> +# The @share boolean option is true by default with shm. Setting it to >>> false >>> +# will cause a failure during allocation because it is not supported by >>> this >>> +# backend. >> >>docs/devel/qapi-code-gen.rst: >> >>For legibility, wrap text paragraphs so every line is at most 70 >>characters long. >> >>Separate sentences with two spaces. >> >>Result: >> >> # Properties for memory-backend-shm objects. >> # >> # The @share boolean option is true by default with shm. Setting it >> # to false will cause a failure during allocation because it is not >> # supported by this backend. > > Ops, sorry, I'll fix! > >> >>However, this contradicts the doc comment for @share: >> >> # @share: if false, the memory is private to QEMU; if true, it is >> # shared (default: false) >> >>Your intention is to override that text. But that's less than clear. >>Moreover, the documentation of @share is pretty far from this override.
Re: [PATCH v2 1/2] copy-before-write: allow specifying minimum cluster size
errp);
This is now more obviously safe. Thanks!
> if (cluster_size < 0) {
> return NULL;
> }
> diff --git a/block/copy-before-write.c b/block/copy-before-write.c
> index cd65524e26..ef0bc4dcfe 100644
> --- a/block/copy-before-write.c
> +++ b/block/copy-before-write.c
> @@ -417,6 +417,7 @@ static BlockdevOptions *cbw_parse_options(QDict *options,
> Error **errp)
> qdict_extract_subqdict(options, NULL, "bitmap");
> qdict_del(options, "on-cbw-error");
> qdict_del(options, "cbw-timeout");
> +qdict_del(options, "min-cluster-size");
>
> out:
> visit_free(v);
> @@ -432,6 +433,7 @@ static int cbw_open(BlockDriverState *bs, QDict *options,
> int flags,
> BDRVCopyBeforeWriteState *s = bs->opaque;
> BdrvDirtyBitmap *bitmap = NULL;
> int64_t cluster_size;
> +uint64_t min_cluster_size = 0;
> g_autoptr(BlockdevOptions) full_opts = NULL;
> BlockdevOptionsCbw *opts;
> int ret;
> @@ -476,8 +478,14 @@ static int cbw_open(BlockDriverState *bs, QDict
> *options, int flags,
> bs->file->bs->supported_zero_flags);
>
> s->discard_source = flags & BDRV_O_CBW_DISCARD_SOURCE;
> +
> +if (opts->has_min_cluster_size) {
> +min_cluster_size = opts->min_cluster_size;
> +}
> +
> s->bcs = block_copy_state_new(bs->file, s->target, bs, bitmap,
> - flags & BDRV_O_CBW_DISCARD_SOURCE, errp);
> + flags & BDRV_O_CBW_DISCARD_SOURCE,
> + min_cluster_size, errp);
> if (!s->bcs) {
> error_prepend(errp, "Cannot create block-copy-state: ");
> return -EINVAL;
You can pass opts->min_cluster_size directly, as in v1. When
!opts->has_min_cluster_size, then opts->min_cluster_size is zero.
> diff --git a/include/block/block-copy.h b/include/block/block-copy.h
> index bdc703bacd..dd5cc82f3b 100644
> --- a/include/block/block-copy.h
> +++ b/include/block/block-copy.h
> @@ -28,6 +28,7 @@ BlockCopyState *block_copy_state_new(BdrvChild *source,
> BdrvChild *target,
> BlockDriverState *copy_bitmap_bs,
> const BdrvDirtyBitmap *bitmap,
> bool discard_source,
> + uint64_t min_cluster_size,
> Error **errp);
>
> /* Function should be called prior any actual copy request */
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index df5e07debd..8fc0a4b234 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -4642,12 +4642,18 @@
> # @on-cbw-error parameter will decide how this failure is handled.
> # Default 0. (Since 7.1)
> #
> +# @min-cluster-size: Minimum size of blocks used by copy-before-write
> +# operations. Has to be a power of 2. No effect if smaller than
> +# the maximum of the target's cluster size and 64 KiB. Default 0.
> +# (Since 9.1)
> +#
> # Since: 6.2
> ##
> { 'struct': 'BlockdevOptionsCbw',
>'base': 'BlockdevOptionsGenericFormat',
>'data': { 'target': 'BlockdevRef', '*bitmap': 'BlockDirtyBitmap',
> -'*on-cbw-error': 'OnCbwError', '*cbw-timeout': 'uint32' } }
> +'*on-cbw-error': 'OnCbwError', '*cbw-timeout': 'uint32',
> +'*min-cluster-size': 'size' } }
>
> ##
> # @BlockdevOptions:
QAPI schema
Acked-by: Markus Armbruster
Re: [PATCH v2 2/2] backup: add minimum cluster size to performance options
Fiona Ebner writes:
> In the context of backup fleecing, discarding the source will not work
> when the fleecing image has a larger granularity than the one used for
> block-copy operations (can happen if the backup target has smaller
> cluster size), because cbw_co_pdiscard_snapshot() will align down the
> discard requests and thus effectively ignore then.
>
> To make @discard-source work in such a scenario, allow specifying the
> minimum cluster size used for block-copy operations and thus in
> particular also the granularity for discard requests to the source.
>
> Suggested-by: Vladimir Sementsov-Ogievskiy
> Signed-off-by: Fiona Ebner
[...]
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 8fc0a4b234..f1219a9dfb 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -1551,11 +1551,16 @@
> # it should not be less than job cluster size which is calculated
> # as maximum of target image cluster size and 64k. Default 0.
> #
> +# @min-cluster-size: Minimum size of blocks used by copy-before-write
> +# and background copy operations. Has to be a power of 2. No
> +# effect if smaller than the maximum of the target's cluster size
> +# and 64 KiB. Default 0. (Since 9.1)
> +#
> # Since: 6.0
> ##
> { 'struct': 'BackupPerf',
> - 'data': { '*use-copy-range': 'bool',
> -'*max-workers': 'int', '*max-chunk': 'int64' } }
> + 'data': { '*use-copy-range': 'bool', '*max-workers': 'int',
> +'*max-chunk': 'int64', '*min-cluster-size': 'size' } }
>
> ##
> # @BackupCommon:
QAPI schema
Acked-by: Markus Armbruster
Re: [RFC PATCH v2 21/22] qapi: Inline and remove QERR_UNSUPPORTED definition
Philippe Mathieu-Daudé writes: > Michael, Konstantin, QERR_UNSUPPORTED is only used by QGA. > > Do you mind giving our maintainer's position on Markus > analysis so we can know how to proceed with this definition? Daniel Berrangé recently posted patches that get rid of most instances of QERR_UNSUPPORTED: [PATCH 00/20] qga: clean up command source locations and conditionals https://lore.kernel.org/qemu-devel/20240604134933.220112-1-berra...@redhat.com/ I pointed out a possible opportunity to remove even more. I think we should let the dust settle there, then figure out how to eliminate remaining QERR_UNSUPPORTED, if any.
Re: [PATCH RESEND v7 10/12] hostmem: add a new memory backend based on POSIX shm_open()
Stefano Garzarella writes:
> shm_open() creates and opens a new POSIX shared memory object.
> A POSIX shared memory object allows creating memory backend with an
> associated file descriptor that can be shared with external processes
> (e.g. vhost-user).
>
> The new `memory-backend-shm` can be used as an alternative when
> `memory-backend-memfd` is not available (Linux only), since shm_open()
> should be provided by any POSIX-compliant operating system.
>
> This backend mimics memfd, allocating memory that is practically
> anonymous. In theory shm_open() requires a name, but this is allocated
> for a short time interval and shm_unlink() is called right after
> shm_open(). After that, only fd is shared with external processes
> (e.g., vhost-user) as if it were associated with anonymous memory.
>
> In the future we may also allow the user to specify the name to be
> passed to shm_open(), but for now we keep the backend simple, mimicking
> anonymous memory such as memfd.
>
> Acked-by: David Hildenbrand
> Acked-by: Stefan Hajnoczi
> Signed-off-by: Stefano Garzarella
[...]
> diff --git a/qapi/qom.json b/qapi/qom.json
> index 9b8f6a7ab5..94e4458288 100644
> --- a/qapi/qom.json
> +++ b/qapi/qom.json
> @@ -601,8 +601,8 @@
> #
> # @share: if false, the memory is private to QEMU; if true, it is
> # shared (default false for backends memory-backend-file and
> -# memory-backend-ram, true for backends memory-backend-epc and
> -# memory-backend-memfd)
> +# memory-backend-ram, true for backends memory-backend-epc,
> +# memory-backend-memfd, and memory-backend-shm)
> #
> # @reserve: if true, reserve swap space (or huge pages) if applicable
> # (default: true) (since 6.1)
> @@ -721,6 +721,22 @@
> '*hugetlbsize': 'size',
> '*seal': 'bool' } }
>
> +##
> +# @MemoryBackendShmProperties:
> +#
> +# Properties for memory-backend-shm objects.
> +#
> +# Setting @share boolean option (defined in the base type) to false
> +# will cause a failure during allocation because it is not
> +# supported by this backend.
This is QMP reference documentation. "Failure during allocation" feels
like unnecessary detail there. Maybe "This memory backend support only
shared memory, which is the default."
> +#
> +# Since: 9.1
> +##
> +{ 'struct': 'MemoryBackendShmProperties',
> + 'base': 'MemoryBackendProperties',
> + 'data': { },
> + 'if': 'CONFIG_POSIX' }
> +
> ##
> # @MemoryBackendEpcProperties:
> #
> @@ -1049,6 +1065,8 @@
> { 'name': 'memory-backend-memfd',
>'if': 'CONFIG_LINUX' },
> 'memory-backend-ram',
> +{ 'name': 'memory-backend-shm',
> + 'if': 'CONFIG_POSIX' },
> 'pef-guest',
> { 'name': 'pr-manager-helper',
>'if': 'CONFIG_LINUX' },
> @@ -1121,6 +1139,8 @@
>'memory-backend-memfd': { 'type': 'MemoryBackendMemfdProperties',
> 'if': 'CONFIG_LINUX' },
>'memory-backend-ram': 'MemoryBackendProperties',
> + 'memory-backend-shm': { 'type': 'MemoryBackendShmProperties',
> + 'if': 'CONFIG_POSIX' },
>'pr-manager-helper': { 'type': 'PrManagerHelperProperties',
>'if': 'CONFIG_LINUX' },
>'qtest': 'QtestProperties',
[...]
Other than that, QAPI schema
Acked-by: Markus Armbruster
Re: [PATCH 06/20] qapi/parser: fix comment parsing immediately following a doc block
John Snow writes:
> On Thu, May 16, 2024 at 2:01 AM Markus Armbruster wrote:
>
>> John Snow writes:
>>
>> > If a comment immediately follows a doc block, the parser doesn't ignore
>> > that token appropriately. Fix that.
>>
>> Reproducer?
>>
>> >
>> > Signed-off-by: John Snow
>> > ---
>> > scripts/qapi/parser.py | 2 +-
>> > 1 file changed, 1 insertion(+), 1 deletion(-)
>> >
>> > diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
>> > index 41b9319e5cb..161768b8b96 100644
>> > --- a/scripts/qapi/parser.py
>> > +++ b/scripts/qapi/parser.py
>> > @@ -587,7 +587,7 @@ def get_doc(self) -> 'QAPIDoc':
>> > line = self.get_doc_line()
>> > first = False
>> >
>> > -self.accept(False)
>> > +self.accept()
>> > doc.end()
>> > return doc
>>
>> Can't judge the fix without understanding the problem, and the problem
>> will be easier to understand for me with a reproducer.
>>
>
> audio.json:
>
> ```
> ##
> # = Audio
> ##
>
> ##
> # @AudiodevPerDirectionOptions:
> #
> # General audio backend options that are used for both playback and
> # recording.
> #
> ```
>
> Modify this excerpt to have a comment after the "= Audio" header, say for
> instance if you were to take out that intro paragraph and transform it into
> a comment that preceded the AudiodevPerDirectionOptions doc block.
>
> e.g.
>
> ```
> ##
> # = Audio
> ##
>
> # Lorem Ipsum
>
> ##
> # @AudiodevPerDirectionOptions:
> ```
>
> the parser breaks because the line I changed that primes the next token is
> still set to "not ignore comments", but that breaks the parser and gives a
> rather unhelpful message:
>
> ../qapi/audio.json:13:1: junk after '##' at start of documentation comment
When ._parse()'s loop sees a comment token in .tok, it expects it to be
the start of a doc comment block (because any other comments are to be
skipped). It then calls .get_doc(), which expects the same. The
unexpected comment token then triggers .get_doc()'s check for junk after
'##'.
> Encountered when converting developer commentary from documentation
> paragraphs to mere QAPI comments.
Your fix is correct.
It's actually a regression. Please add
Fixes: 3d035cd2cca6 (qapi: Rewrite doc comment parser)
to the commit message.
Let's add a reproducer to tests/qapi-schema/doc-good.json right in this
patch, e.g.
diff --git a/tests/qapi-schema/doc-good.json b/tests/qapi-schema/doc-good.json
index de38a386e8..8b39eb946a 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:
#
Re: [PATCH 07/20] qapi/parser: add semantic 'kind' parameter to QAPIDoc.Section
John Snow writes:
> On Thu, May 16, 2024, 2:18 AM Markus Armbruster wrote:
>
>> John Snow writes:
>>
>> > When iterating all_sections, this is helpful to be able to distinguish
>> > "members" from "features"; the only other way to do so is to
>> > cross-reference these sections against QAPIDoc.args or QAPIDoc.features,
>> > but if the desired end goal for QAPIDoc is to remove everything except
>> > all_sections, we need *something* accessible to distinguish them.
>> >
>> > To keep types simple, add this semantic parameter to the base Section
>> > and not just ArgSection; we can use this to filter out paragraphs and
>> > tagged sections, too.
>> >
>> > Signed-off-by: John Snow
>> > ---
>> > scripts/qapi/parser.py | 25 -
>> > 1 file changed, 16 insertions(+), 9 deletions(-)
>> >
>> > diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
>> > index 161768b8b96..cf4cbca1c1f 100644
>> > --- a/scripts/qapi/parser.py
>> > +++ b/scripts/qapi/parser.py
>> > @@ -613,21 +613,27 @@ class QAPIDoc:
>> >
>> > class Section:
>> > # pylint: disable=too-few-public-methods
>> > -def __init__(self, info: QAPISourceInfo,
>> > - tag: Optional[str] = None):
>> > +def __init__(
>> > +self,
>> > +info: QAPISourceInfo,
>> > +tag: Optional[str] = None,
>> > +kind: str = 'paragraph',
>> > +):
>> > # section source info, i.e. where it begins
>> > self.info = info
>> > # section tag, if any ('Returns', '@name', ...)
>> > self.tag = tag
>> > # section text without tag
>> > self.text = ''
>> > +# section type - {paragraph, feature, member, tagged}
>> > +self.kind = kind
>>
>> Hmm. .kind is almost redundant with .tag.
>>
>
> Almost, yes. But the crucial bit is members/features as you notice. That's
> the real necessity here that saves a lot of code when relying on *only*
> all_sections.
>
> (If you want to remove the other fields leaving only all_sections behind,
> this is strictly necessary.)
>
>
>> Untagged section:.kind is 'paragraph', .tag is None
>>
>> Member description: .kind is 'member', .tag matches @NAME
>>
>> Feature description: .kind is 'feature', .tag matches @NAME
>
>
>> Tagged section: .kind is 'tagged', .tag matches
>> r'Returns|Errors|Since|Notes?|Examples?|TODO'
>>
>> .kind can directly be derived from .tag except for member and feature
>> descriptions. And you want to tell these two apart in a straightforward
>> manner in later patches, as you explain in your commit message.
>>
>> If .kind is 'member' or 'feature', then self must be an ArgSection.
>> Worth a comment? An assertion?
>>
>
> No real need. The classes don't differ much in practice so there's not much
> benefit, and asserting it won't help the static typer out anyway because it
> can't remember the inference from string->type anyway.
>
> If you wanted to be FANCY, we could use string literal typing on the field
> and restrict valid values per-class, but that's so needless not even I'm
> tempted by it.
>
>
>> Some time back, I considered changing .tag for member and feature
>> descriptions to suitable strings, like your 'member' and 'feature', and
>> move the member / feature name into ArgSection. I didn't, because the
>> benefit wasn't worth the churn at the time. Perhaps it's worth it now.
>> Would it result in simpler code than your solution?
>>
>
> Not considerably, I think. Would just be shuffling around which field names
> I touch and where/when.
The way .tag works irks me. I might explore the change I proposed just
to see whether I hate the result less. On top of your work, to not
annoy you without need.
> It might actually just add some lines where I have to assert isinstance to
> do type narrowing in the generator.
>
>> Terminology nit: the section you call 'paragraph' isn't actually a
>> paragraph: it could be several paragraphs. Best to call it 'untagged',
>> as in .ensure_untagged_section().
>>
>
> Oh, I hate when you mak
Re: [PATCH 09/20] qapi/parser: add undocumented stub members to all_sections
John Snow writes:
> This helps simplify the doc generator if it doesn't have to check for
> undocumented members.
>
> Signed-off-by: John Snow
> ---
> scripts/qapi/parser.py | 20 ++--
> 1 file changed, 18 insertions(+), 2 deletions(-)
>
> diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
> index b1794f71e12..3cd8e7ee295 100644
> --- a/scripts/qapi/parser.py
> +++ b/scripts/qapi/parser.py
> @@ -740,8 +740,24 @@ def connect_member(self, member: 'QAPISchemaMember') ->
> None:
> raise QAPISemError(member.info,
> "%s '%s' lacks documentation"
> % (member.role, member.name))
> -self.args[member.name] = QAPIDoc.ArgSection(
> -self.info, '@' + member.name, 'member')
> +
> +# Insert stub documentation section for missing member docs.
> +section = QAPIDoc.ArgSection(
> +self.info, f"@{member.name}", "member")
Although I like f-strings in general, I'd pefer to stick to '@' +
member.name here, because it's simpler.
Also, let's not change 'member' to "member". Existing practice: single
quotes for string literals unless double quotes avoid escapes. Except
English prose (like error messages) is always in double quotes.
> +self.args[member.name] = section
> +
> +# Determine where to insert stub doc.
If we have some member documentation, the member doc stubs clearly must
go there. Inserting them at the end makes sense.
Else we want to put them where the parser would accept real member
documentation.
"The parser" is .get_doc(). This is what it accepts (I'm prepared to
explain this in detail if necessary):
One untagged section
Member documentation, if any
Zero ore more tagged or untagged sections
Feature documentation, if any
Zero or more tagged or untagged sections
If we there is no member documentation, this is
One untagged section
Zero ore more tagged or untagged sections
Feature documentation, if any
Zero or more tagged or untagged sections
Note that we cannot have two adjacent untagged sections (we only create
one if the current section isn't untagged; if it is, we extend it
instead). Thus, the second section must be tagged or feature
documentation.
Therefore, the member doc stubs must go right after the first section.
This is also where qapidoc.py inserts member documentation.
> +index = 0
> +for i, sect in enumerate(self.all_sections):
> +# insert after these:
> +if sect.kind in ('intro-paragraph', 'member'):
> +index = i + 1
> +# but before these:
> +elif sect.kind in ('tagged', 'feature', 'outro-paragraph'):
> +index = i
> +break
Can you describe what this does in English? As a specification; simply
paraphrasing the code is cheating. I tried, and gave up.
Above, I derived what I believe we need to do. It's simple enough: if
we have member documentation, it starts right after the first (untagged)
section, and the stub goes to the end of the member documentation.
Else, the stub goes right after the first section.
Code:
index = 1;
while self.all_sections[index].kind == 'member':
index += 1
Of course future patches I haven't seen might change the invariants in
ways that break my simple code. We'll see.
> +self.all_sections.insert(index, section)
> +
> self.args[member.name].connect(member)
>
> def connect_feature(self, feature: 'QAPISchemaFeature') -> None:
Re: [PATCH 11/20] qapi/schema: add doc_visible property to QAPISchemaDefinition
John Snow writes: > The intent here is to mark only certain definitions as visible in the > end-user docs. > > All commands and events are inherently visible. Everything else is > visible only if it is a member (or a branch member) of a type that is > visible, or if it is named as a return type for a command. > > Notably, this excludes arg_type for commands and events, and any > base_types specified for structures/unions. Those objects may still be > marked visible if they are named as members from a visible type. Why? I figure the answer is "because the transmogrifier inlines the things excluded". Correct? > This does not necessarily match the data revealed by introspection: in > this case, we want anything that we are cross-referencing in generated > documentation to be available to target. I don't get the part after the colon. > Some internal and built-in types may be marked visible with this > approach, but if they do not have a documentation block, they'll be > skipped by the generator anyway. This includes array types and built-in > primitives which do not get their own documentation objects. > > This information is not yet used by qapidoc, which continues to render > documentation exactly as it has. This information will be used by the > new qapidoc (the "transmogrifier"), to be introduced later. The new > generator verifies that all of the objects that should be rendered *are* > by failing if any cross-references are missing, verifying everything is > in place. So... we decide "doc should be visible" here, and then the transmogrifier decides again, and we check the two decisions match? > Signed-off-by: John Snow
Re: [PATCH 13/20] 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.
I'm getting vibes of someone having had hours of "fun" with Sphinx...
Can you give you an idea of how a reproducer would look like?
> 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.
Terribly sad.
> 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 34e95bd168d..cfc0cf169ef 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
Looks plausible. I lack the Sphinx-fu to say more.
Re: [PATCH 14/20] qapi: fix non-compliant JSON examples
John Snow writes: > If we parse all examples as QMP, we need them to conform to a standard > so that they render correctly. Once the QMP lexer is active for > examples, these will produce warning messages and fail the build. > > The QMP lexer still supports elisions, but they must be represented as > the value "...", so two examples have been adjusted to support that > format here. I think this could use a bit more context. I believe you're referring to docs/sphinx/qmp_lexer.py. It describes itself as "a Sphinx extension that provides a QMP lexer for code blocks." "If we parse all examples as QMP" and "Once the QMP lexer is active for examples" suggests we're *not* using it for (some?) examples. So what are we using it for? > Signed-off-by: John Snow Patch looks lovely. Hat tip to Victor Toso, who fixed up most examples two years ago. Back then we couldn't decide how to do elisions, so we left some unfixed.
Re: [PATCH 18/20] 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. Just for consistency? Or do you have other shenanigans up your sleeve? If we want the Errors sections to remain all rST lists, we should update docs/devel/qapi-code-gen.rst to say so. > Signed-off-by: John Snow
Re: [PATCH 19/20] 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.
>
> Update the QAPI parser to now prohibit special "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.
>
> 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 ".. notes::" almost everywhere, with just two "caution"
> directives. ".. admonition:: notes" is used in a few places where we had
> an ordered list of multiple notes.
>
> Signed-off-by: John Snow
> ---
> qapi/block-core.json | 30 +++
> qapi/block.json | 2 +-
> qapi/char.json| 12 +--
> qapi/control.json | 15 ++--
> qapi/dump.json| 2 +-
> qapi/introspect.json | 6 +-
> qapi/machine-target.json | 26 +++---
> qapi/machine.json | 47 +-
> qapi/migration.json | 12 +--
> qapi/misc.json| 88 +--
> qapi/net.json | 6 +-
> qapi/pci.json | 7 +-
> qapi/qdev.json| 30 +++
> qapi/qom.json | 19 ++--
> qapi/rocker.json | 16 ++--
> qapi/run-state.json | 18 ++--
> qapi/sockets.json | 10 +--
> qapi/stats.json | 22 ++---
> qapi/transaction.json | 8 +-
> qapi/ui.json | 29 +++---
> qapi/virtio.json | 12 +--
> qga/qapi-schema.json | 48 +-
> scripts/qapi/parser.py| 9 ++
> tests/qapi-schema/doc-empty-section.err | 2 +-
> tests/qapi-schema/doc-empty-section.json | 2 +-
> tests/qapi-schema/doc-good.json | 6 +-
> tests/qapi-schema/doc-good.out| 10 ++-
> tests/qapi-schema/doc-good.txt| 14 ++-
> .../qapi-schema/doc-interleaved-section.json | 2 +-
> 29 files changed, 258 insertions(+), 252 deletions(-)
Missing: update of docs/devel/qapi-code-gen.rst. Something like
diff --git a/docs/devel/qapi-code-gen.rst b/docs/devel/qapi-code-gen.rst
index f453bd3546..1a4e240adb 100644
--- a/docs/devel/qapi-code-gen.rst
+++ b/docs/devel/qapi-code-gen.rst
@@ -995,14 +995,13 @@ line "Features:", like this::
# @feature: Description text
A tagged section begins with a paragraph that starts with one of the
-following words: "Note:"/"Notes:", "Since:", "Example:"/"Examples:",
-"Returns:", "Errors:", "TODO:". It ends with the start of a new
-section.
+following words: "Since:", "Example:"/"Examples:", "Returns:",
+"Errors:", "TODO:". It ends with the start of a new section.
The second and subsequent lines of tagged sections must be indented
like this::
- # Note: Ut enim ad minim veniam, quis nostrud exercitation ullamco
+ # TODO: Ut enim ad minim veniam, quis nostrud exercitation ullamco
# laboris nisi ut aliquip ex ea commodo consequat.
#
# Duis aute irure dolor in reprehenderit in voluptate velit esse
>
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 64fe5240cc9..530af40404d 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -1510,7 +1510,7 @@
> # @mode: whether and how QEMU should create a new image, default is
> # 'absolute-paths'.
> #
> -# Note: Either @device or @node-name must be set but not both.
> +# .. note:: Either @device or @node-name must be set but not both.
The commit message talks about ".. Note::", but you actually use
".. note::". Is there a difference?
> #
> ##
> { 'struct': 'BlockdevSnapshotSync',
> @@ -1616,9 +1616,9 @@
> #
> # @unstable: Member @x-perf is experimental.
> #
> -# Note: @on-source-error and @on-target-error only affect background
> -# I/O. If an error occurs during a guest write request, the
> -# device's rerror/werror actions will be used.
> +# .. note:: @on-source-error and @on-target-error only affect backg
Re: [PATCH 20/20] 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 by converting
> "Example:" or "Examples:" lines into ".. code-block:: QMP" lines.
>
> This patch does also allow for the use of the rST syntax "Example::" by
> exempting double-colon syntax from the QAPI doc parser, but that form is
> not used by this conversion patch. The phrase "Example" here is not
> special, it is the double-colon syntax that transforms the following
> block into a code-block. By default, *this* form does not apply QMP
> highlighting.
>
> 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.
>
> 3. All code blocks are now lexed and validated as QMP; increasing
>usability of the docs and ensuring validity of example snippets.
>
> 4. Each code-block can be captioned independently without bypassing the
>QMP lexer/validator.
>
> 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
> ---
> qapi/acpi.json | 6 +-
> qapi/block-core.json| 120 --
> qapi/block.json | 60 +++--
> qapi/char.json | 36 ++--
> qapi/control.json | 16 ++--
> qapi/dump.json | 12 ++-
> qapi/machine-target.json| 3 +-
> qapi/machine.json | 79 ++---
> qapi/migration.json | 145 +++-
> qapi/misc-target.json | 33 +---
> qapi/misc.json | 48 +++
> qapi/net.json | 30 +--
> qapi/pci.json | 6 +-
> qapi/qapi-schema.json | 6 +-
> qapi/qdev.json | 15 +++-
> qapi/qom.json | 20 +++--
> qapi/replay.json| 12 ++-
> qapi/rocker.json| 12 ++-
> qapi/run-state.json | 45 ++
> qapi/tpm.json | 9 +-
> qapi/trace.json | 6 +-
> qapi/transaction.json | 3 +-
> qapi/ui.json| 62 +-
> qapi/virtio.json| 38 +
> qapi/yank.json | 6 +-
> scripts/qapi/parser.py | 15 +++-
> tests/qapi-schema/doc-good.json | 12 +--
> tests/qapi-schema/doc-good.out | 17 ++--
> tests/qapi-schema/doc-good.txt | 17 +---
> 29 files changed, 574 insertions(+), 315 deletions(-)
Missing: update of docs/devel/qapi-code-gen.rst.
> diff --git a/qapi/acpi.json b/qapi/acpi.json
> index aa4dbe57943..3da01f1b7fc 100644
> --- a/qapi/acpi.json
> +++ b/qapi/acpi.json
> @@ -111,7 +111,8 @@
> #
> # Since: 2.1
> #
> -# Example:
> +# .. code-block:: QMP
> +#:caption: Example
I wish this was a bit less verbose. Oh well, we'll live.
> #
> # -> { "execute": "query-acpi-ospm-status" }
> # <- { "return": [ { "device": "d1", "slot": "0", "slot-type": "DIMM",
> "source": 1, "status": 0},
This is rendered as a light green box with the caption on top, in
italics and centered. I'm not sure I like the use of the caption. The
previous patch's Note boxes look nicer.
The contents of the box is highlighted. I am sure I like that.
> @@ -131,7 +132,8 @@
> #
> # Since: 2.1
> #
> -# Example:
> +# .. code-block:: QMP
> +#:caption: Example
> #
> # <- { "event": "ACPI_DEVICE_OST",
> # "data": { "info": { "device": "d1", "slot": "0",
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 530af40404d..bb0447207df 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -763,7 +763,8 @@
> #
> # Since: 0.14
> #
> -# Example:
> +# .. code-block:: QMP
> +#:caption: Example
> #
> # -> { "execute": "query-block" }
> # <- {
> @@ -1167,7 +1168,8 @@
> #
> # Since: 0.14
> #
> -# Example:
> +# .. code
Re: [PATCH RESEND v7 00/12] vhost-user: support any POSIX system (tested on macOS, FreeBSD, OpenBSD)
Stefano Garzarella writes: > Hi Michael, > > On Wed, Jun 12, 2024 at 03:01:28PM GMT, Stefano Garzarella wrote: >>This series should be in a good shape, in which tree should we queue it? >>@Micheal would your tree be okay? > > Markus suggested a small change to patch 10, so do you want me to resend the > whole series, or is it okay to resend just the last 3 patches (which are also > the ones that depend on the other patch queued by Markus)? I guess you mean [PATCH v2] qapi: clarify that the default is backend dependent Message-ID: <20240611130231.83152-1-sgarz...@redhat.com> > In the last case I would ask you to queue up the first 9 patches of this > series if that is okay with you. Michael, feel free to merge the patch I queued.
Re: [PATCH 14/20] qapi: fix non-compliant JSON examples
John Snow writes: > On Fri, Jun 14, 2024, 6:55 AM Markus Armbruster wrote: > >> John Snow writes: >> >> > If we parse all examples as QMP, we need them to conform to a standard >> > so that they render correctly. Once the QMP lexer is active for >> > examples, these will produce warning messages and fail the build. >> > >> > The QMP lexer still supports elisions, but they must be represented as >> > the value "...", so two examples have been adjusted to support that >> > format here. >> >> I think this could use a bit more context. I believe you're referring >> to docs/sphinx/qmp_lexer.py. It describes itself as "a Sphinx extension >> that provides a QMP lexer for code blocks." >> > > That's our guy! I explain its use a bit more in ... some other patch, > somewhere... > > >> "If we parse all examples as QMP" and "Once the QMP lexer is active for >> examples" suggests we're *not* using it for (some?) examples. So what >> are we using it for? >> > > My incremental backup doc makes use of it; you have to "opt in" to using > the QMP lexer instead of the generic lexer. The ".. code-block:: QMP" lines I can see in a few files? Namely: docs/devel/s390-cpu-topology.rst docs/interop/bitmaps.rst docs/interop/qmp-spec.rst > The example conversion patch later in this series opts all of the qapi docs > into using it. > > ((Later, it's possible to make "Example::" choose the QMP lexer by default > on any of our generated QMP pages. (and opting out would require explicit > code-block syntax with the lexer of choice named.))) The patch does two related things: 1. Fix invalid JSON. Doesn't need justification. 2. Normalize elisions to ... You pick ... because that's what qmp_lexer.py wants. Doing both in one patch is fine. Perhaps qapi: Fix invalid JSON in examples, and normalize elisions A few examples elide part of the output. Normalize elision to exactly ... Together with the JSON fixing, this enables use of docs/sphinx/qmp_lexer.py to highlight the examples in a later patch. >> > Signed-off-by: John Snow >> >> Patch looks lovely. >> >> Hat tip to Victor Toso, who fixed up most examples two years ago. Back >> then we couldn't decide how to do elisions, so we left some unfixed. >> > > Sorry I didn't chime in back then! "..." is arbitrary, but it's what we > already use for the qmp lexer and in the incremental backup/bitmap docs, so > I figured consistency was good. It is. > The QMP lexer has syntax support for ->, <- and ... and otherwise requires > the examples to be valid JSON. It doesn't understand grammar, though, so > it's kind of "dumb", but this is one small protection.
Re: [PATCH 18/20] qapi: ensure all errors sections are uniformly typset
John Snow writes: > On Fri, Jun 14, 2024, 7:24 AM Markus Armbruster wrote: > >> John Snow writes: >> >> > Transactions have the only instance of an Errors section that isn't a >> > rST list; turn it into one. >> >> Just for consistency? Or do you have other shenanigans up your sleeve? > > Just consistency at this precise moment in time, but it's *possible* I may > introduce shenanigans for visual consistency in the rendered output, for > which having a uniform format would make mechanical conversions in the > generator easier/possible. > > It's an idea I had but didn't implement yet. I figured I'd write this patch > anyway because it isn't wrong, and you yourself seemed to believe it would > *always* be a RST list, when that isn't strictly true. > > >> If we want the Errors sections to remain all rST lists, we should update >> docs/devel/qapi-code-gen.rst to say so. >> > > OK, will do. With such an update, we could perhaps sell the patch like 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. > > >> > Signed-off-by: John Snow >> >>
Re: [PATCH 19/20] qapi: convert "Note" sections to plain rST
John Snow writes: > On Fri, Jun 14, 2024, 9:44 AM Markus Armbruster wrote: > >> 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. >> > >> > Update the QAPI parser to now prohibit special "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. >> > > Store this paragraph in your L1 cache for a moment ... > >> >> > 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 ".. notes::" almost everywhere, with just two "caution" >> > directives. ".. admonition:: notes" is used in a few places where we had >> > an ordered list of multiple notes. >> > >> > Signed-off-by: John Snow >> > --- >> > qapi/block-core.json | 30 +++ >> > qapi/block.json | 2 +- >> > qapi/char.json| 12 +-- >> > qapi/control.json | 15 ++-- >> > qapi/dump.json| 2 +- >> > qapi/introspect.json | 6 +- >> > qapi/machine-target.json | 26 +++--- >> > qapi/machine.json | 47 +- >> > qapi/migration.json | 12 +-- >> > qapi/misc.json| 88 +-- >> > qapi/net.json | 6 +- >> > qapi/pci.json | 7 +- >> > qapi/qdev.json| 30 +++ >> > qapi/qom.json | 19 ++-- >> > qapi/rocker.json | 16 ++-- >> > qapi/run-state.json | 18 ++-- >> > qapi/sockets.json | 10 +-- >> > qapi/stats.json | 22 ++--- >> > qapi/transaction.json | 8 +- >> > qapi/ui.json | 29 +++--- >> > qapi/virtio.json | 12 +-- >> > qga/qapi-schema.json | 48 +- >> > scripts/qapi/parser.py| 9 ++ >> > tests/qapi-schema/doc-empty-section.err | 2 +- >> > tests/qapi-schema/doc-empty-section.json | 2 +- >> > tests/qapi-schema/doc-good.json | 6 +- >> > tests/qapi-schema/doc-good.out| 10 ++- >> > tests/qapi-schema/doc-good.txt| 14 ++- >> > .../qapi-schema/doc-interleaved-section.json | 2 +- >> > 29 files changed, 258 insertions(+), 252 deletions(-) >> >> Missing: update of docs/devel/qapi-code-gen.rst. Something like >> >> diff --git a/docs/devel/qapi-code-gen.rst b/docs/devel/qapi-code-gen.rst >> index f453bd3546..1a4e240adb 100644 >> --- a/docs/devel/qapi-code-gen.rst >> +++ b/docs/devel/qapi-code-gen.rst >> @@ -995,14 +995,13 @@ line "Features:", like this:: >># @feature: Description text >> >> A tagged section begins with a paragraph that starts with one of the >> -following words: "Note:"/"Notes:", "Since:", "Example:"/"Examples:", >> -"Returns:", "Errors:", "TODO:". It ends with the start of a new >> -section. >> +following words: "Since:", "
Re: [PATCH 20/20] qapi: convert "Example" sections to rST
John Snow writes:
> On Fri, Jun 14, 2024 at 10:39 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 by converting
>> > "Example:" or "Examples:" lines into ".. code-block:: QMP" lines.
>> >
>> > This patch does also allow for the use of the rST syntax "Example::" by
>> > exempting double-colon syntax from the QAPI doc parser, but that form is
>> > not used by this conversion patch. The phrase "Example" here is not
>> > special, it is the double-colon syntax that transforms the following
>> > block into a code-block. By default, *this* form does not apply QMP
>> > highlighting.
>> >
>> > 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.
>> >
>> > 3. All code blocks are now lexed and validated as QMP; increasing
>> >usability of the docs and ensuring validity of example snippets.
>> >
>> > 4. Each code-block can be captioned independently without bypassing the
>> >QMP lexer/validator.
>> >
>> > 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
>> > ---
>> > qapi/acpi.json | 6 +-
>> > qapi/block-core.json| 120 --
>> > qapi/block.json | 60 +++--
>> > qapi/char.json | 36 ++--
>> > qapi/control.json | 16 ++--
>> > qapi/dump.json | 12 ++-
>> > qapi/machine-target.json| 3 +-
>> > qapi/machine.json | 79 ++---
>> > qapi/migration.json | 145 +++-
>> > qapi/misc-target.json | 33 +---
>> > qapi/misc.json | 48 +++
>> > qapi/net.json | 30 +--
>> > qapi/pci.json | 6 +-
>> > qapi/qapi-schema.json | 6 +-
>> > qapi/qdev.json | 15 +++-
>> > qapi/qom.json | 20 +++--
>> > qapi/replay.json| 12 ++-
>> > qapi/rocker.json| 12 ++-
>> > qapi/run-state.json | 45 ++
>> > qapi/tpm.json | 9 +-
>> > qapi/trace.json | 6 +-
>> > qapi/transaction.json | 3 +-
>> > qapi/ui.json| 62 +-
>> > qapi/virtio.json| 38 +
>> > qapi/yank.json | 6 +-
>> > scripts/qapi/parser.py | 15 +++-
>> > tests/qapi-schema/doc-good.json | 12 +--
>> > tests/qapi-schema/doc-good.out | 17 ++--
>> > tests/qapi-schema/doc-good.txt | 17 +---
>> > 29 files changed, 574 insertions(+), 315
Re: [PATCH 09/20] qapi/parser: add undocumented stub members to all_sections
John Snow writes:
> On Fri, Jun 14, 2024 at 4:53 AM Markus Armbruster wrote:
>
>> John Snow writes:
>>
>> > This helps simplify the doc generator if it doesn't have to check for
>> > undocumented members.
>> >
>> > Signed-off-by: John Snow
>> > ---
>> > scripts/qapi/parser.py | 20 ++--
>> > 1 file changed, 18 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
>> > index b1794f71e12..3cd8e7ee295 100644
>> > --- a/scripts/qapi/parser.py
>> > +++ b/scripts/qapi/parser.py
>> > @@ -740,8 +740,24 @@ def connect_member(self, member: 'QAPISchemaMember')
>> > -> None:
>> > raise QAPISemError(member.info,
>> > "%s '%s' lacks documentation"
>> > % (member.role, member.name))
>> > -self.args[member.name] = QAPIDoc.ArgSection(
>> > -self.info, '@' + member.name, 'member')
>> > +
>> > +# Insert stub documentation section for missing member docs.
>> > +section = QAPIDoc.ArgSection(
>> > +self.info, f"@{member.name}", "member")
>>
>> Although I like f-strings in general, I'd pefer to stick to '@' +
>> member.name here, because it's simpler.
>
> Tomayto, Tomahto. (OK.)
Apropos healthy vegetables: at some time, we might want to mass-convert
to f-strings where they are easier to read.
>> Also, let's not change 'member' to "member". Existing practice: single
>> quotes for string literals unless double quotes avoid escapes. Except
>> English prose (like error messages) is always in double quotes.
>>
>
> OK. I realize I'm not consistent in this patch either, but I'll explain
> that my using double quotes here is a black-ism that is sneaking in the
> more I use it to auto-format my patches :)
>
> Maybe time for a flag day when I move scripts/qapi to python/qemu/qapi ...
>
> (Sorry, this type of stuff is ... invisible to me, and I really do rely on
> the linters to make sure I don't do this kind of thing.)
>
>
>>
>> > +self.args[member.name] = section
>> > +
>> > +# Determine where to insert stub doc.
>>
>> If we have some member documentation, the member doc stubs clearly must
>> go there. Inserting them at the end makes sense.
>>
>> Else we want to put them where the parser would accept real member
>> documentation.
>>
>> "The parser" is .get_doc(). This is what it accepts (I'm prepared to
>> explain this in detail if necessary):
>>
>> One untagged section
>>
>> Member documentation, if any
>>
>> Zero ore more tagged or untagged sections
>>
>> Feature documentation, if any
>>
>> Zero or more tagged or untagged sections
>>
>> If we there is no member documentation, this is
>>
>> One untagged section
>>
>> Zero ore more tagged or untagged sections
>>
>> Feature documentation, if any
>>
>> Zero or more tagged or untagged sections
>>
>> Note that we cannot have two adjacent untagged sections (we only create
>> one if the current section isn't untagged; if it is, we extend it
>> instead). Thus, the second section must be tagged or feature
>> documentation.
>>
>> Therefore, the member doc stubs must go right after the first section.
>>
>> This is also where qapidoc.py inserts member documentation.
>>
>> > +index = 0
>> > +for i, sect in enumerate(self.all_sections):
>> > +# insert after these:
>> > +if sect.kind in ('intro-paragraph', 'member'):
>> > +index = i + 1
>> > +# but before these:
>> > +elif sect.kind in ('tagged', 'feature',
>> > 'outro-paragraph'):
>> > +index = i
>> > +break
>>
>> Can you describe what this does in English? As a specification; simply
>> paraphrasing the code is cheating. I tried, and gave up.
>>
>
> It inserts after any intro-paragraph or member section it finds, but before
> any tagged, feature, or outro-paragraph it finds.
>
> The loop breaks on the very first instan
Re: [PATCH 03/13] docs/qapidoc: delint a tiny portion of the module
+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 = env.config.qapidoc_srctree + "/" + self.arguments[0]
> qapidir = os.path.dirname(qapifile)
>
> try:
> @@ -523,13 +536,14 @@ def do_parse(self, rstlist, node):
> # plain self.state.nested_parse(), and so we can drop the saving
> # of title_styles and section_level that kerneldoc.py does,
> # because nested_parse_with_titles() does that for us.
> -if Use_SSI:
> +if USE_SSI:
> with switch_source_input(self.state, rstlist):
> nested_parse_with_titles(self.state, rstlist, node)
> else:
> save = self.state.memo.reporter
> self.state.memo.reporter = AutodocReporter(
> -rstlist, self.state.memo.reporter)
> +rstlist, self.state.memo.reporter
> +)
> try:
> nested_parse_with_titles(self.state, rstlist, node)
> finally:
> @@ -537,12 +551,12 @@ def do_parse(self, rstlist, node):
>
>
> def setup(app):
> -""" Register qapi-doc directive with Sphinx"""
> -app.add_config_value('qapidoc_srctree', None, 'env')
> -app.add_directive('qapi-doc', QAPIDocDirective)
> +"""Register qapi-doc directive with Sphinx"""
> +app.add_config_value("qapidoc_srctree", None, "env")
> +app.add_directive("qapi-doc", QAPIDocDirective)
>
> -return dict(
> -version=__version__,
> -parallel_read_safe=True,
> -parallel_write_safe=True
> -)
> +return {
> +"version": __version__,
> +"parallel_read_safe": True,
> +"parallel_write_safe": True,
> +}
With intersperse() deleted
Reviewed-by: Markus Armbruster
Re: [PATCH 04/13] qapi/parser: preserve indentation in QAPIDoc sections
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):
>
> termtext.extend(self._nodes_for_ifcond(section.member.ifcond))
> # TODO drop fallbacks when undocumented members are outlawed
> if section.text:
> -defn = section.text
> +defn = dedent(section.text)
> else:
> defn = [nodes.Text('Not documented')]
>
> @@ -249,7 +265,7 @@ def _nodes_for_features(self, doc):
> dlnode = nodes.definition_list()
> for section in doc.features.values():
> dlnode += self._make_dlitem(
> -[nodes.literal('', section.member.name)], section.text)
> +[nodes.literal('', section.member.name)],
> dedent(section.text))
> seen_item = True
>
> if not seen_item:
> @@ -272,9 +288,12 @@ def _nodes_for_sections(self, doc):
> continue
> snode = self._make_section(section.tag)
> if section.tag and section.tag.startswith('Example'):
> -snode += self._nodes_for_example(section.text)
> +snode += self._nodes_for_example(dedent(section.text))
> else:
> -self._parse_text_into_node(section.text, snode)
> +self._parse_text_into_node(
> +dedent(section.text) if section.tag else section.text,
> +snode,
> +)
> nodelist.append(snode)
> return nodelist
>
> diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
> index 7b13a583ac1..43167ef0ab3 100644
> --- a/scripts/qapi/parser.py
> +++ b/scripts/qapi/parser.py
> @@ -437,6 +437,7 @@ def _match_at_name_colon(string: str) ->
> Optional[Match[str]]:
> return re.match(r'@([^:]*): *', string)
>
> def get_doc_indented(self, doc: 'QAPIDoc') -> Optional[str]:
> +"""get_doc_indented preserves indentation for later rST parsing."""
A proper function comment explains what the function does. This one
merely points out one minor aspect. Easy fix: delete it. Alternative
fix: write a proper function comment.
> self.accept(False)
> line = self.get_doc_line()
> while line == '':
[...]
Just commit message and doc nitpicks, so
Reviewed-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 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 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 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 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 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 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,
> -# @ve
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
> 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 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
Markus Armbruster writes: > John Snow writes: [...] >> 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 [...] >> @@ -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, >> -# @version, @version-id, @variant and @variant-id follow the >> -# definition specified in os-release(5). Refer to the manual page >> -# for exact description of the fields. Their values are taken >> -# from the os-release file. If the file is not present in the >> -# system, or the values are not present in the file, the fields >> -# are not included. >> +# .. note:: On POSIX systems the fields @id, @name, @pretty-name, >> +#@version, @version-id, @variant and @variant-id follow the >> +#definition specified in os-release(5). Refer to the manual page for >> +#exact description of the fields. Their values are taken from the >> +#os-release file. If the file is not present in the system, or the >> +#values are not present in the file, the fields are not included. >> # >> -# On Windows the values are filled from information gathered from >> -# the system. >> +#On Windows the values are filled from information gathered from >> +#the system. > > Please don't change the indentation here. I get the same output with > > @@ -1461,7 +1462,7 @@ ># * POSIX: as defined by os-release(5) ># * Windows: contains string "server" or "client" ># > -# Notes: On POSIX systems the fields @id, @name, @pretty-name, > +# .. note:: On POSIX systems the fields @id, @name, @pretty-name, ># @version, @version-id, @variant and @variant-id follow the ># definition specified in os-release(5). Refer to the manual page ># for exact description of the fields. Their values are taken I'm blind. Actually, you change indentation of subsequent lines from 4 to 3 *everywhere*. I guess you do that to make subsequent lines line up with the directive, here "note". Everywhere else, we indent such lines by 4. Hmm. How terrible would it be not to mess with the alignment? If we want to use 3 for directives, is it worth pointing out 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, Not mentioned, and may or may not be worth mentioning: both "Note:" and "Notes:" become ".. note::", which renders as "Note". One instance quoted below. No objection to the change; you obviously double-checked it reads okay that way. >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/block-core.json b/qapi/block-core.json > index df5e07debd2..cacedfb771c 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 > # > -# Notes: In 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. > +# .. note:: In 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. > # > # Since: 1.7 > ## [...]
Re: [PATCH 04/13] qapi/parser: preserve indentation in QAPIDoc sections
John Snow writes: > On Wed, Jun 19, 2024, 8:03 AM Markus Armbruster wrote: > >> 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." >> > > Aight. > > >> > 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". >> > > Why? (I mean, I'll do it, but I don't see the semantic difference > personally) > "Parse as " vs. "Parse 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"? >> > > Poorly phrased, was thinking about emacs too much. I mean munging the text > post-hoc for the doc generator such that newlines are added or removed in > the process of re-formatting text to get the proper indentation for the new > rST form. > > In prototyping, this got messy very quickly and was difficult to correlate > source line numbers across the transformation. > > It was easier to just not munge the text at all instead of munging it and > then un-munging it. > > (semantic satiation: munge munge munge munge.) Is this about a possible alternative solution you explored? Keeping .get_doc_indented() as is, and then try to undo its damage? [...]
Re: [PATCH 04/13] qapi/parser: preserve indentation in QAPIDoc sections
John Snow writes: > On Thu, Jun 20, 2024, 11:07 AM Markus Armbruster wrote: > >> John Snow writes: >> >> > On Wed, Jun 19, 2024, 8:03 AM Markus Armbruster wrote: >> > >> >> 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." >> >> >> > >> > Aight. >> > >> > >> >> > 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". >> >> >> > >> > Why? (I mean, I'll do it, but I don't see the semantic difference >> > personally) >> > >> >> "Parse as " vs. "Parse 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"? >> >> >> > >> > Poorly phrased, was thinking about emacs too much. I mean munging the text >> > post-hoc for the doc generator such that newlines are added or removed in >> > the process of re-formatting text to get the proper indentation for the new >> > rST form. >> > >> > In prototyping, this got messy very quickly and was difficult to correlate >> > source line numbers across the transformation. >> > >> > It was easier to just not munge the text at all instead of munging it and >> > then un-munging it. >> > >> > (semantic satiation: munge munge munge munge.) >> >> Is this about a possible alternative solution you explored? Keeping >> .get_doc_indented() as is, and then try to undo its damage? >> > > precisamente. That solution was categorically worse. Since .get_doc_indented() removes N spaces of indentation, we'd want to add back N spaces of indentation. But we can't know N, so I guess we'd make do with an arbitrary number. Where would reflowing come it? I'd like you to express more clearly that you're talking about an alternative you rejected. Perhaps like this: block-level constructs such as code blocks, lists, and other such markup. The alternative would be to somehow undo .get_doc_indented()'s indentation changes in the new generator. Much messier. Feel free to add more detail to the last paragraph. >> [...]
Re: [PATCH 09/13] qapi: convert "Note" sections to plain rST
John Snow writes: > Apologies, I meant to do this but forgot there were two cases and only > nabbed one. > > Fixing. No problem at all!
Re: [PATCH 09/13] qapi: convert "Note" sections to plain rST
John Snow writes: > On Wed, Jun 19, 2024, 8:49 AM Markus Armbruster wrote: > >> John Snow writes: [...] >> > diff --git a/tests/qapi-schema/doc-interleaved-section.json >> > b/tests/qapi-schema/doc-interleaved-section.json >> > index adb29e98daa..b26bc0bbb79 100644 >> > --- a/tests/qapi-schema/doc-interleaved-section.json >> > +++ b/tests/qapi-schema/doc-interleaved-section.json >> > @@ -10,7 +10,7 @@ >> > # >> > # bao >> > # >> > -# Note: a section. >> > +# Returns: a section. >> > # >> > # @foobar: catch this >> > # >> >> "Returns:" is only valid for commands, and this is a struct. Let's use >> "TODO:" instead. >> > > Weird that it didn't prohibit it. Bug? No: it simply chokes on "description of '@foobar:' follows a section" before it can choke on "'Returns' section is only valid for commands".
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/qom.json b/qapi/qom.json
> index 8bd299265e3..5bfa0ded42c 100644
> --- a/qapi/qom.json
> +++ b/qapi/qom.json
> @@ -195,12 +195,12 @@
> #
> # @typename: the type name of an object
> #
> -# Note: objects can create properties at runtime, for example to
> -# describe links between different devices and/or objects. These
> -# properties are not included in the output of this command.
> -#
> # Returns: a list of ObjectPropertyInfo describing object properties
> #
> +# .. note:: Objects can create properties at runtime, for example to
> +#describe links between different devices and/or objects. These
> +#properties are not included in the output of this command.
> +#
> # Since: 2.12
> ##
You move the note. Commit message doesn't tell why.
> { 'command': 'qom-list-properties',
[...]
Re: [PATCH 09/13] qapi: convert "Note" sections to plain rST
John Snow writes: > On Thu, Jun 20, 2024 at 11:46 AM John Snow wrote: > >> >> >> On Thu, Jun 20, 2024, 9:35 AM Markus Armbruster wrote: >> >>> Markus Armbruster writes: >>> >>> > John Snow writes: >>> >>> [...] >>> >>> >> 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 >>> >>> [...] >>> >>> >> @@ -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, >>> >> -# @version, @version-id, @variant and @variant-id follow the >>> >> -# definition specified in os-release(5). Refer to the manual page >>> >> -# for exact description of the fields. Their values are taken >>> >> -# from the os-release file. If the file is not present in the >>> >> -# system, or the values are not present in the file, the fields >>> >> -# are not included. >>> >> +# .. note:: On POSIX systems the fields @id, @name, @pretty-name, >>> >> +#@version, @version-id, @variant and @variant-id follow the >>> >> +#definition specified in os-release(5). Refer to the manual page >>> for >>> >> +#exact description of the fields. Their values are taken from the >>> >> +#os-release file. If the file is not present in the system, or >>> the >>> >> +#values are not present in the file, the fields are not included. >>> >> # >>> >> -# On Windows the values are filled from information gathered from >>> >> -# the system. >>> >> +#On Windows the values are filled from information gathered from >>> >> +#the system. >>> > >>> > Please don't change the indentation here. I get the same output with >>> > >>> > @@ -1461,7 +1462,7 @@ >>> ># * POSIX: as defined by os-release(5) >>> ># * Windows: contains string "server" or "client" >>> ># >>> > -# Notes: On POSIX systems the fields @id, @name, @pretty-name, >>> > +# .. note:: On POSIX systems the fields @id, @name, @pretty-name, >>> ># @version, @version-id, @variant and @variant-id follow the >>> ># definition specified in os-release(5). Refer to the manual page >>> ># for exact description of the fields. Their values are taken >>> >>> I'm blind. Actually, you change indentation of subsequent lines from 4 >>> to 3 *everywhere*. I guess you do that to make subsequent lines line up >>> with the directive, here "note". >>> >>> Everywhere else, we indent such lines by 4. Hmm. How terrible would it >>> be not to mess with the alignment? >>> >>> If we want to use 3 for directives, is it worth pointing out in the >>> commit message? >>> >>> [...] >>> >> >> Let me look up some conventions and see what's the most prominent... as >> well as testing what emacs does by default (or if emacs can be configured >> to do what we want with in-tree style config. Warning: I am functionally >> inept at emacs lisp. Warning 2x: [neo]vi[m] users, you're entirely on your >> own. I'm sorry.) >> >> I use three myself by force of habit and have some mild reluctance to >> respin for that reason, but ... guess we ought to be consistent if we can. >> >> (No idea where my habit came from. Maybe it is just because it looks nice >> to me and no other reason.) >> >> ((I have no plans, nor desire, to write any kind of che
Re: [PATCH 04/13] qapi/parser: preserve indentation in QAPIDoc sections
John Snow writes: > On Fri, Jun 21, 2024 at 2:38 AM Markus Armbruster wrote: [...] >> I'd like you to express more clearly that you're talking about an >> alternative you rejected. Perhaps like this: >> >> block-level constructs such as code blocks, lists, and other such >> markup. >> >> The alternative would be to somehow undo .get_doc_indented()'s >> indentation changes in the new generator. Much messier. >> >> Feel free to add more detail to the last paragraph. >> > > Eh, I just deleted it. I recall running into troubles but I can't > articulate the precise conditions because as you point out, it's a doomed > strategy for other reasons - you can't reconstruct the proper indentation. > > This patch is still the correct way to go, so I don't have to explain my > failures at length in the commit message ... I just like giving people > clues for *why* I decided to implement things a certain way, because I > often find that more instructive than the "how". "Why" tends to be much more useful in a commit message than "how". I should be able to figure out "how" by reading the patch, whereas for "why", I may have to read the author's mind. > In this case, the "why" is > probably more properly summarized as "it's a total shitshow in that > direction, trust me" The right amount of detail is often not obvious. Use your judgement.
Re: [PATCH 09/13] qapi: convert "Note" sections to plain rST
John Snow writes: > On Fri, Jun 21, 2024 at 8:23 AM Markus Armbruster wrote: [...] >> My reason for four spaces is reducing churn. To see by how much, I >> redid your change. I found a few more notes that don't start with a >> capital letter, or don't end with a period. >> > > ^ Guess I'll re-audit for v2. Hang on to the list of cases you found. Happy to share my patch. > (Sorry for the churn, though. I obviously don't mind it as much as you do, > but I suspect I'm a lot less nimble with fiddling through git history than > you are and find the value of avoiding churn to be ... lower than you do, > in general. Respecting reviewer time is a strong argument, I apologize that > some non-mechanical changes snuck into the patch. The downside of hacking > together a very large series.) You did a good job splitting it up. Minor mistakes are bound to happen. Got to give the reviewer soemthing to find ;) [...]
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.
[...]
> diff --git a/qapi/migration.json b/qapi/migration.json
> index 85a14bb4308..849358b6387 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
[...]
> @@ -336,7 +338,35 @@
> # }
> #}
> #
> -# 5. Migration is being performed and XBZRLE is active:
> +# .. code-block:: QMP
> +#:caption: Example: Migration is being performed and XBZRLE is active
> +#
> +# -> { "execute": "query-migrate" }
> +# <- {
> +# "return":{
> +# "status":"active",
> +# "total-time":12345,
> +# "setup-time":12345,
> +# "expected-downtime":12345,
> +# "ram":{
> +# "total":1057024,
> +# "remaining":1053304,
> +# "transferred":3720,
> +# "duplicate":123,
> +# "normal":123,
> +# "normal-bytes":123456,
> +# "dirty-sync-count":15
> +# },
> +# "disk":{
> +# "total":20971520,
> +# "remaining":20880384,
> +# "transferred":91136
> +# }
> +# }
> +#}
> +#
> +# .. code-block:: QMP
> +#:caption: Example: Migration is being performed and XBZRLE is active
> #
> # -> { "execute": "query-migrate" }
> # <- {
Example accidentally duplicated.
[...]
> diff --git a/tests/qapi-schema/doc-good.json b/tests/qapi-schema/doc-good.json
> index 4b338cc0186..2774a7ce14d 100644
> --- a/tests/qapi-schema/doc-good.json
> +++ b/tests/qapi-schema/doc-good.json
> @@ -46,11 +46,13 @@
> #
> # Duis aute irure dolor
> #
> -# Example:
> +# .. code-block:: QMP
> +#:caption: Example:
See [*] below.
> #
> # -> in
> # <- out
> -# Examples:
> +# .. code-block::
> +#
Likewise.
> # - *verbatim*
> # - {braces}
> ##
> @@ -172,12 +174,13 @@
> #
> # Duis aute irure dolor
> #
> -# Example:
> +# .. code-block::
> #
> # -> in
> # <- out
> #
> -# Examples:
> +# .. code-block::
> +#
> # - *verbatim*
> # - {braces}
> #
> @@ -196,7 +199,7 @@
> # @cmd-feat1: a feature
> # @cmd-feat2: another feature
> #
> -# Example:
> +# .. code-block::
> #
> # -> in
> #
> diff --git a/tests/qapi-schema/doc-good.out b/tests/qapi-schema/doc-good.out
> index 2c9b4e419cb..347b9cb7134 100644
> --- a/tests/qapi-schema/doc-good.out
> +++ b/tests/qapi-schema/doc-good.out
> @@ -93,11 +93,13 @@ Notes:
>
> Duis aute irure dolor
>
> -Example:
> +.. code-block:: QMP
> + :caption: Example:
[*] This demonstrates the "Example: ..." is *not* recognized as a
"Example" section before the patch (compare to the change we get for
recognized sections below).
I pointed out the same issue for "Note" in review of PATCH 9, and
suggested ways to resolve it. Pick a way there, and use it here as well
>
> -> in
> <- out
> -Examples:
> +.. code-block::
> +
> - *verbatim*
> - {braces}
> doc symbol=Enum
> @@ -184,10 +186,14 @@ frobnicate
> - Ut enim ad minim veniam
>
> Duis aute irure dolor
> -section=Example
> +
> +.. code-block::
> +
> -> in
> <- out
> -section=Examples
> +
> +.. code-block::
> +
> - *verbatim*
> - {braces}
> section=Since
> @@ -199,7 +205,9 @@ If you're bored enough to read this, go see a video of
> boxed cats
> a feature
> feature=cmd-feat2
> another feature
> -section=Example
> +section=None
> +.. code-block::
> +
> -> in
>
> <- out
> diff --git a/tests/qapi-schema/doc-good.txt b/tests/qapi-schema/doc-good.txt
> index b89f35d5476..1bd31f0938d 100644
> --- a/tests/qapi-schema/doc-good.txt
> +++ b/tests/qapi-schema/doc-good.txt
> @@ -35,7 +35,10 @@ Duis aute irure dolor
>
> Example:
>
> --> in <- out Examples: - *verbatim* - {braces}
> +-> in <- out .. code-block:
Looks like Sphinx didn't recognize the .. code-block: directive.
Generator bug?
> +
> + - *verbatim*
> + - {braces}
>
>
> "Enum" (Enum)
> @@ -219,17 +222,9 @@ Notes:
>
> Duis aute irure dolor
>
> -
> -Example
> -~~~
> -
> -> in
> <- out
>
> -
> -Examples
> -
> -
> - *verbatim*
> - {braces}
>
> @@ -260,10 +255,6 @@ Features
> "cmd-feat2"
> another feature
>
> -
> -Example
> -~~~
> -
> -> in
>
> <- out
Rendering to text now loses the "Example" heading.
We render to manual pages in addition to HTML. Looks like we don't lose
"Example" there. Odd.
We render to text only for diffing purposes. The loss there could
perhaps be tolerated. Still, could you avoid it with reasonable effort?
Re: [PATCH 00/13] qapi: convert "Note" and "Example" sections to rST
You asked for a summary of my review findings. Here it is: * PATCH 01: DO-NOT-MERGE, not reviewed * PATCH 02, 05..07, 10..12: R-by or A-by * PATCH 03: R-by with straightforward minor adjustments * PATCH 04, 08: R-by with commit message and doc tweaks * PATCH 09: - Commit message tweaks - You convert "Note:" even in free-form doc, where it isn't recognized; separate patch or mention in commit message - You silently move one note; don't, separate patch, or mention in commit message - Tweak doc-interleaved-section.json - A few more notes need to start with a capital letter and / or end with a period - I don't like the indentation change * PATCH 12: Want a positive test * PATCH 13: - One hunk should be squashed into PATCH 09 - Accidentally duplicated example should be dropped - You convert "Example:" even in free-form doc, where it isn't recognized; separate patch or mention in commit message - Sphinx doesn't recognize a .. code-block: directive in doc-good.json - doc-good.txt loses "Example" - Generated HTML looks somehwat ugly Feel free to leave reverting indentation changes to me. Same for starting notes with a capital letter and ending them with a period. I'm willing to accept ugly HTML along with a promise of future improvement :)
Re: [PATCH v2 03/21] docs/qapidoc: remove unused intersperse function
John Snow writes: > This function has been unused since fd62bff901b. > > Signed-off-by: John Snow Reviewed-by: Markus Armbruster I assume you won't mind me adding a bit of polish: "since commit fd62bff901b (sphinx/qapidoc: Drop code to generate doc for simple union tag)".
Re: [PATCH v2 05/21] 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 that > removes indentation the same way get_doc_indented() did. > > This is being done for the benefit of a new 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 into: > > ``` > 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 into: > > ``` > 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. > > 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'll restore this paragraph if you don't mind: 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. > Signed-off-by: John Snow > [Edited commit message and code comments per review --js] And I'll drop this line. > Reviewed-by: Markus Armbruster
Re: [PATCH v2 03/21] docs/qapidoc: remove unused intersperse function
Accidental duplicate, please ignore.
Re: [PATCH v2 05/21] qapi/parser: preserve indentation in QAPIDoc sections
Accidental duplicate, please ignore.
Re: [PATCH v2 03/21] docs/qapidoc: remove unused intersperse function
John Snow writes: > This function has been unused since fd62bff901b. > > Signed-off-by: John Snow Reviewed-by: Markus Armbruster I assume you won't mind me adding a bit of polish: "since commit fd62bff901b (sphinx/qapidoc: Drop code to generate doc for simple union tag)".
Re: [PATCH v2 05/21] 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 that > removes indentation the same way get_doc_indented() did. > > This is being done for the benefit of a new 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 into: > > ``` > 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 into: > > ``` > 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. > > 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'll restore this paragraph if you don't mind: 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. > Signed-off-by: John Snow > [Edited commit message and code comments per review --js] And I'll drop this line. > Reviewed-by: Markus Armbruster
Re: [PATCH v2 13/21] 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 Reviewed-by: Markus Armbruster
Re: [PATCH v2 04/21] 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 > Reviewed-by: Markus Armbruster Not an objection, just so you know: I still see a few C0411 like 'third party import "import sphinx" should be placed before ...' R-by stands.
Re: [PATCH v2 07/21] 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 > Acked-by: Markus Armbruster Generated HTML changes, but the diff is hard to review due to id attribute changes all over the place. Generated qemu-ga-ref.7 also changes: diff -rup old/qemu-ga-ref.7 new/qemu-ga-ref.7 --- old/qemu-ga-ref.7 2024-06-27 10:42:21.466096276 +0200 +++ new/qemu-ga-ref.7 2024-06-27 10:45:36.502414099 +0200 @@ -397,6 +397,7 @@ shutdown request, with no guarantee of s .B \fBmode\fP: \fBstring\fP (optional) \(dqhalt\(dq, \(dqpowerdown\(dq (default), or \(dqreboot\(dq .UNINDENT +.sp This command does NOT return a response on success. Success condition is indicated by the VM exiting with a zero exit status or, when running with \-\-no\-shutdown, by issuing the query\-status QMP @@ -1348,6 +1349,7 @@ the new password entry string, base64 en .B \fBcrypted\fP: \fBboolean\fP true if password is already crypt()d, false if raw .UNINDENT +.sp If the \fBcrypted\fP flag is true, it is the caller\(aqs responsibility to ensure the correct crypt() encryption scheme is used. This command does not attempt to interpret or report on the encryption scheme. We add vertical space. Visible when viewed with man. Looks like an improvement to me. Here's the first of these two spots in HTML: - -guest-shutdown (Command) + +guest-shutdown (Command) Initiate guest-activated shutdown. Note: this is an asynchronous shutdown request, with no guarantee of successful shutdown. @@ -502,22 +502,20 @@ shutdown request, with no guarantee of s - This command does NOT return a response on success. Success condition is indicated by the VM exiting with a zero exit status or, when running with –no-shutdown, by issuing the query-status QMP command to confirm the VM status is “shutdown”. - - -Since + +Since 0.15.0 The id changes muddy the waters. With them manually removed: guest-shutdown (Command) Initiate guest-activated shutdown. Note: this is an asynchronous shutdown request, with no guarantee of successful shutdown. @@ -502,22 +502,20 @@ shutdown request, with no guarantee of s - This command does NOT return a response on success. Success condition is indicated by the VM exiting with a zero exit status or, when running with –no-shutdown, by issuing the query-status QMP command to confirm the VM status is “shutdown”. - Since 0.15.0 Makes no visual difference in my browser. Do these differences match your expectations?
Re: [PATCH v2 10/21] 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. Markup is also re-aligned to > the de-facto standard of 3 spaces for directives. > > 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 ... >... dolor sit amet ... >... consectetur adipiscing elit ... > > ... 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. Several instances of "Notes:" have been converted to merely > ".. note::" where appropriate, but ".. 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. I looked for hunks that don't 1:1 replace "Note:" or "Notes:" by ".. note::." Findings: * Two hunks replace by ".. caution::" instead. Commit message got it. Good. * Four hunks replace by ".. admonition:: notes", one of them as a test. Commit message got it. Good. * Three hunks split "Notes:" into multiple ".. note::". Good, but could be mentioned in commit message. * Two hunks drop "Note:", changing it into paragraph. The paragraph merges into the preceding "Example" section. Good, but should be mentioned in the commit message, or turned into a separate patch. * One hunk adjusts a test case for the removal of the "Note:" tag. Good, but could be mentioned in the commit message. Perhaps tweak the paragraph above: This patch uses ".. note::" almost everywhere, with just two "caution" directives. Several instances of "Notes:" have been converted to merely ".. note::", or multiple ".. note::" where appropriate. ".. 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. Two "Note:" following "Example:" have been turned into ordinary paragraphs within the example. Okay? > NOTE: Because qapidoc.py does not attempt to preserve source ordering of > sections, the conversion of Notes from a "tagged section" to an > "untagged section" means that rendering order for some notes *may > change* as a result of this patch. The forthcoming qapidoc.py rewrite > strictly preserves source ordering in the rendered documentation, so > this issue will be rectified in the new generator. > > Signed-off-by: John Snow > Acked-by: Stefan Hajnoczi [for block*.json] I dislike the indentation changes, and may revert them in my tree. Reviewed-by: Markus Armbruster
Re: [PATCH v2 14/21] docs/qapidoc: factor out do_parse()
John Snow writes: > Factor out the compatibility parser helper so it can be shared by other > directives. Suggest "Factor out the compatibility parser helper into a base class, so it can be shared by other directives." > > Signed-off-by: John Snow > --- > docs/sphinx/qapidoc.py | 64 +++--- > 1 file changed, 35 insertions(+), 29 deletions(-) > > diff --git a/docs/sphinx/qapidoc.py b/docs/sphinx/qapidoc.py > index efcd84656fa..43dd99e21e6 100644 > --- a/docs/sphinx/qapidoc.py > +++ b/docs/sphinx/qapidoc.py > @@ -494,7 +494,41 @@ def visit_module(self, name): > super().visit_module(name) > > > -class QAPIDocDirective(Directive): > +class NestedDirective(Directive): > +def run(self): > +raise NotImplementedError Should this class be abstract? > + > +def do_parse(self, rstlist, node): > +""" > +Parse rST source lines and add them to the specified node > + > +Take the list of rST source lines rstlist, parse them as > +rST, and add the resulting docutils nodes as children of node. > +The nodes are parsed in a way that allows them to include > +subheadings (titles) without confusing the rendering of > +anything else. > +""" > +# This is from kerneldoc.py -- it works around an API change in > +# Sphinx between 1.6 and 1.7. Unlike kerneldoc.py, we use > +# sphinx.util.nodes.nested_parse_with_titles() rather than the > +# plain self.state.nested_parse(), and so we can drop the saving > +# of title_styles and section_level that kerneldoc.py does, > +# because nested_parse_with_titles() does that for us. > +if USE_SSI: > +with switch_source_input(self.state, rstlist): > +nested_parse_with_titles(self.state, rstlist, node) > +else: > +save = self.state.memo.reporter > +self.state.memo.reporter = AutodocReporter( > +rstlist, self.state.memo.reporter > +) > +try: > +nested_parse_with_titles(self.state, rstlist, node) > +finally: > +self.state.memo.reporter = save > + > + > +class QAPIDocDirective(NestedDirective): > """Extract documentation from the specified QAPI .json file""" > > required_argument = 1 > @@ -532,34 +566,6 @@ def run(self): > # so they are displayed nicely to the user > raise ExtensionError(str(err)) from err > > -def do_parse(self, rstlist, node): > -"""Parse rST source lines and add them to the specified node > - > -Take the list of rST source lines rstlist, parse them as > -rST, and add the resulting docutils nodes as children of node. > -The nodes are parsed in a way that allows them to include > -subheadings (titles) without confusing the rendering of > -anything else. > -""" > -# This is from kerneldoc.py -- it works around an API change in > -# Sphinx between 1.6 and 1.7. Unlike kerneldoc.py, we use > -# sphinx.util.nodes.nested_parse_with_titles() rather than the > -# plain self.state.nested_parse(), and so we can drop the saving > -# of title_styles and section_level that kerneldoc.py does, > -# because nested_parse_with_titles() does that for us. > -if USE_SSI: > -with switch_source_input(self.state, rstlist): > -nested_parse_with_titles(self.state, rstlist, node) > -else: > -save = self.state.memo.reporter > -self.state.memo.reporter = AutodocReporter( > - rstlist, self.state.memo.reporter > -) > -try: > -nested_parse_with_titles(self.state, rstlist, node) > -finally: > -self.state.memo.reporter = save > - > > def setup(app): > """Register qapi-doc directive with Sphinx""" Reviewed-by: Markus Armbruster
Re: [PATCH v2 15/21] docs/qapidoc: create qmp-example directive
John Snow writes:
> This is a directive that creates a syntactic sugar for creating
> "Example" boxes very similar to the ones already used in the bitmaps.rst
> document, please see e.g.
> https://www.qemu.org/docs/master/interop/bitmaps.html#creation-block-dirty-bitmap-add
>
> In its simplest form, when a custom title is not needed or wanted, and
> the example body is *solely* a QMP example:
>
> ```
> .. qmp-example::
>
>{body}
> ```
>
> is syntactic sugar for:
>
> ```
> .. admonition:: Example:
>
>.. code-block:: QMP
>
> {body}
> ```
>
> When a custom, plaintext title that describes the example is desired,
> this form:
>
> ```
> .. qmp-example::
>:title: Defrobnification
>
>{body}
> ```
>
> Is syntactic sugar for:
>
> ```
> .. admonition:: Example: Defrobnification
>
>.. code-block:: QMP
>
> {body}
> ```
>
> Lastly, when Examples are multi-step processes that require non-QMP
> exposition, have lengthy titles, or otherwise involve prose with rST
> markup (lists, cross-references, etc), the most complex form:
>
> ```
> .. qmp-example::
>:annotated:
>
>This example shows how to use `foo-command`::
>
> {body}
> ```
>
> Is desugared to:
>
> ```
> .. admonition:: Example:
>
>This example shows how to use `foo-command`::
>
> {body}
>
>For more information, please see `frobnozz`.
> ```
Can we combine the latter two? Like this:
.. qmp-example::
:title: Defrobnification
:annotated:
This example shows how to use `foo-command`::
{body}
> The primary benefit here being documentation source consistently using
> the same directive for all forms of examples to ensure consistent visual
> styling, and ensuring all relevant prose is visually grouped alongside
> the code literal block.
>
> Note that as of this commit, the code-block rST syntax "::" does not
> apply QMP highlighting; you would need to use ".. code-block:: QMP". The
> very next commit changes this behavior to assume all "::" code blocks
> within this directive are QMP blocks.
>
> Signed-off-by: John Snow
> ---
> docs/sphinx/qapidoc.py | 60 --
> 1 file changed, 58 insertions(+), 2 deletions(-)
No tests? Hmm, I see you convert existing tests in PATCH 19-21. While
that works, test coverage now would make it easier to see how each patch
affects doc generator output.
> diff --git a/docs/sphinx/qapidoc.py b/docs/sphinx/qapidoc.py
> index 43dd99e21e6..a2fa05fc491 100644
> --- a/docs/sphinx/qapidoc.py
> +++ b/docs/sphinx/qapidoc.py
> @@ -27,16 +27,19 @@
> import os
> import re
> import textwrap
> +from typing import List
>
> from docutils import nodes
> -from docutils.parsers.rst import Directive, directives
> +from docutils.parsers.rst import directives
> from docutils.statemachine import ViewList
> from qapi.error import QAPIError, QAPISemError
> from qapi.gen import QAPISchemaVisitor
> from qapi.schema import QAPISchema
>
> import sphinx
> +from sphinx.directives.code import CodeBlock
> from sphinx.errors import ExtensionError
> +from sphinx.util.docutils import SphinxDirective
> from sphinx.util.nodes import nested_parse_with_titles
>
>
> @@ -494,7 +497,7 @@ def visit_module(self, name):
> super().visit_module(name)
>
>
> -class NestedDirective(Directive):
> +class NestedDirective(SphinxDirective):
What is this about?
> def run(self):
> raise NotImplementedError
>
> @@ -567,10 +570,63 @@ def run(self):
> raise ExtensionError(str(err)) from err
>
>
> +class QMPExample(CodeBlock, NestedDirective):
> +"""
> +Custom admonition for QMP code examples.
> +
> +When the :annotated: option is present, the body of this directive
> +is parsed as normal rST instead. Code blocks must be explicitly
> +written by the user, but this allows for intermingling explanatory
> +paragraphs with arbitrary rST syntax and code blocks for more
> +involved examples.
> +
> +When :annotated: is absent, the directive body is treated as a
> +simple standalone QMP code block literal.
> +"""
> +
> +required_argument = 0
> +optional_arguments = 0
> +has_content = True
> +option_spec = {
> +"annotated": directives.flag,
> +"title": directives.unchanged,
> +}
> +
> +def admonition_wrap(self, *content) -> List[nodes.Node]:
> +title = "Example:"
> +if "title" in self.options:
> +title = f"{title} {self.options['title']}"
> +
> +admon = nodes.admonition(
> +"",
> +nodes.title("", title),
> +*content,
> +classes=["admonition", "admonition-example"],
> +)
> +return [admon]
> +
> +def run_annotated(self) -> List[nodes.Node]:
> +content_node: nodes.Element = nodes.section()
> +self.do_parse(self.content, content_node)
> +return content_node.children
> +
> +def run(self) -> List[nodes.Node]:
> +
Re: [PATCH v2 07/21] docs/qapidoc: fix nested parsing under untagged sections
John Snow writes: > On Fri, Jun 28, 2024, 3:55 AM Markus Armbruster wrote: > >> 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 >> > Acked-by: Markus Armbruster >> >> Generated HTML changes, but the diff is hard to review due to id >> attribute changes all over the place. >> >> Generated qemu-ga-ref.7 also changes: >> >> diff -rup old/qemu-ga-ref.7 new/qemu-ga-ref.7 >> --- old/qemu-ga-ref.7 2024-06-27 10:42:21.466096276 +0200 >> +++ new/qemu-ga-ref.7 2024-06-27 10:45:36.502414099 +0200 >> @@ -397,6 +397,7 @@ shutdown request, with no guarantee of s >> .B \fBmode\fP: \fBstring\fP (optional) >> \(dqhalt\(dq, \(dqpowerdown\(dq (default), or \(dqreboot\(dq >> .UNINDENT >> +.sp >> This command does NOT return a response on success. Success >> condition is indicated by the VM exiting with a zero exit status or, >> when running with \-\-no\-shutdown, by issuing the query\-status QMP >> @@ -1348,6 +1349,7 @@ the new password entry string, base64 en >> .B \fBcrypted\fP: \fBboolean\fP >> true if password is already crypt()d, false if raw >> .UNINDENT >> +.sp >> If the \fBcrypted\fP flag is true, it is the caller\(aqs >> responsibility to >> ensure the correct crypt() encryption scheme is used. This command >> does not attempt to interpret or report on the encryption scheme. >> >> We add vertical space. Visible when viewed with man. Looks like an >> improvement to me. >> >> Here's the first of these two spots in HTML: [...] >> The id changes muddy the waters. With them manually removed: [...] >> Makes no visual difference in my browser. >> >> Do these differences match your expectations? >> > > Yep! > > It does change the output just a little, but Sphinx really doesn't like > title-less sections. > > I thought the change looked fine, and I'm still planning on removing this > old generator anyway, so... I'll add to the commit message The resulting output changes are basically invisible. Thanks!
Re: [PATCH v2 10/21] 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. Markup is also re-aligned to > the de-facto standard of 3 spaces for directives. > > 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 ... >... dolor sit amet ... >... consectetur adipiscing elit ... > > ... 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. Several instances of "Notes:" have been converted to merely > ".. note::" where appropriate, but ".. 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. > > NOTE: Because qapidoc.py does not attempt to preserve source ordering of > sections, the conversion of Notes from a "tagged section" to an > "untagged section" means that rendering order for some notes *may > change* as a result of this patch. The forthcoming qapidoc.py rewrite > strictly preserves source ordering in the rendered documentation, so > this issue will be rectified in the new generator. > > Signed-off-by: John Snow > Acked-by: Stefan Hajnoczi [for block*.json] [...] > diff --git a/qapi/migration.json b/qapi/migration.json > index 9ec9ef36c47..26ad5e5e7a3 100644 > --- a/qapi/migration.json > +++ b/qapi/migration.json > @@ -1456,8 +1456,8 @@ > # > # Cancel the current executing migration process. > # > -# Notes: This command succeeds even if there is no migration process > -# running. > +# .. note:: This command succeeds even if there is no migration process > +#running. > # > # Since: 0.14 > # > @@ -1589,16 +1589,16 @@ > # > # Since: 0.14 > # > -# Notes: > +# .. admonition:: Notes > # > # 1. The 'query-migrate' command should be used to check > #migration's progress and final result (this information is > #provided by the 'status' member) Missing period, will touch up in my tree. > # > -# 2. All boolean arguments default to false > +# 2. All boolean arguments default to false. > # > # 3. The user Monitor's "detach" argument is invalid in QMP and > -#should not be used > +#should not be used. > # > # 4. The uri argument should have the Uniform Resource Identifier > #of default destination VM. This connection will be bound to > @@ -1672,7 +1672,7 @@ > # > # Since: 2.3 > # > -# Notes: > +# .. admonition:: Notes > # > # 1. It's a bad idea to use a string for the uri, but it needs to > #stay compatible with -incoming and the format of the uri is [...]
Re: [PATCH 5/8] qapi: convert "Example" sections without titles
John Snow writes:
> Use the no-option form of ".. qmp-example::" to convert any Examples
> that do not have any form of caption or explanation whatsoever. Note
> that in a few cases, example sections are split into two or more
> separate example blocks. This is only done stylistically to create a
> delineation between two or more logically independent examples.
>
> See commit-3: "docs/qapidoc: create qmp-example directive", for a
> detailed explanation of this custom directive syntax.
>
> See commit+3: "qapi: remove "Example" doc section" for a detailed
> explanation of why.
>
> Signed-off-by: John Snow
[...]
> diff --git a/qapi/run-state.json b/qapi/run-state.json
> index 252d7d6afa7..718a3c958e9 100644
> --- a/qapi/run-state.json
> +++ b/qapi/run-state.json
[...]
> @@ -453,7 +453,7 @@
> #
> # Since: 5.0
> #
> -# Example:
> +# .. qmp-example::
> #
> # <- { "event": "GUEST_CRASHLOADED",
> # "data": { "action": "run" },
Trivial semantic conflict, we need
@@ -469,7 +469,7 @@
#
# Since: 9.1
#
-# Example:
+# .. qmp-example::
#
# <- { "event": "GUEST_PVSHUTDOWN",
# "timestamp": { "seconds": 1648245259, "microseconds": 893771 } }
> @@ -597,7 +597,7 @@
> #
> # Since: 5.2
> #
> -# Example:
> +# .. qmp-example::
> #
> # <- { "event": "MEMORY_FAILURE",
> # "data": { "recipient": "hypervisor",
[...]
Re: [PATCH 1/8] docs/qapidoc: factor out do_parse()
John Snow writes: > Factor out the compatibility parser helper into a base class, so it can > be shared by other directives. > > Signed-off-by: John Snow R-by stands.
Re: QEMU Community Call Agenda Items (July 9th, 2024)
Cc: Block layer core maintainers Philippe Mathieu-Daudé writes: > On 8/7/24 16:58, Alex Bennée wrote: >> Hi, >> The KVM/QEMU community call is at: >>https://meet.jit.si/kvmcallmeeting >>@ >>9/7/2024 14:00 UTC >> Are there any agenda items for the sync-up? > > - I don't remember who mentioned "3 phase reset and KVM", > maybe Daniel Barboza or Peter Xu. > > - Questions for block team: > > . Are there plan to remove the legacy DriveInfo? What should > we use instead, blk_by_name() and/or blk_by_qdev_id()? > > . Are there plan to move away from the IF_FOO (see blockdev::if_name) > or is it OK to use them and keep them in mind with new designs? > > . To model one HW with multiple drives (at least 3), is there > a recommended way to create that from the CLI?
Re: [PATCH 1/8] docs/qapidoc: factor out do_parse()
John Snow writes: > On Sat, Jul 6, 2024, 10:47 AM Markus Armbruster wrote: > >> John Snow writes: >> >> > Factor out the compatibility parser helper into a base class, so it can >> > be shared by other directives. >> > >> > Signed-off-by: John Snow >> >> R-by stands. > > Assuming true even if I rebase on top of the 3.x patches and do_parse() > becomes quite a bit more trivial? If you think the changes don't warrant a fresh review, keep my R-by. You may get one anyway ;)
Re: [PATCH 2/8] docs/qapidoc: create qmp-example directive
John Snow writes:
> This is a directive that creates a syntactic sugar for creating
> "Example" boxes very similar to the ones already used in the bitmaps.rst
> document, please see e.g.
> https://www.qemu.org/docs/master/interop/bitmaps.html#creation-block-dirty-bitmap-add
>
> In its simplest form, when a custom title is not needed or wanted, and
> the example body is *solely* a QMP example:
>
> ```
> .. qmp-example::
>
>{body}
> ```
>
> is syntactic sugar for:
>
> ```
> .. admonition:: Example:
>
>.. code-block:: QMP
>
> {body}
> ```
>
> When a custom, plaintext title that describes the example is desired,
> this form:
>
> ```
> .. qmp-example::
>:title: Defrobnification
>
>{body}
> ```
>
> Is syntactic sugar for:
>
> ```
> .. admonition:: Example: Defrobnification
>
>.. code-block:: QMP
>
> {body}
> ```
>
> Lastly, when Examples are multi-step processes that require non-QMP
> exposition, have lengthy titles, or otherwise involve prose with rST
> markup (lists, cross-references, etc), the most complex form:
>
> ```
> .. qmp-example::
>:annotated:
>
>This example shows how to use `foo-command`::
>
> {body}
>
>For more information, please see `frobnozz`.
> ```
>
> Is desugared to:
>
> ```
> .. admonition:: Example:
>
>This example shows how to use `foo-command`::
>
> {body}
>
>For more information, please see `frobnozz`.
> ```
>
> Note that :annotated: and :title: options can be combined together, if
> desired.
>
> The primary benefit here being documentation source consistently using
> the same directive for all forms of examples to ensure consistent visual
> styling, and ensuring all relevant prose is visually grouped alongside
> the code literal block.
>
> Note that as of this commit, the code-block rST syntax "::" does not
> apply QMP highlighting; you would need to use ".. code-block:: QMP". The
> very next commit changes this behavior to assume all "::" code blocks
> within this directive are QMP blocks.
>
> Signed-off-by: John Snow
Acked-by: Markus Armbruster
Re: [PATCH 3/8] docs/qapidoc: add QMP highlighting to annotated qmp-example blocks
John Snow writes:
> For any code literal blocks inside of a qmp-example directive, apply and
> enforce the QMP lexer/highlighter to those blocks.
>
> This way, you won't need to write:
>
> ```
> .. qmp-example::
>:annotated:
>
>Blah blah
>
>.. code-block:: QMP
>
> -> { "lorem": "ipsum" }
> ```
>
> But instead, simply:
>
> ```
> .. qmp-example::
>:annotated:
>
>Blah blah::
>
> -> { "lorem": "ipsum" }
> ```
>
> Once the directive block is exited, whatever the previous default
> highlight language was will be restored; localizing the forced QMP
> lexing to exclusively this directive.
>
> Note, if the default language is *already* QMP, this directive will not
> generate and restore redundant highlight configuration nodes. We may
> well decide that the default language ought to be QMP for any QAPI
> reference pages, but this way the directive behaves consistently no
> matter where it is used.
>
> Signed-off-by: John Snow
Sadly, the previous patch didn't add tests, so this patch's effect
isn't as easy to observe as it could be.
Acked-by: Markus Armbruster
Re: [PATCH 4/8] docs/sphinx: add CSS styling for qmp-example directive
John Snow writes: > From: Harmonie Snow > > Add CSS styling for qmp-example directives to increase readability and > consistently style all example blocks. > > Signed-off-by: Harmonie Snow > Signed-off-by: John Snow Same sadness as for the previous patch. Acked-by: Markus Armbruster
Re: [PATCH 8/8] qapi: remove "Example" doc section
John Snow writes:
> Fully eliminate the "Example" sections in QAPI doc blocks now that they
> have all been converted to arbitrary rST syntax using the
> ".. qmp-example::" directive. Update tests to match.
>
> Migrating to the new syntax
> ---
>
> The old "Example:" or "Examples:" section syntax is now caught as an
> error, but "Example::" is stil permitted as explicit rST syntax for an
> un-lexed, generic preformatted text block.
>
> ('Example' is not special in this case, any sentence that ends with "::"
> will start an indented code block in rST.)
>
> Arbitrary rST for Examples is now possible, but it's strongly
> recommended that documentation authors use the ".. qmp-example::"
> directive for consistent visual formatting in rendered HTML docs. The
> ":title:" directive option may be used to add extra information into the
> title bar for the example. The ":annotated:" option can be used to write
> arbitrary rST instead, with nested "::" blocks applying QMP formatting
> where desired.
>
> Other choices available are ".. code-block:: QMP" which will not create
> an "Example:" box, or the short-form "::" code-block syntax which will
> not apply QMP highlighting when used outside of the qmp-example
> directive.
>
> Why?
>
>
> 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.
>
> 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 qmp-example can be titled or annotated independently without
>bypassing the QMP lexer/validator.
>
>(i.e. code blocks are now for *code* only, so we don't have to
>sacrifice exposition for having lexically valid examples.)
>
> NOTE: As with the "Notes" conversion patch,
Commit d461c279737 (qapi: convert "Note" sections to plain rST).
> this patch (and those
> preceding) may change the rendering order for Examples in the
The three preceding ones, to be precise.
> current generator. The forthcoming qapidoc rewrite will fix this
> by always generating documentation in source order.
Conversions from "Example" section to plain reST may change order. This
patch converts a test, and the preceding three convert the real uses.
Does any of the patches actually change order?
> Signed-off-by: John Snow
> ---
> docs/devel/qapi-code-gen.rst| 58 -
> scripts/qapi/parser.py | 10 +-
> tests/qapi-schema/doc-good.json | 19 +++
> tests/qapi-schema/doc-good.out | 26 ++-
> tests/qapi-schema/doc-good.txt | 23 ++---
> 5 files changed, 98 insertions(+), 38 deletions(-)
>
> diff --git a/docs/devel/qapi-code-gen.rst b/docs/devel/qapi-code-gen.rst
> index ae97b335cbf..2e10a3cbd69 100644
> --- a/docs/devel/qapi-code-gen.rst
> +++ b/docs/devel/qapi-code-gen.rst
> @@ -899,7 +899,7 @@ Documentation markup
>
>
> Documentation comments can use most rST markup. In particular,
> -a ``::`` literal block can be used for examples::
> +a ``::`` literal block can be used for pre-formatted text::
>
> # ::
> #
> @@ -995,8 +995,8 @@ line "Features:", like this::
># @feature: Description text
>
> A tagged section begins with a paragraph that starts with one of the
> -following words: "Since:", "Example:"/"Examples:", "Returns:",
> -"Errors:", "TODO:". It ends with the start of a new section.
> +following words: "Since:", "Returns:", "Errors:", "TODO:". It ends with
> +the start of a new section.
>
> The second and subsequent lines of tagged sections must be indented
> like this::
> @@ -1020,13 +1020,53 @@ detailing a relevant error condition. For example::
> A "Since: x.y.z" tagged section lists the release that introduced the
> definition.
>
> -An "Example" or "Examples" section is rendered entirely
> -as literal fixed-width text. "TODO" sections are not rendered at all
> -(they are for developers, not users of QMP). In other sections, the
> -text is formatted, and rST markup can be used.
> +"TODO" sections are not rendered at all (they are for developers, not
Drop "at all"?
> +users of QMP). In other sections, the text is formatted, and rST markup
> +can be used.
> +
> +QMP Examples can be added by using the ``.. qmp-example::``
> +directive. In its simplest form, this can be used to contain a single
> +QMP code block which accepts standard JSON syntax with additional server
> +directionality indicators (``->`` and ``<-``), and elisions (``...``).
> +
> +Optionally, a plaintext title may be provided by using the ``:title:``
> +direct
Re: [PATCH 5/8] qapi: convert "Example" sections without titles
John Snow writes:
> On Sat, Jul 6, 2024, 10:42 AM Markus Armbruster wrote:
>
>> John Snow writes:
>>
>> > Use the no-option form of ".. qmp-example::" to convert any Examples
>> > that do not have any form of caption or explanation whatsoever. Note
>> > that in a few cases, example sections are split into two or more
>> > separate example blocks. This is only done stylistically to create a
>> > delineation between two or more logically independent examples.
>> >
>> > See commit-3: "docs/qapidoc: create qmp-example directive", for a
>> > detailed explanation of this custom directive syntax.
>> >
>> > See commit+3: "qapi: remove "Example" doc section" for a detailed
>> > explanation of why.
>> >
>> > Signed-off-by: John Snow
>>
>> [...]
>>
>> > diff --git a/qapi/run-state.json b/qapi/run-state.json
>> > index 252d7d6afa7..718a3c958e9 100644
>> > --- a/qapi/run-state.json
>> > +++ b/qapi/run-state.json
>>
>> [...]
>>
>> > @@ -453,7 +453,7 @@
>> > #
>> > # Since: 5.0
>> > #
>> > -# Example:
>> > +# .. qmp-example::
>> > #
>> > # <- { "event": "GUEST_CRASHLOADED",
>> > # "data": { "action": "run" },
>>
>> Trivial semantic conflict, we need
>>
>
> Caught on rebase late Fri, already fixed locally and will be in v2 (which I
> rebased on top of my sphinx 3.x patches, which change the do_parse() stuff
> too.)
With that fix
Reviewed-by: Markus Armbruster
[...]
Re: [PATCH 6/8] qapi: convert "Example" sections with titles
John Snow writes: > When an Example section has a brief explanation, convert it to a > qmp-example:: section using the :title: option. > > Rule of thumb: If the title can fit on a single line and requires no rST > markup, it's a good candidate for using the :title: option of > qmp-example. > > In this patch, trailing punctuation is removed from the title section > for consistent headline aesthetics. In just one case, specifics of the > example are removed to make the title read better. > > See commit-4: "docs/qapidoc: create qmp-example directive", for a > detailed explanation of this custom directive syntax. > > See commit+2: "qapi: remove "Example" doc section" for a detailed > explanation of why. > > Signed-off-by: John Snow Reviewed-by: Markus Armbruster
Re: [PATCH 7/8] qapi: convert "Example" sections with longer prose
John Snow writes:
> These examples require longer explanations or have explanations that
> require markup to look reasonable when rendered and so use the longer
> form of the ".. qmp-example::" directive.
>
> By using the :annotated: option, the content in the example block is
> assumed *not* to be a code block literal and is instead parsed as normal
> rST - with the exception that any code literal blocks after `::` will
> assumed to be a QMP code literal block.
>
> Note: There's one title-less conversion in this patch that comes along
> for the ride because it's part of a larger "Examples" block that was
> better to convert all at once.
>
> See commit-5: "docs/qapidoc: create qmp-example directive", for a
> detailed explanation of this custom directive syntax.
>
> See commit+1: "qapi: remove "Example" doc section" for a detailed
> explanation of why.
>
> Signed-off-by: John Snow
> ---
> qapi/block.json | 26 --
> qapi/machine.json | 30 --
> qapi/migration.json | 7 +--
> qapi/virtio.json| 24 ++--
> 4 files changed, 59 insertions(+), 28 deletions(-)
>
> diff --git a/qapi/block.json b/qapi/block.json
> index 5ddd061e964..d95e9fd8140 100644
> --- a/qapi/block.json
> +++ b/qapi/block.json
> @@ -545,31 +545,37 @@
> #
> # Since: 4.0
> #
> -# Example:
> +# .. qmp-example::
> +#:annotated:
> #
> -# Set new histograms for all io types with intervals
> -# [0, 10), [10, 50), [50, 100), [100, +inf):
> +#Set new histograms for all io types with intervals
> +#[0, 10), [10, 50), [50, 100), [100, +inf)::
> #
> # -> { "execute": "block-latency-histogram-set",
> # "arguments": { "id": "drive0",
> # "boundaries": [10, 50, 100] } }
> # <- { "return": {} }
> #
> -# Example:
> +# .. qmp-example::
> +#:annotated:
> #
> -# Set new histogram only for write, other histograms will remain
> -# not changed (or not created):
> +#Set new histogram only for write, other histograms will remain
> +#not changed (or not created)::
> #
> # -> { "execute": "block-latency-histogram-set",
> # "arguments": { "id": "drive0",
> # "boundaries-write": [10, 50, 100] } }
> # <- { "return": {} }
> #
> -# Example:
> +# .. qmp-example::
> +#:annotated:
> #
> -# Set new histograms with the following intervals:
> -# read, flush: [0, 10), [10, 50), [50, 100), [100, +inf)
> -# write: [0, 1000), [1000, 5000), [5000, +inf)
> +#Set new histograms with the following intervals:
> +#
> +#- read, flush: [0, 10), [10, 50), [50, 100), [100, +inf)
> +#- write: [0, 1000), [1000, 5000), [5000, +inf)
> +#
> +#::
> #
> # -> { "execute": "block-latency-histogram-set",
> # "arguments": { "id": "drive0",
# "boundaries": [10, 50, 100],
# "boundaries-write": [1000, 5000] } }
# <- { "return": {} }
#
# .. qmp-example::
#:title: Remove all latency histograms
#
# -> { "execute": "block-latency-histogram-set",
# "arguments": { "id": "drive0" } }
# <- { "return": {} }
##
I think using :annotated: for this one as well will look better.
[...]
Reviewed-by: Markus Armbruster
Re: [PATCH 0/8] qapi: convert example sections to qmp-example rST directives
You achieved a clean & consistent look for notes and examples in the browser. Love it!
Re: [PATCH 4/8] docs/sphinx: add CSS styling for qmp-example directive
John Snow writes: > On Tue, Jul 9, 2024 at 6:34 AM Markus Armbruster wrote: > >> John Snow writes: >> >> > From: Harmonie Snow >> > >> > Add CSS styling for qmp-example directives to increase readability and >> > consistently style all example blocks. >> > >> > Signed-off-by: Harmonie Snow >> > Signed-off-by: John Snow >> >> Same sadness as for the previous patch. >> > > Should we do anything about that? In the long run, I don't expect anyone > will actually need to care about what this directive looked like in some > intermediate state before we ever used it. If you want to evaluate the > directive in the in-between states, I recommend modifying a document and > seeing what it does; but I didn't really intend for anyone to really see it > that way. > > (Isn't it a bit overboard to write unit tests for intermediate tree > states...?) I'm not asking for temporary tests, I just wonder why you delay permanent ones until "[PATCH 8/8] qapi: remove "Example" doc section". No big deal, thus: >> Acked-by: Markus Armbruster
Re: [PATCH 8/8] qapi: remove "Example" doc section
John Snow writes:
> On Tue, Jul 9, 2024 at 6:52 AM Markus Armbruster wrote:
>
>> John Snow writes:
>>
>> > Fully eliminate the "Example" sections in QAPI doc blocks now that they
>> > have all been converted to arbitrary rST syntax using the
>> > ".. qmp-example::" directive. Update tests to match.
>> >
>> > Migrating to the new syntax
>> > ---
>> >
>> > The old "Example:" or "Examples:" section syntax is now caught as an
>> > error, but "Example::" is stil permitted as explicit rST syntax for an
>> > un-lexed, generic preformatted text block.
>> >
>> > ('Example' is not special in this case, any sentence that ends with "::"
>> > will start an indented code block in rST.)
>> >
>> > Arbitrary rST for Examples is now possible, but it's strongly
>> > recommended that documentation authors use the ".. qmp-example::"
>> > directive for consistent visual formatting in rendered HTML docs. The
>> > ":title:" directive option may be used to add extra information into the
>> > title bar for the example. The ":annotated:" option can be used to write
>> > arbitrary rST instead, with nested "::" blocks applying QMP formatting
>> > where desired.
>> >
>> > Other choices available are ".. code-block:: QMP" which will not create
>> > an "Example:" box, or the short-form "::" code-block syntax which will
>> > not apply QMP highlighting when used outside of the qmp-example
>> > directive.
>> >
>> > Why?
>> >
>> >
>> > 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.
>> >
>> > 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 qmp-example can be titled or annotated independently without
>> >bypassing the QMP lexer/validator.
>> >
>> >(i.e. code blocks are now for *code* only, so we don't have to
>> >sacrifice exposition for having lexically valid examples.)
>> >
>> > NOTE: As with the "Notes" conversion patch,
>>
>> Commit d461c279737 (qapi: convert "Note" sections to plain rST).
>>
>
> Didn't have a stable commit ID at the time, will work it in if/when the
> notes patches hit main.
They have.
>> > this patch (and those
>> > preceding) may change the rendering order for Examples in the
>>
>> The three preceding ones, to be precise.
>>
>> > current generator. The forthcoming qapidoc rewrite will fix this
>> > by always generating documentation in source order.
>>
>> Conversions from "Example" section to plain reST may change order. This
>> patch converts a test, and the preceding three convert the real uses.
>>
>> Does any of the patches actually change order?
>
> I do not actually know ...! It has the *potential* in the same exact way
> that the notes patch did, but I don't actually know if it *did*. My hunch
> is "no" because there's only one intermediate section we identified with
> the notes series, but I didn't exhaustively prove it. That's why I used the
> "may" weasel wording.
Alright, I checked.
In documentation of command announce-self, the example moves from after
the arguments to before. Unwanted change.
I can keep it in place if I insert a TODO before the example like this:
diff --git a/qapi/net.json b/qapi/net.json
index 9a723e56b5..50bfd5b681 100644
--- a/qapi/net.json
+++ b/qapi/net.json
@@ -930,6 +930,8 @@
# switches. This can be useful when network bonds fail-over the
# active slave.
#
+# TODO: This line is a hack to separate the example from the body
+#
# .. qmp-example::
#
# -> { "execute": "announce-self",
I had to delete the .doctrees cache to make sphinx-build generate
corrected output.
>>
Re: [PATCH v2 6/9] qapi: convert "Example" sections without titles
John Snow writes:
> Use the no-option form of ".. qmp-example::" to convert any Examples
> that do not have any form of caption or explanation whatsoever. Note
> that in a few cases, example sections are split into two or more
> separate example blocks. This is only done stylistically to create a
> delineation between two or more logically independent examples.
>
> See commit-3: "docs/qapidoc: create qmp-example directive", for a
> detailed explanation of this custom directive syntax.
>
> See commit+3: "qapi: remove "Example" doc section" for a detailed
> explanation of why.
>
> Note: an empty "TODO" line was added to announce-self to keep the
> example from floating up into the body; this will be addressed more
> rigorously in the new qapidoc generator.
>
> Signed-off-by: John Snow
[...]
> diff --git a/qapi/run-state.json b/qapi/run-state.json
> index 4d40c88876c..ab7116680b3 100644
> --- a/qapi/run-state.json
> +++ b/qapi/run-state.json
[...]
> @@ -469,7 +469,7 @@
> #
> # Since: 9.1
> #
> -# Example:
> +# ..qmp-example::
Lacks a space, output is messed up. Can fix in my tree.
> #
> # <- { "event": "GUEST_PVSHUTDOWN",
> # "timestamp": { "seconds": 1648245259, "microseconds": 893771 } }
[...]
R-by stands.
Re: [PATCH v2 9/9] qapi: remove "Example" doc section
John Snow writes:
> Fully eliminate the "Example" sections in QAPI doc blocks now that they
> have all been converted to arbitrary rST syntax using the
> ".. qmp-example::" directive. Update tests to match.
>
> Migrating to the new syntax
> ---
>
> The old "Example:" or "Examples:" section syntax is now caught as an
> error, but "Example::" is stil permitted as explicit rST syntax for an
> un-lexed, generic preformatted text block.
>
> ('Example' is not special in this case, any sentence that ends with "::"
> will start an indented code block in rST.)
>
> Arbitrary rST for Examples is now possible, but it's strongly
> recommended that documentation authors use the ".. qmp-example::"
> directive for consistent visual formatting in rendered HTML docs. The
> ":title:" directive option may be used to add extra information into the
> title bar for the example. The ":annotated:" option can be used to write
> arbitrary rST instead, with nested "::" blocks applying QMP formatting
> where desired.
>
> Other choices available are ".. code-block:: QMP" which will not create
> an "Example:" box, or the short-form "::" code-block syntax which will
> not apply QMP highlighting when used outside of the qmp-example
> directive.
>
> Why?
>
>
> 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.
>
> 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 qmp-example can be titled or annotated independently without
>bypassing the QMP lexer/validator.
>
>(i.e. code blocks are now for *code* only, so we don't have to
>sacrifice exposition for having lexically valid examples.)
>
> NOTE: As with the "Notes" conversion (d461c279737), this patch (and the
> three preceding) may change the rendering order for Examples in
> the current generator. The forthcoming qapidoc rewrite will fix
> this by always generating documentation in source order.
>
> Signed-off-by: John Snow
Reviewed-by: Markus Armbruster
Re: [PATCH v2 0/9] qapi: convert example sections to qmp-example rST directives
John Snow writes: > This patchset focuses on converting example sections to rST directives > using a new `.. qmp-example::` directive. Queued, thanks!
Re: [PATCH v2 6/9] qapi: convert "Example" sections without titles
John Snow writes:
> On Wed, Jul 17, 2024, 3:44 AM Markus Armbruster wrote:
>
>> John Snow writes:
>>
>> > Use the no-option form of ".. qmp-example::" to convert any Examples
>> > that do not have any form of caption or explanation whatsoever. Note
>> > that in a few cases, example sections are split into two or more
>> > separate example blocks. This is only done stylistically to create a
>> > delineation between two or more logically independent examples.
>> >
>> > See commit-3: "docs/qapidoc: create qmp-example directive", for a
>> > detailed explanation of this custom directive syntax.
>> >
>> > See commit+3: "qapi: remove "Example" doc section" for a detailed
>> > explanation of why.
>> >
>> > Note: an empty "TODO" line was added to announce-self to keep the
>> > example from floating up into the body; this will be addressed more
>> > rigorously in the new qapidoc generator.
>> >
>> > Signed-off-by: John Snow
>>
>> [...]
>>
>> > diff --git a/qapi/run-state.json b/qapi/run-state.json
>> > index 4d40c88876c..ab7116680b3 100644
>> > --- a/qapi/run-state.json
>> > +++ b/qapi/run-state.json
>>
>> [...]
>>
>> > @@ -469,7 +469,7 @@
>> > #
>> > # Since: 9.1
>> > #
>> > -# Example:
>> > +# ..qmp-example::
>>
>> Lacks a space, output is messed up. Can fix in my tree.
>
> Whoops. Wish rST was a bit pickier sometimes...
"I find it to be the Perl of ASCII-based markups." (Paolo)
>> > #
>> > # <- { "event": "GUEST_PVSHUTDOWN",
>> > # "timestamp": { "seconds": 1648245259, "microseconds": 893771
>> } }
>>
>> [...]
>>
>> R-by stands.
Re: [PATCH v5 3/3] qapi: introduce device-sync-config
Vladimir Sementsov-Ogievskiy writes: > Add command to sync config from vhost-user backend to the device. It > may be helpful when VHOST_USER_SLAVE_CONFIG_CHANGE_MSG failed or not > triggered interrupt to the guest or just not available (not supported > by vhost-user server). > > Command result is racy if allow it during migration. Let's allow the > sync only in RUNNING state. Is this still accurate? The runstate_is_running() check is gone in v4, the migration_is_running() check remains. > Signed-off-by: Vladimir Sementsov-Ogievskiy QAPI schema and QMP part: Signed-off-by: Markus Armbruster
Re: [PATCH v5 1/3] qdev-monitor: add option to report GenericError from find_device_state
Vladimir Sementsov-Ogievskiy writes: > Here we just prepare for the following patch, making possible to report > GenericError as recommended. > > This patch doesn't aim to prevent further use of DeviceNotFound by > future interfaces: > > - find_device_state() is used in blk_by_qdev_id() and qmp_get_blk() >functions, which may lead to spread of DeviceNotFound anyway > - also, nothing prevent simply copy-pasting find_device_state() calls >with false argument A possible way to reduce the likelihood of further spread: 1. Rename find_device_state() to find_device_state_legacy(). 2. New find_device_state() that reports GenericError. Could also be done in a follow-up. > > Signed-off-by: Vladimir Sementsov-Ogievskiy The patch does what it says on the tin, so Reviewed-by: Markus Armbruster
Re: [PATCH v5 0/3] vhost-user-blk: live resize additional APIs
Vladimir Sementsov-Ogievskiy writes: > ping. Markus, Eric, could someone give an ACC for QAPI part? I apologize for the delay. It was pretty bad.
Re: [PATCH v2 1/7] qapi: rename BlockJobChangeOptions to JobChangeOptions
Vladimir Sementsov-Ogievskiy writes: > We are going to move change action from block-job to job > implementation, and then move to job-* extenral APIs, deprecating > block-job-* APIs. This commit simplifies further transition. > > The commit is made by command > > git grep -l BlockJobChangeOptions | \ > xargs sed -i 's/BlockJobChangeOptions/JobChangeOptions/g' > > Signed-off-by: Vladimir Sementsov-Ogievskiy Acked-by: Markus Armbruster
Re: [PATCH v2 3/7] qapi: block-job-change: make copy-mode parameter optional
Vladimir Sementsov-Ogievskiy writes:
> We are going to add more parameters to change. We want to make possible
> to change only one or any subset of available options. So all the
> options should be optional.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy
> ---
> block/mirror.c | 4
> qapi/block-core.json | 3 ++-
> 2 files changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/block/mirror.c b/block/mirror.c
> index 2816bb1042..60e8d83e4f 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -1272,6 +1272,10 @@ static void mirror_change(BlockJob *job,
> JobChangeOptions *opts,
>
> GLOBAL_STATE_CODE();
>
> +if (!change_opts->has_copy_mode) {
> +return;
> +}
> +
> if (qatomic_read(&s->copy_mode) == change_opts->copy_mode) {
> return;
> }
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 4ec5632596..660c7f4a48 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -3071,11 +3071,12 @@
> #
> # @copy-mode: Switch to this copy mode. Currently, only the switch
> # from 'background' to 'write-blocking' is implemented.
> +# If absent, copy mode remains the same. (optional since 9.1)
> #
> # Since: 8.2
> ##
> { 'struct': 'JobChangeOptionsMirror',
> - 'data': { 'copy-mode' : 'MirrorCopyMode' } }
> + 'data': { '*copy-mode' : 'MirrorCopyMode' } }
>
> ##
> # @JobChangeOptions:
Acked-by: Markus Armbruster
Re: [PATCH v2 5/7] qapi: add job-change
Vladimir Sementsov-Ogievskiy writes:
> Add a new-style command job-change, doing same thing as
> block-job-change. The aim is finally deprecate block-job-* APIs and
> move to job-* APIs.
>
> We add a new command to qapi/block-core.json, not to
> qapi/job.json to avoid resolving json file including loops for now.
> This all would be a lot simple to refactor when we finally drop
> deprecated block-job-* APIs.
>
> @type argument of the new command immediately becomes deprecated.
Where?
> Signed-off-by: Vladimir Sementsov-Ogievskiy
> ---
> job-qmp.c| 14 ++
> qapi/block-core.json | 10 ++
> 2 files changed, 24 insertions(+)
>
> diff --git a/job-qmp.c b/job-qmp.c
> index c764bd3801..248e68f554 100644
> --- a/job-qmp.c
> +++ b/job-qmp.c
> @@ -139,6 +139,20 @@ void qmp_job_dismiss(const char *id, Error **errp)
> job_dismiss_locked(&job, errp);
> }
>
> +void qmp_job_change(JobChangeOptions *opts, Error **errp)
> +{
> +Job *job;
> +
> +JOB_LOCK_GUARD();
> +job = find_job_locked(opts->id, errp);
> +
> +if (!job) {
> +return;
> +}
> +
> +job_change_locked(job, opts, errp);
> +}
> +
> /* Called with job_mutex held. */
> static JobInfo *job_query_single_locked(Job *job, Error **errp)
> {
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 660c7f4a48..9087ce300c 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -3104,6 +3104,16 @@
> { 'command': 'block-job-change',
>'data': 'JobChangeOptions', 'boxed': true }
>
> +##
> +# @job-change:
> +#
> +# Change the block job's options.
> +#
> +# Since: 9.1
> +##
> +{ 'command': 'job-change',
> + 'data': 'JobChangeOptions', 'boxed': true }
> +
> ##
> # @BlockdevDiscardOptions:
> #
Re: [PATCH v2 6/7] qapi/block-core: derpecate block-job-change
Vladimir Sementsov-Ogievskiy writes: > That's a first step to move on newer job-* APIs. > > The difference between block-job-change and job-change is in > find_block_job_locked() vs find_job_locked() functions. What's > different? > > 1. find_block_job_locked() do check, is found job a block-job. This OK Do you mean something like find_block_job_locked() finds only block jobs, whereas find_job_locked() finds any kind of job? >when moving to more generic API, no needs to document this change. > > 2. find_block_job_locked() reports DeviceNotActive on failure, when >find_job_locked() reports GenericError. Still, for block-job-change >errors are not documented at all, so be silent in deprecated.txt as >well. > > Signed-off-by: Vladimir Sementsov-Ogievskiy
Re: [PATCH v9 2/7] block/export: add blk_by_export_id()
Vladimir Sementsov-Ogievskiy writes:
> We need it for further blockdev-replace functionality.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy
> ---
> block/export/export.c | 18 ++
> include/sysemu/block-backend-global-state.h | 1 +
> 2 files changed, 19 insertions(+)
>
> diff --git a/block/export/export.c b/block/export/export.c
> index 6d51ae8ed7..57beae7982 100644
> --- a/block/export/export.c
> +++ b/block/export/export.c
> @@ -355,3 +355,21 @@ BlockExportInfoList *qmp_query_block_exports(Error
> **errp)
>
> return head;
> }
> +
> +BlockBackend *blk_by_export_id(const char *id, Error **errp)
> +{
> +BlockExport *exp;
> +
> +exp = blk_exp_find(id);
> +if (exp == NULL) {
> +error_setg(errp, "Export '%s' not found", id);
> +return NULL;
> +}
> +
> +if (!exp->blk) {
> +error_setg(errp, "Export '%s' is empty", id);
Can this happen?
> +return NULL;
> +}
> +
> +return exp->blk;
> +}
> diff --git a/include/sysemu/block-backend-global-state.h
> b/include/sysemu/block-backend-global-state.h
> index ccb35546a1..410d0cc5c7 100644
> --- a/include/sysemu/block-backend-global-state.h
> +++ b/include/sysemu/block-backend-global-state.h
> @@ -74,6 +74,7 @@ void blk_detach_dev(BlockBackend *blk, DeviceState *dev);
> DeviceState *blk_get_attached_dev(BlockBackend *blk);
> BlockBackend *blk_by_dev(void *dev);
> BlockBackend *blk_by_qdev_id(const char *id, Error **errp);
> +BlockBackend *blk_by_export_id(const char *id, Error **errp);
> void blk_set_dev_ops(BlockBackend *blk, const BlockDevOps *ops, void
> *opaque);
>
> void blk_activate(BlockBackend *blk, Error **errp);
Re: [PATCH v9 4/7] qapi: add blockdev-replace command
Vladimir Sementsov-Ogievskiy writes:
> Add a command that can replace bs in following BdrvChild structures:
>
> - qdev blk root child
> - block-export blk root child
> - any child of BlockDriverState selected by child-name
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy
> ---
> blockdev.c | 56 +++
> qapi/block-core.json | 88 ++
> stubs/blk-by-qdev-id.c | 13 +++
> stubs/meson.build | 1 +
> 4 files changed, 158 insertions(+)
> create mode 100644 stubs/blk-by-qdev-id.c
>
> diff --git a/blockdev.c b/blockdev.c
> index ba7e90b06e..2190467022 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -3559,6 +3559,62 @@ void qmp_x_blockdev_set_iothread(const char
> *node_name, StrOrNull *iothread,
> bdrv_try_change_aio_context(bs, new_context, NULL, errp);
> }
>
> +void qmp_blockdev_replace(BlockdevReplace *repl, Error **errp)
> +{
> +BdrvChild *child = NULL;
> +BlockDriverState *new_child_bs;
> +
> +if (repl->parent_type == BLOCK_PARENT_TYPE_DRIVER) {
> +BlockDriverState *parent_bs;
> +
> +parent_bs = bdrv_find_node(repl->u.driver.node_name);
> +if (!parent_bs) {
> +error_setg(errp, "Block driver node with node-name '%s' not "
> + "found", repl->u.driver.node_name);
> +return;
> +}
> +
> +child = bdrv_find_child(parent_bs, repl->u.driver.child);
> +if (!child) {
> +error_setg(errp, "Block driver node '%s' doesn't have child "
> + "named '%s'", repl->u.driver.node_name,
> + repl->u.driver.child);
> +return;
> +}
> +} else {
> +/* Other types are similar, they work through blk */
> +BlockBackend *blk;
> +bool is_qdev = repl->parent_type == BLOCK_PARENT_TYPE_QDEV;
> +const char *id =
> +is_qdev ? repl->u.qdev.qdev_id : repl->u.export.export_id;
> +
> +assert(is_qdev || repl->parent_type == BLOCK_PARENT_TYPE_EXPORT);
> +
> +blk = is_qdev ? blk_by_qdev_id(id, errp) : blk_by_export_id(id,
> errp);
blk_by_export_id() finds export @exp, and returns the associated block
backend exp->blk. Fine.
blk_by_qdev_id() finds the device, and then searches @block_backends for
a blk with blk->dev == blk. If a device has more than one block
backend, you get the one first in @block_backends. I figure that's the
one created first.
Interface issue: when a device has multiple block backends, only one of
them can be replaced, and which one is kind of random.
Do such devices exist?
If no, could they exist?
If yes, what should we do about it now?
> +if (!blk) {
> +return;
> +}
> +
> +child = blk_root(blk);
> +if (!child) {
> +error_setg(errp, "%s '%s' is empty, nothing to replace",
> + is_qdev ? "Device" : "Export", id);
> +return;
> +}
> +}
> +
> +assert(child);
> +assert(child->bs);
> +
> +new_child_bs = bdrv_find_node(repl->new_child);
> +if (!new_child_bs) {
> +error_setg(errp, "Node '%s' not found", repl->new_child);
> +return;
> +}
> +
> +bdrv_replace_child_bs(child, new_child_bs, errp);
> +}
> +
> QemuOptsList qemu_common_drive_opts = {
> .name = "drive",
> .head = QTAILQ_HEAD_INITIALIZER(qemu_common_drive_opts.head),
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index df5e07debd..0a6f08a6e0 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -6148,3 +6148,91 @@
> ##
> { 'struct': 'DummyBlockCoreForceArrays',
>'data': { 'unused-block-graph-info': ['BlockGraphInfo'] } }
> +
> +##
> +# @BlockParentType:
> +#
> +# @qdev: block device, such as created by device_add, and denoted by
> +# qdev-id
> +#
> +# @driver: block driver node, such as created by blockdev-add, and
> +# denoted by node-name
node-name and child?
> +#
> +# @export: block export, such created by block-export-add, and
> +# denoted by export-id
> +#
> +# Since 9.1
> +##
I'm kind of unhappy with this doc comment. I feel makes sense only in
the context of its use. Let me try to avoid that:
# @driver: the parent is a block node, the child is one of its
# children.
#
# @export: the parent is a block export, the child is its block
# backend.
#
# @qdev: the parent is a device, the child is its block backend.
> +{ 'enum': 'BlockParentType',
> + 'data': ['qdev', 'driver', 'export'] }
If you take my comment, change the order here as well.
> +
> +##
> +# @BdrvChildRefQdev:
> +#
> +# @qdev-id: the device's ID or QOM path
> +#
> +# Since 9.1
> +##
> +{ 'struct': 'BdrvChildRefQdev',
> + 'data': { 'qdev-id': 'str' } }
> +
> +##
> +# @BdrvChildRefExport:
> +#
> +# @export-id: block export identifier
block-export.json calls this "block export id" in some places, and
"block export identifier" in oth
[PATCH] qapi-block-core: Clean up blockdev-snapshot-internal-sync doc
BlockdevSnapshotInternal is the arguments type of command
blockdev-snapshot-internal-sync. Its doc comment contains this note:
# .. 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.
"In a transaction" is misleading, and "if @name is empty or any
snapshot matching @name exists, the operation will fail" is redundant
with the command's Errors documentation. Drop.
The remainder is fine. Move it to the command's doc comment, where it
is more prominently visible, with a slight rephrasing for clarity.
Signed-off-by: Markus Armbruster
---
qapi/block-core.json | 7 +++
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/qapi/block-core.json b/qapi/block-core.json
index f400b334c8..994e384a71 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -6046,10 +6046,6 @@
#
# @name: the name of the internal snapshot to be created
#
-# .. 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.
-#
# Since: 1.7
##
{ 'struct': 'BlockdevSnapshotInternal',
@@ -6070,6 +6066,9 @@
# - If the format of the image used does not support it,
# GenericError
#
+# .. note:: Only some image formats such as qcow2 and rbd support
+#internal snapshots.
+#
# Since: 1.7
#
# .. qmp-example::
--
2.45.0
Re: [PATCH v2] block-backend: per-device throttling of BLOCK_IO_ERROR reports
Kevin Wolf writes:
> Am 09.01.2024 um 14:13 hat Vladimir Sementsov-Ogievskiy geschrieben:
>> From: Leonid Kaplan
>>
>> BLOCK_IO_ERROR events comes from guest, so we must throttle them.
>> We still want per-device throttling, so let's use device id as a key.
>>
>> Signed-off-by: Leonid Kaplan
>> Signed-off-by: Vladimir Sementsov-Ogievskiy
>> ---
>>
>> v2: add Note: to QAPI doc
>>
>> monitor/monitor.c| 10 ++
>> qapi/block-core.json | 2 ++
>> 2 files changed, 12 insertions(+)
>>
>> diff --git a/monitor/monitor.c b/monitor/monitor.c
>> index 01ede1babd..ad0243e9d7 100644
>> --- a/monitor/monitor.c
>> +++ b/monitor/monitor.c
>> @@ -309,6 +309,7 @@ int error_printf_unless_qmp(const char *fmt, ...)
>> static MonitorQAPIEventConf monitor_qapi_event_conf[QAPI_EVENT__MAX] = {
>> /* Limit guest-triggerable events to 1 per second */
>> [QAPI_EVENT_RTC_CHANGE]= { 1000 * SCALE_MS },
>> +[QAPI_EVENT_BLOCK_IO_ERROR]= { 1000 * SCALE_MS },
>> [QAPI_EVENT_WATCHDOG] = { 1000 * SCALE_MS },
>> [QAPI_EVENT_BALLOON_CHANGE]= { 1000 * SCALE_MS },
>> [QAPI_EVENT_QUORUM_REPORT_BAD] = { 1000 * SCALE_MS },
>> @@ -498,6 +499,10 @@ static unsigned int qapi_event_throttle_hash(const void
>> *key)
>> hash += g_str_hash(qdict_get_str(evstate->data, "qom-path"));
>> }
>>
>> +if (evstate->event == QAPI_EVENT_BLOCK_IO_ERROR) {
>> +hash += g_str_hash(qdict_get_str(evstate->data, "device"));
>> +}
>
> Using "device" only works with -drive, i.e. when the BlockBackend
> actually has a name. In modern configurations with a -blockdev
> referenced by -device, the BlockBackend doesn't have a name any more.
>
> Maybe we should be using the qdev id (or more generally, QOM path) here,
> but that's something the event doesn't even contain yet.
Uh, does the event reliably identify the I/O error's node or not? If
not, then that's a serious design defect.
There's @node-name. Several commands use "either @device or @node-name"
to identify a node. Is that sufficient here?
[...]
