Re: [Qemu-devel] [PATCH v6 10/17] json: reorder documentation body

[Markus Armbruster <armbru@redhat.com>]

Marc-André Lureau <marcandre.lureau@redhat.com> writes:

> Place the body of expression documentation at the top (after the
> @symbol:). This prevents ambiguity with other argument or
> tagged-section documentation.

I suspect the ambiguity is due to sub-optimal doc language design.  But
consistently putting the "what is this good for" part of the doc comment
at the top makes sense on its own.

>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  qapi-schema.json     | 83 ++++++++++++++++++++++++++--------------------------
>  qapi/block-core.json | 14 ++++-----
>  qapi/introspect.json | 28 ++++++++----------
>  qapi/trace.json      | 16 +++++-----
>  qga/qapi-schema.json | 27 +++++++++--------
>  5 files changed, 83 insertions(+), 85 deletions(-)
>
> diff --git a/qapi-schema.json b/qapi-schema.json
> index fbea3b18d9..f11b3bd178 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -1946,11 +1946,11 @@
>  #
>  # Set XBZRLE cache size
>  #
> -# @value: cache size in bytes
> -#
> -# The size will be rounded down to the nearest power of 2.
>  # The cache size can be modified before and during ongoing migration
>  #
> +# @value: cache size in bytes. The size will be rounded down to the
> +#         nearest power of 2.
> +#
>  # Returns: nothing on success
>  #
>  # Since: 1.2

More than just movement.  But I like it.

> @@ -2293,16 +2293,16 @@
>  ##
>  # @device_add:
>  #
> +# Add a device.
> +#

This line belongs here, but ...

> +# Additional arguments depend on the type.
> +#

... this one should stay where it is.

>  # @driver: the name of the new device's driver
>  #
>  # @bus: #optional the device's parent bus (device tree path)
>  #
>  # @id: #optional the device's ID, must be unique
>  #
> -# Additional arguments depend on the type.
> -#
> -# Add a device.
> -#
>  # Notes:
>  # 1. For detailed information about this command, please refer to the
>  #    'docs/qdev-device-use.txt' file.
> @@ -2319,13 +2319,13 @@
>  #                     "mac": "52:54:00:12:34:56" } }
>  # <- { "return": {} }
>  #
> +# Since: 0.13
> +##
>  # TODO This command effectively bypasses QAPI completely due to its
>  # "additional arguments" business.  It shouldn't have been added to
>  # the schema in this form.  It should be qapified properly, or
>  # replaced by a properly qapified command.
>  #
> -# Since: 0.13
> -##

## in the middle of a comment block looks weird.  If you want to keep
the TODO out of the doc comment, I suggest to put it right after the
expression.  But I'd leave it right where it is.

>  { 'command': 'device_add',
>    'data': {'driver': 'str', '*bus': 'str', '*id': 'str'},
>    'gen': false } # so we can get the additional arguments
> @@ -2499,10 +2499,10 @@
>  #
>  # Dump guest's storage keys
>  #
> -# @filename: the path to the file to dump to
> -#
>  # This command is only supported on s390 architecture.
>  #
> +# @filename: the path to the file to dump to
> +#
>  # Since: 2.5
>  ##

Meh.  Make it a note instead?

