John Snow <js...@redhat.com> writes:
> This patch changes the qapidoc parser to generate stub Return value
> documentation for any command that has a return value but does not have
> a "Returns:" doc section.
>
> The stubs include just the type name, which will be rendered with a
> cross-reference link in the HTML output.
>
> The algorithm for where return sections are placed is as follows:
>
> 1. If we have arguments, return goes right after them.
> 2. Else if we have errors, return goes right before them.
> 3. Else if we have features, return goes right before them.
> 4. Else return goes right after the intro
>
> To facilitate this algorithm, a "TODO:" hack line is used to separate
> the intro from the remainder of the documentation block in cases where
> there are no other sections to separate the intro from e.g. examples and
> additional detail meant to appear below the key sections of interest.
Good, except it neglects to spell out *why* we do this. Here's my
attempt:
qapi: Fix undocumented return values by generating something
Generated command documentation lacks information on return value in
several cases, e.g. query-tpm.
The obvious fix would be to require a Returns: section when a command
returns something.
However, note that many existing Returns: sections are pretty useless:
the description is basically the return type, which then gets rendered
like "Return: <Type> – <basically the return type>". This suggests
that a description is often not really necessary, and requiring one
isn't useful.
Instead, generate the obvious minimal thing when Returns: is absent:
"Return: <Type>".
This auto-generated Return documentation is placed is as follows:
>
> Signed-off-by: John Snow <js...@redhat.com>
> ---
> docs/sphinx/qapidoc.py | 14 ++++++++------
> qapi/machine.json | 2 ++
> scripts/qapi/parser.py | 35 +++++++++++++++++++++++++++++++++++
> scripts/qapi/schema.py | 3 +++
> 4 files changed, 48 insertions(+), 6 deletions(-)
>
> diff --git a/docs/sphinx/qapidoc.py b/docs/sphinx/qapidoc.py
> index 8011ac9efaf..77e28a65cfc 100644
> --- a/docs/sphinx/qapidoc.py
> +++ b/docs/sphinx/qapidoc.py
> @@ -255,16 +255,18 @@ def visit_feature(self, section: QAPIDoc.ArgSection) ->
> None:
> def visit_returns(self, section: QAPIDoc.Section) -> None:
> assert isinstance(self.entity, QAPISchemaCommand)
> rtype = self.entity.ret_type
> - # q_empty can produce None, but we won't be documenting anything
> - # without an explicit return statement in the doc block, and we
> - # should not have any such explicit statements when there is no
> - # return value.
> + # return statements will not be present (and won't be
> + # autogenerated) for any command that doesn't return
> + # *something*, so rtype will always be defined here.
> assert rtype
>
> typ = self.format_type(rtype)
> assert typ
> - assert section.text
> - self.add_field("return", typ, section.text, section.info)
> +
> + if section.text:
> + self.add_field("return", typ, section.text, section.info)
> + else:
> + self.add_lines(f":return-nodesc: {typ}", section.info)
>
> def visit_errors(self, section: QAPIDoc.Section) -> None:
> # FIXME: the formatting for errors may be inconsistent and may
> diff --git a/qapi/machine.json b/qapi/machine.json
> index f712e7da6d6..e3e0505a4b4 100644
> --- a/qapi/machine.json
> +++ b/qapi/machine.json
> @@ -1301,6 +1301,8 @@
> # Return the amount of initially allocated and present hotpluggable
> # (if enabled) memory in bytes.
> #
> +# TODO: This line is a hack to separate the example from the body
> +#
> # .. qmp-example::
> #
> # -> { "execute": "query-memory-size-summary" }
> diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
> index 949d9e8bff7..aa8f1852c50 100644
> --- a/scripts/qapi/parser.py
> +++ b/scripts/qapi/parser.py
> @@ -815,6 +815,41 @@ def connect_feature(self, feature: 'QAPISchemaFeature')
> -> None:
> % feature.name)
> self.features[feature.name].connect(feature)
>
> + def ensure_returns(self, info: QAPISourceInfo) -> None:
> +
> + def _insert_near_kind(
> + kind: QAPIDoc.Kind,
> + new_sect: QAPIDoc.Section,
> + after: bool = False,
> + ) -> bool:
> + for sect in filter(lambda sect: sect.kind == kind, reversed(
> + self.all_sections)):
> + idx = self.all_sections.index(sect) + (1 if after else 0)
> + self.all_sections.insert(idx, new_sect)
> + return True
> + return False
I find this function a bit hard to understand.
filter() returns an iterator for the sections of kind @kind in
@all_sections.
The for loop doesn't actually loop, it executes its body at most once.
If there are no sections of kind @kind, it doesn't execute the body, and
returns False.
If there are such sections, the body is executed for the first one,
i.e. the last section of kind @kind in @all_sections. It then searches
@all_sections again to find its index, inserts the new section before or
after that index, and returns True.
Here's my attempt to write less clever code:
def _insert_near_kind(
kind: QAPIDoc.Kind,
new_sect: QAPIDoc.Section,
after: bool = False,
) -> bool:
for idx, sect in enumerate(reversed(self.all_sections)):
if sect.kind == kind:
pos = len(self.all_sections) - idx - 1
if after:
pos += 1
self.all_sections.insert(pos, new_sect)
return True
return False
What do you think?
> +
> + if any(s.kind == QAPIDoc.Kind.RETURNS for s in self.all_sections):
> + return
> +
> + # Stub "Returns" section for undocumented returns value
> + stub = QAPIDoc.Section(info, QAPIDoc.Kind.RETURNS)
> +
> + if any(_insert_near_kind(kind, stub, after) for kind, after in (
> + # 1. If arguments, right after those.
> + (QAPIDoc.Kind.MEMBER, True),
> + # 2. Elif errors, right *before* those.
> + (QAPIDoc.Kind.ERRORS, False),
> + # 3. Elif features, right *before* those.
> + (QAPIDoc.Kind.FEATURE, False),
> + )):
> + return
> +
> + # Otherwise, it should go right after the intro. The intro
> + # is always the first section and is always present (even
> + # when empty), so we can insert directly at index=1 blindly.
> + self.all_sections.insert(1, stub)
> +
> def check_expr(self, expr: QAPIExpression) -> None:
> if 'command' in expr:
> if self.returns and 'returns' not in expr:
> diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
> index cbe3b5aa91e..3abddea3525 100644
> --- a/scripts/qapi/schema.py
> +++ b/scripts/qapi/schema.py
> @@ -1062,6 +1062,9 @@ def connect_doc(self, doc: Optional[QAPIDoc] = None) ->
> None:
> if self.arg_type and self.arg_type.is_implicit():
> self.arg_type.connect_doc(doc)
>
> + if self.ret_type and self.info:
> + doc.ensure_returns(self.info)
> +
> def visit(self, visitor: QAPISchemaVisitor) -> None:
> super().visit(visitor)
> visitor.visit_command(
Let's check how generated output changes:
diff -rup 01-qapi-gen-1711c67f7c/qemu-qmp-ref.txt
02-qapi-gen-cfb5bf6462/qemu-qmp-ref.txt
--- 01-qapi-gen-1711c67f7c/qemu-qmp-ref.txt 2025-07-11 11:38:27.142499312
+0200
+++ 02-qapi-gen-cfb5bf6462/qemu-qmp-ref.txt 2025-07-11 11:41:35.193205635
+0200
@@ -3607,6 +3607,9 @@ Command x-debug-query-block-graph (Since
Get the block graph.
+ Return:
+ "XDbgBlockGraph"
+
Clause 3. Else if we have features, return goes right before them.
Good.
Features:
* **unstable** -- This command is meant for debugging.
@@ -10350,6 +10353,9 @@ Command query-tpm (Since: 1.5)
Return information about the TPM device
+ Return:
+ "[""TPMInfo""]"
+
Clause 4. Else return goes right after the intro. Good.
Note: schema source terminates intro with Since: section.
Example::
-> { "execute": "query-tpm" }
@@ -13991,6 +13997,9 @@ Command query-dirty-rate (Since: 5.2)
which to report calculation time. By default it is reported in
seconds. (Since 8.2)
+ Return:
+ "DirtyRateInfo"
+
Clause 1. If we have arguments, return goes right after them. Good.
Example: Measurement is in progress:
<- {"status": "measuring", "sample-pages": 512,
@@ -14061,6 +14070,9 @@ Command query-vcpu-dirty-limit (Since: 7
Return information about virtual CPU dirty page rate limits, if
any.
+ Return:
+ "[""DirtyLimitInfo""]"
+
Clause 4. Else return goes right after the intro. Good.
Example::
-> {"execute": "query-vcpu-dirty-limit"}
@@ -16775,6 +16787,9 @@ Command query-vm-generation-id (Since: 2
Show Virtual Machine Generation ID
+ Return:
+ "GuidInfo"
+
Clause 4. Else return goes right after the intro. Good.
Command system_reset (Since: 0.14)
Performs a hard reset of a guest.
@@ -17548,6 +17563,9 @@ Command query-memory-size-summary (Since
Return the amount of initially allocated and present hotpluggable
(if enabled) memory in bytes.
+ Return:
+ "MemoryInfo"
+
Clause 4. Else return goes right after the intro. Good.
Example::
-> { "execute": "query-memory-size-summary" }
@@ -17739,6 +17757,9 @@ Command query-memory-devices (Since: 2.1
Lists available memory devices and their state
+ Return:
+ "[""MemoryDeviceInfo""]"
+
Clause 4. Else return goes right after the intro. Good.
Example::
-> { "execute": "query-memory-devices" }
@@ -20012,6 +20033,9 @@ Command query-acpi-ospm-status (Since: 2
Return a list of ACPIOSTInfo for devices that support status
reporting via ACPI _OST method.
+ Return:
+ "[""ACPIOSTInfo""]"
+
Clause 4. Else return goes right after the intro. Good.
Example::
-> { "execute": "query-acpi-ospm-status" }
@@ -20498,6 +20522,9 @@ Command query-stats-schemas (Since: 7.1)
* **provider** ("StatsProvider", *optional*) -- a provider to
restrict the query to.
+ Return:
+ "[""StatsSchema""]"
+
Clause 1. If we have arguments, return goes right after them. Good.
Note:
Runtime-collected statistics and their names fall outside QEMU's
Placement is all good.
Clauses 1, 3, 4 covered. The next patch will cover clause 2.
In all instances of clause 4 but one, intro is terminated by a Since:
section.
The exception is query-memory-size-summary, which has its Since: at the
end. It therefore needs the "TODO:" hack line you add to separate intro
from example.
All but one of the intro-terminating Since: are followed by an example.
We more commonly put Since: at the very end (roughly 3 out of four
times). But if we moved these to the end, we'd need a "TODO:" hack line
to separate intro from example.
The need for terminating the intro is not obvious when writing doc
comments. Mistakes are bound to happen.
I understand the inliner similarly relies on intro-termination for
placement.
We've toyed with the idea of automating since information. If we pull
that off, the Since: go away.
I think relying on existing Since: for separation is fine for now, but
we'll likely need a more robust solution eventually.
With the commit message amended and preferably with a less clever
version of _insert_near_kind():
Reviewed-by: Markus Armbruster <arm...@redhat.com>
