Re: [PATCH 03/18] qapi: add cross-references to block layer

[Markus Armbruster <armbru@redhat.com>]

John Snow <jsnow@redhat.com> writes:

> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  qapi/block-core.json   | 186 ++++++++++++++++++++---------------------
>  qapi/block-export.json |  36 ++++----
>  qapi/block.json        |  14 ++--
>  qapi/transaction.json  |  20 ++---
>  4 files changed, 128 insertions(+), 128 deletions(-)
>
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index f18db3149a3..74e6a81a5e9 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json

[...]

> @@ -3089,7 +3089,7 @@
>  # necessary cleanup.  This command requires that all involved jobs are
>  # in the PENDING state.
>  #
> -# For jobs in a transaction, instructing one job to finalize will
> +# For jobs in a :qapi:cmd:`transaction`, instructing one job to finalize will

Why and when do we need :qapi:cmd:?

>  # force ALL jobs in the transaction to finalize, so it is only
>  # necessary to instruct a single member job to finalize.
>  #

[...]

> @@ -5855,8 +5855,8 @@
>  # @BLOCK_JOB_PENDING:
>  #
>  # Emitted when a block job is awaiting explicit authorization to
> -# finalize graph changes via @job-finalize.  If this job is part
> -# of a transaction, it will not emit this event until the transaction
> +# finalize graph changes via `job-finalize`.  If this job is part
> +# of a :qapi:cmd:`transaction`, it will not emit this event until the transaction
>  # has converged first.
>  #
>  # @type: job type

[...]

> diff --git a/qapi/transaction.json b/qapi/transaction.json
> index 9d9e7af26cb..c02e402790e 100644
> --- a/qapi/transaction.json
> +++ b/qapi/transaction.json
> @@ -11,7 +11,7 @@
>  ##
>  # @Abort:
>  #
> -# This action can be used to test transaction failure.
> +# This action can be used to test `transaction` failure.

I have doubts about this one.

The comment isn't great to begin with.  It's from commit 78b18b78aa8
(blockdev: add Abort transaction):

    blockdev: add Abort transaction
    
    The Abort action can be used to test QMP 'transaction' failure.  Add it
    as the last action to exercise the .abort() and .cleanup() code paths
    for all previous actions.

So, Abort for testing transactionable commands' handling of transaction
failure, not for testing failure of the transaction command.  But the
link points to the command, and thus suggests it's about command failure.

Ideally, we'd point to some "introduction to transactions" text.  We
don't have one.

The other occurences of `transaction` are similarly problematic, except
for one noted below.  Let's leave the problematic ones alone in this
patch, to keep it mechanical and uncontroversial.

Not this patch's problem: the transaction command's doc comment is
*crap*.  Most of it discusses about snapshots, as if snapshots where the
only transactionable commands.  It talks about "the dictionary", and the
reader can only guess what that might mean.

>  #
>  # Since: 1.6
>  ##
> @@ -67,8 +67,8 @@
>  #
>  # Features:
>  #
> -# @deprecated: Member @drive-backup is deprecated.  Use member
> -#     @blockdev-backup instead.
> +# @deprecated: Member `drive-backup` is deprecated.  Use member
> +#     `blockdev-backup` instead.
>  #
>  # Since: 1.1
>  ##
> @@ -156,7 +156,7 @@
>  # @TransactionAction:
>  #
>  # A discriminated record of operations that can be performed with
> -# @transaction.
> +# `transaction`.

This link is a keeper.

>  #
>  # @type: the operation to be performed
>  #
> @@ -187,7 +187,7 @@
>  #
>  # @completion-mode: Controls how jobs launched asynchronously by
>  #     Actions will complete or fail as a group.  See
> -#     @ActionCompletionMode for details.
> +#     `ActionCompletionMode` for details.
>  #
>  # Since: 2.5
>  ##
> @@ -224,20 +224,20 @@
>  # support it, for example, qcow2, and rbd,
>  #
>  # On failure, QEMU will try delete the newly created internal snapshot
> -# in the transaction.  When an I/O error occurs during deletion, the
> +# in the `transaction`.  When an I/O error occurs during deletion, the
>  # user needs to fix it later with qemu-img or other command.
>  #
> -# @actions: List of @TransactionAction; information needed for the
> +# @actions: List of `TransactionAction`; information needed for the
>  #     respective operations.
>  #
>  # @properties: structure of additional options to control the
> -#     execution of the transaction.  See @TransactionProperties for
> +#     execution of the `transaction`.  See `TransactionProperties` for
>  #     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,
> +# .. note:: The `transaction` aborts on the first failure.  Therefore,
>  #    there will be information on only one failed operation returned
>  #    in an error condition, and subsequent actions will not have been
>  #    attempted.