>  { 'command': 'dump-skeys',
> @@ -2513,23 +2513,24 @@
>  #
>  # Add a network backend.
>  #
> +# Additional arguments depend on the type.
> +#
>  # @type: the type of network backend.  Current valid values are 'user', 'tap',
>  #        'vde', 'socket', 'dump' and 'bridge'
>  #
>  # @id: the name of the new network backend
>  #
> -# Additional arguments depend on the type.
> +# Since: 0.14.0
> +#
> +# Returns: Nothing on success
> +#          If @type is not a valid network backend, DeviceNotFound
> +##
>  #
>  # TODO This command effectively bypasses QAPI completely due to its
>  # "additional arguments" business.  It shouldn't have been added to
>  # the schema in this form.  It should be qapified properly, or
>  # replaced by a properly qapified command.
>  #
> -# Since: 0.14.0
> -#
> -# Returns: Nothing on success
> -#          If @type is not a valid network backend, DeviceNotFound
> -##
>  { 'command': 'netdev_add',
>    'data': {'type': 'str', 'id': 'str'},
>    'gen': false }                # so we can get the additional arguments

Comments on device_add apply.

> @@ -3209,6 +3210,22 @@
>  #
>  # Virtual CPU definition.
>  #
> +# @unavailable-features is a list of QOM property names that
> +# represent CPU model attributes that prevent the CPU from running.
> +# If the QOM property is read-only, that means there's no known
> +# way to make the CPU model run in the current host. Implementations
> +# that choose not to provide specific information return the
> +# property name "type".
> +# If the property is read-write, it means that it MAY be possible
> +# to run the CPU model in the current host if that property is
> +# changed. Management software can use it as hints to suggest or
> +# choose an alternative for the user, or just to generate meaningful
> +# error messages explaining why the CPU model can't be used.
> +# If @unavailable-features is an empty list, the CPU model is
> +# runnable using the current host and machine-type.
> +# If @unavailable-features is not present, runnability
> +# information for the CPU is not available.
> +#
>  # @name: the name of the CPU definition
>  #
>  # @migration-safe: #optional whether a CPU definition can be safely used for
> @@ -3227,22 +3244,6 @@
>  #                        the CPU model from running in the current
>  #                        host. (since 2.8)
>  #
> -# @unavailable-features is a list of QOM property names that
> -# represent CPU model attributes that prevent the CPU from running.
> -# If the QOM property is read-only, that means there's no known
> -# way to make the CPU model run in the current host. Implementations
> -# that choose not to provide specific information return the
> -# property name "type".
> -# If the property is read-write, it means that it MAY be possible
> -# to run the CPU model in the current host if that property is
> -# changed. Management software can use it as hints to suggest or
> -# choose an alternative for the user, or just to generate meaningful
> -# error messages explaining why the CPU model can't be used.
> -# If @unavailable-features is an empty list, the CPU model is
> -# runnable using the current host and machine-type.
> -# If @unavailable-features is not present, runnability
> -# information for the CPU is not available.
> -#

This reorders argument sections.  I doubt this is intentional.

>  # Since: 1.2.0
>  ##
>  { 'struct': 'CpuDefinitionInfo',
> @@ -3381,10 +3382,6 @@
>  #
>  # The result of a CPU model comparison.
>  #
> -# @result: The result of the compare operation.
> -# @responsible-properties: List of properties that led to the comparison result
> -#                          not being identical.
> -#
>  # @responsible-properties is a list of QOM property names that led to
>  # both CPUs not being detected as identical. For identical models, this
>  # list is empty.
> @@ -3392,6 +3389,10 @@
>  # CPU models identical. If the special property name "type" is included, the
>  # models are by definition not identical and cannot be made identical.
>  #
> +# @result: The result of the compare operation.
> +# @responsible-properties: List of properties that led to the comparison result
> +#                          not being identical.
> +#
>  # Since: 2.8.0
>  ##
>  { 'struct': 'CpuModelCompareInfo',

Less readable than before, I'm afraid: the long explanation of
@responsible-properties now comes before the short one (args section).

You could try merging the two.

> @@ -3617,6 +3618,10 @@
>  ##
>  # @QKeyCode:
>  #
> +# An enumeration of key name.
> +#
> +# This is used by the send-key command.
> +#
>  # @unmapped: since 2.0
>  # @pause: since 2.0
>  # @ro: since 2.4
> @@ -3624,10 +3629,6 @@
>  # @kp_equals: since 2.6
>  # @power: since 2.6
>  #
> -# An enumeration of key name.
> -#
> -# This is used by the send-key command.
> -#
>  # Since: 1.3.0
>  #
>  ##

Okay.

> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 05cedc3f23..e1ab80e419 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -1977,10 +1977,9 @@
>  # @user:                #optional user as which to connect, defaults to current
>  #                       local user name
>  #
> -# TODO: Expose the host_key_check option in QMP
> -#
>  # Since: 2.8
>  ##
> +# TODO: Expose the host_key_check option in QMP

Comment on device_add's TODO applies.

>  { 'struct': 'BlockdevOptionsSsh',
>    'data': { 'server': 'InetSocketAddress',
>              'path': 'str',
> @@ -2164,8 +2163,6 @@
>  #
>  # Details for connecting to a gluster server
>  #
> -# @type:       Transport type used for gluster connection
> -#
>  # This is similar to SocketAddress, only distinction:
>  #
>  # 1. GlusterServer is a flat union, SocketAddress is a simple union.
> @@ -2178,6 +2175,8 @@
>  # GlusterServer is actually not Gluster-specific, its a
>  # compatibility evolved into an alternate for SocketAddress.
>  #
> +# @type:       Transport type used for gluster connection
> +#
>  # Since: 2.7
>  ##
>  { 'union': 'GlusterServer',

Okay.

> @@ -2356,8 +2355,9 @@
>  ##
>  # @BlockdevOptions:
>  #
> -# Options for creating a block device.  Many options are available for all
> -# block devices, independent of the block driver:
> +# Options for creating a block device.  Many options are available for
> +# all block devices, independent of the block driver, remaining
> +# options are determined by the block driver.
>  #
>  # @driver:        block driver name
>  # @node-name:     #optional the node name of the new node (Since 2.0).
> @@ -2369,8 +2369,6 @@
>  # @detect-zeroes: #optional detect and optimize zero writes (Since 2.1)
>  #                 (default: off)
>  #
> -# Remaining options are determined by the block driver.
> -#
>  # Since: 1.7
>  ##

Less readable than before, I'm afraid: we now talk about common members,
then variant members, then common members again.

I expect this to change when we document unions properly.

>  { 'union': 'BlockdevOptions',
> diff --git a/qapi/introspect.json b/qapi/introspect.json
> index fd4dc84196..b1661c5b7c 100644
> --- a/qapi/introspect.json
> +++ b/qapi/introspect.json
> @@ -77,19 +77,18 @@
>  ##
>  # @SchemaInfo:
>  #
> +# Additional members depend on the value of @meta-type.
> +#
>  # @name: the entity's name, inherited from @base.
>  #        Commands and events have the name defined in the QAPI schema.
>  #        Unlike command and event names, type names are not part of
>  #        the wire ABI.  Consequently, type names are meaningless
>  #        strings here, although they are still guaranteed unique
> -#        regardless of @meta-type.
> -#
> -# All references to other SchemaInfo are by name.
> +#        regardless of @meta-type. All references to other SchemaInfo
> +#        are by name.

I don't like this.  I'd be okay with something like

# @name: the entity's name, inherited from @base.
#        The SchemaInfo is always referenced by this name.
#        Commands and events have the name defined in the QAPI schema.
#        Unlike command and event names, type names are not part of
#        the wire ABI.  Consequently, type names are meaningless
#        strings here, although they are still guaranteed unique
#        regardless of @meta-type.

>  #
>  # @meta-type: the entity's meta type, inherited from @base.
>  #
> -# Additional members depend on the value of @meta-type.
> -#

Comment on BlockdevOptions applies.

>  # Since: 2.5
>  ##
>  { 'union': 'SchemaInfo',
> @@ -134,10 +133,10 @@
>  #
>  # Additional SchemaInfo members for meta-type 'enum'.
>  #
> -# @values: the enumeration type's values, in no particular order.
> -#
>  # Values of this type are JSON string on the wire.
>  #
> +# @values: the enumeration type's values, in no particular order.
> +#
>  # Since: 2.5
>  ##
>  { 'struct': 'SchemaInfoEnum',

Less readable than before, I'm afraid: talks about additional members,
then the JSON type, then about members again.

> @@ -148,10 +147,10 @@
>  #
>  # Additional SchemaInfo members for meta-type 'array'.
>  #
> -# @element-type: the array type's element type.
> -#
>  # Values of this type are JSON array on the wire.
>  #
> +# @element-type: the array type's element type.
> +#
>  # Since: 2.5
>  ##
>  { 'struct': 'SchemaInfoArray',

Likewise.

> @@ -162,6 +161,8 @@
>  #
>  # Additional SchemaInfo members for meta-type 'object'.
>  #
> +# Values of this type are JSON object on the wire.
> +#
>  # @members: the object type's (non-variant) members, in no particular order.
>  #
>  # @tag: #optional the name of the member serving as type tag.
> @@ -173,8 +174,6 @@
>  #            and may even differ from the order of the values of the
>  #            enum type of the @tag.
>  #
> -# Values of this type are JSON object on the wire.
> -#
>  # Since: 2.5
>  ##
>  { 'struct': 'SchemaInfoObject',

Likewise.

> @@ -225,12 +224,12 @@
>  #
>  # Additional SchemaInfo members for meta-type 'alternate'.
>  #
> +# On the wire, this can be any of the members.
> +#
>  # @members: the alternate type's members, in no particular order.
>  #           The members' wire encoding is distinct, see
>  #           docs/qapi-code-gen.txt section Alternate types.
>  #
> -# On the wire, this can be any of the members.
> -#
>  # Since: 2.5
>  ##
>  { 'struct': 'SchemaInfoAlternate',

Likewise.

> @@ -258,10 +257,9 @@
>  #
>  # @ret-type: the name of the command's result type.
>  #
> -# TODO @success-response (currently irrelevant, because it's QGA, not QMP)
> -#
>  # Since: 2.5
>  ##
> +# TODO @success-response (currently irrelevant, because it's QGA, not QMP)
>  { 'struct': 'SchemaInfoCommand',
>    'data': { 'arg-type': 'str', 'ret-type': 'str' } }
>  

Comment on device_add's TODO applies.

> diff --git a/qapi/trace.json b/qapi/trace.json
> index 3ad7df7fdb..c52352cfb6 100644
> --- a/qapi/trace.json
> +++ b/qapi/trace.json
> @@ -30,13 +30,13 @@
>  #
>  # Information of a tracing event.
>  #
> +# An event is per-vCPU if it has the "vcpu" property in the "trace-events"
> +# files.
> +#
>  # @name: Event name.
>  # @state: Tracing state.
>  # @vcpu: Whether this is a per-vCPU event (since 2.7).
>  #
> -# An event is per-vCPU if it has the "vcpu" property in the "trace-events"
> -# files.
> -#
>  # Since: 2.2
>  ##
>  { 'struct': 'TraceEventInfo',

Less readable than before, I'm afraid: it defines terms (the parameters)
only after it uses them.

> @@ -72,11 +72,6 @@
>  #
>  # Set the dynamic tracing state of events.
>  #
> -# @name: Event name pattern (case-sensitive glob).
> -# @enable: Whether to enable tracing.
> -# @ignore-unavailable: #optional Do not match unavailable events with @name.
> -# @vcpu: #optional The vCPU to act upon (all by default; since 2.7).
> -#
>  # An event's state is modified if:
>  # - its name matches the @name pattern, and
>  # - if @vcpu is given, the event has the "vcpu" property.
> @@ -86,6 +81,11 @@
>  # match, @vcpu is given and the event does not have the "vcpu" property, an
>  # error is returned.
>  #
> +# @name: Event name pattern (case-sensitive glob).
> +# @enable: Whether to enable tracing.
> +# @ignore-unavailable: #optional Do not match unavailable events with @name.
> +# @vcpu: #optional The vCPU to act upon (all by default; since 2.7).
> +#
>  # Since: 2.2
>  ##
>  { 'command': 'trace-event-set-state',

Likewise.

> diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
> index ad63737fce..8c881dd5d5 100644
> --- a/qga/qapi-schema.json
> +++ b/qga/qapi-schema.json
> @@ -186,13 +186,13 @@
>  # Initiate guest-activated shutdown. Note: this is an asynchronous
>  # shutdown request, with no guarantee of successful shutdown.
>  #
> -# @mode: #optional "halt", "powerdown" (default), or "reboot"
> -#
>  # 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".
>  #
> +# @mode: #optional "halt", "powerdown" (default), or "reboot"
> +#
>  # Since: 0.15.0
>  ##
>  { 'command': 'guest-shutdown', 'data': { '*mode': 'str' },

Meh.  Make it a note instead?

> @@ -815,20 +815,21 @@
>  #
>  # @username: the user account whose password to change
>  # @password: the new password entry string, base64 encoded
> -# @crypted: true if password is already crypt()d, false if raw
>  #
> -# If the @crypted flag is true, it is the caller's responsibility
> -# to ensure the correct crypt() encryption scheme is used. This
> -# command does not attempt to interpret or report on the encryption
> -# scheme. Refer to the documentation of the guest operating system
> -# in question to determine what is supported.
> +#            The @password parameter must always be base64 encoded before
> +#            transmission, even if already crypt()d, to ensure it is 8-bit
> +#            safe when passed as JSON.
> +#
> +# @crypted: true if password is already crypt()d, false if raw
>  #
> -# Not all guest operating systems will support use of the
> -# @crypted flag, as they may require the clear-text password
> +#           If the @crypted flag is true, it is the caller's responsibility
> +#           to ensure the correct crypt() encryption scheme is used. This
> +#           command does not attempt to interpret or report on the encryption
> +#           scheme. Refer to the documentation of the guest operating system
> +#           in question to determine what is supported.
>  #
> -# The @password parameter must always be base64 encoded before
> -# transmission, even if already crypt()d, to ensure it is 8-bit
> -# safe when passed as JSON.
> +#           Not all guest operating systems will support use of the
> +#           @crypted flag, as they may require the clear-text password
>  #
>  # Returns: Nothing on success.
>  #

Okay.

Having reviewed this, I really, really doubt permitting untagged,
non-argument sections only at the top make sense.  It's too much of a
straightjacket.

Can you explain the ambiguity probem you mentioned in the commit message
to me once more?