Re: [Qemu-devel] [PATCH v2 8/8] qapi: query-blockstat: add driver specific file-posix stats

[Eric Blake]

On 01/19/2018 06:50 AM, Anton Nefedov wrote:
> A block driver can provide a callback to report driver-specific
> statistics.
> 
> file-posix driver now reports discard statistics
> 
> Signed-off-by: Anton Nefedov <address@hidden>
> Reviewed-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> ---

> +++ b/qapi/block-core.json
> @@ -774,6 +774,40 @@
>             'timed_stats': ['BlockDeviceTimedStats'] } }
>  
>  ##
> +# @BlockDriverStatsFile:
> +#
> +# File driver statistics
> +#
> +# @discard_nb_ok: The number of succeeded discard operations performed by
> +#                 the driver.
> +#
> +# @discard_nb_failed: The number of failed discard operations performed by
> +#                     the driver.
> +#
> +# @discard_bytes_ok: The number of bytes discarded by the driver.
> +#
> +# Since 2.12
> +##
> +{ 'struct': 'BlockDriverStatsFile',
> +  'data': {
> +      'discard_nb_ok': 'int',
> +      'discard_nb_failed': 'int',
> +      'discard_bytes_ok': 'int'
> +  } }

New interfaces should prefer '-' over '_', where possible (a reason for
using '_' is if the fields are alongside pre-existing fields that
already used '_').  Let's see how this gets used...[1]

> +
> +##
> +# @BlockDriverStats:
> +#
> +# Statistics of a block driver (driver-specific)
> +#
> +# Since: 2.12
> +##
> +{ 'union': 'BlockDriverStats',
> +  'data': {
> +      'file': 'BlockDriverStatsFile'
> +  } }

Markus has been adamant that we add no new "simple unions" (unions with
a 'discriminator' field) - because they are anything but simple in the
long run.

> +
> +##
>  # @BlockStats:
>  #
>  # Statistics of a virtual block device or a block backing device.
> @@ -785,6 +819,8 @@
>  #
>  # @stats:  A @BlockDeviceStats for the device.
>  #
> +# @driver-stats: Optional driver-specific statistics. (Since 2.12)
> +#
>  # @parent: This describes the file block device if it has one.
>  #          Contains recursively the statistics of the underlying
>  #          protocol (e.g. the host file for a qcow2 image). If there is
> @@ -798,6 +834,7 @@
>  { 'struct': 'BlockStats',
>    'data': {'*device': 'str', '*node-name': 'str',
>             'stats': 'BlockDeviceStats',
> +           '*driver-stats': 'BlockDriverStats',

...[1] You are using it alongside a struct that already uses '-'
(node-name), so you should use dashes.

So, the difference between your proposal (a simple union) and using a
"flat union", on the wire, is yours:

"return": { ..., "driver-stats": { "type": "file", "data": {
"discard_nb_ok: ... } } }

vs. a flat union:

"return": { ..., "driver-stats": { "driver": "file", "discard-nb-ok":
... } }

where you can benefit from less nesting and a saner discriminator name.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

signature.asc
Description: OpenPGP digital signature