Re: [PATCH 3/8] qapi: add "Details:" disambiguation marker

[Markus Armbruster <armbru@redhat.com>]

John Snow <jsnow@redhat.com> writes:

> When a documentation block consists only of plaintext, there is nothing
> to semantically differentiate the "intro" from the "details"
> section. For the purposes of the inliner, the intro section is likely to
> be dropped while the details section will be merged onto the end of the
> parent's details section.
>
> When the delineation between "intro" and "details" is not clear because
> there is no intervening "members", "features", "errors", "returns",
> "TODO", or "Since" section, the parser assumes the entire text section
> is an "intro" section. This may not always be semantically true, so this
> patch clarifies certain sections explicitly as "details" sections by
> using an empty "Details:" marker.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  qapi/block-core.json   | 3 +++
>  qapi/machine.json      | 2 +-
>  qapi/migration.json    | 8 ++++++--
>  qapi/net.json          | 2 +-
>  qapi/qom.json          | 4 ++++
>  qapi/yank.json         | 2 +-
>  scripts/qapi/parser.py | 8 ++++++++
>  7 files changed, 24 insertions(+), 5 deletions(-)

Missing: update to docs/devel/qapi-code-gen.rst.  Suggest to put in a
FIXME, so we don't forget.

>
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index f8d446b3d6e..c5ff15ae7a1 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -5007,6 +5007,9 @@
>  # @blockdev-reopen:
>  #
>  # Reopens one or more block devices using the given set of options.
> +#
> +# Details:
> +#
>  # Any option not specified will be reset to its default value
>  # regardless of its previous status.  If an option cannot be changed
>  # or a particular driver does not support reopening then the command

Rendered documentation before the patch:

    Command blockdev-reopen (Since: 6.1)

       Reopens one or more block devices using the given set of options.
       Any option not specified will be reset to its default value
       regardless of its previous status.  If an option cannot be changed
       or a particular driver does not support reopening then the command
       will return an error.  All devices in the list are reopened in one
       transaction, so if one of them fails then the whole transaction is
       cancelled.

       [...]

       Unlike with "blockdev-add", the "backing" option must always be
       present unless the node being reopened does not have a backing file
       and its image does not have a default backing file name as part of
       its metadata.

       Arguments:
          * options ([BlockdevOptions]) -- Not documented

The command's single argument is undocumented, and the doc generator
papered over this (sadly common defect) by splicing in this "Not
documented" description.

Aside: why not boxed, like blockdev-add?  Design mistake?

The patch moves the splice point.  Not wrong, but we should really fill
in the document gap instead.

> diff --git a/qapi/machine.json b/qapi/machine.json
> index 685e4e29b87..bc2279b2526 100644
> --- a/qapi/machine.json
> +++ b/qapi/machine.json
> @@ -1259,7 +1259,7 @@
>  # 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
> +# Details:

This replaces a hack by a proper solution.  More of the same below, not
mentioning this again.

>  #
>  # .. qmp-example::
>  #
> diff --git a/qapi/migration.json b/qapi/migration.json
> index 7134d4ce47e..558b4f145ed 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -1633,7 +1633,7 @@
>  #
>  # Query replication status while the vm is running.
>  #
> -# TODO: This line is a hack to separate the example from the body
> +# Details:
>  #
>  # .. qmp-example::
>  #
> @@ -1651,6 +1651,8 @@
>  #
>  # Xen uses this command to notify replication to trigger a checkpoint.
>  #
> +# Details:
> +#
>  # .. qmp-example::
>  #
>  #     -> { "execute": "xen-colo-do-checkpoint" }

No change to generated documentation.  I guess this is needed to avoid
the yelping added in the next patch.  Correct?

> @@ -1687,7 +1689,7 @@
>  #
>  # Query COLO status while the vm is running.
>  #
> -# TODO: This line is a hack to separate the example from the body
> +# Details:
>  #
>  # .. qmp-example::
>  #
> @@ -1724,6 +1726,8 @@
>  #
>  # Pause a migration.  Currently it only supports postcopy.
>  #
> +# Details:
> +#
>  # .. qmp-example::
>  #
>  #     -> { "execute": "migrate-pause" }

Likewise?

> diff --git a/qapi/net.json b/qapi/net.json
> index 118bd349651..c011d6dc1a9 100644
> --- a/qapi/net.json
> +++ b/qapi/net.json
> @@ -1070,7 +1070,7 @@
>  # 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
> +# Details:
>  #
>  # .. qmp-example::
>  #
> diff --git a/qapi/qom.json b/qapi/qom.json
> index 1b47abd44e9..568b7d4b997 100644
> --- a/qapi/qom.json
> +++ b/qapi/qom.json
> @@ -777,6 +777,8 @@
>  #
>  # Properties for memory-backend-shm objects.
>  #
> +# Details:
> +#
>  # This memory backend supports only shared memory, which is the
>  # default.
>  #

Rendered documentation changes from

    Object MemoryBackendShmProperties (Since: 9.1)
        Availability: CONFIG_POSIX

       Properties for memory-backend-shm objects.

       This memory backend supports only shared memory, which is the
       default.

       Members:
          * The members of MemoryBackendProperties.

to

    Object MemoryBackendShmProperties (Since: 9.1)
        Availability: CONFIG_POSIX

       Properties for memory-backend-shm objects.

       Members:
          * The members of MemoryBackendProperties.

       This memory backend supports only shared memory, which is the
       default.

Feels like a wash.

I guess we need a "Details:" somewhere to avoid yelping.  Correct?

> @@ -792,6 +794,8 @@
>  #
>  # Properties for memory-backend-epc objects.
>  #
> +# Details:
> +#
>  # The @merge boolean option is false by default with epc
>  #
>  # The @dump boolean option is false by default with epc

Similar change to rendered documentation, same request to confirm
yelping.

> diff --git a/qapi/yank.json b/qapi/yank.json
> index f3cd5c15d60..2854a8a9d2a 100644
> --- a/qapi/yank.json
> +++ b/qapi/yank.json
> @@ -104,7 +104,7 @@
>  #
>  # Query yank instances.  See `YankInstance` for more information.
>  #
> -# TODO: This line is a hack to separate the example from the body
> +# Details:
>  #
>  # .. qmp-example::
>  #

[Skipping the Python code in my first pass...]

I'd be tempted to split the patch into hack replacement and yelping
avoidance.