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

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> 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.

Indeed.  You could make this a flat union, similar to BlockdevOptions:

{ 'union': 'BlockDriverStats':
  'base': { 'driver': 'BlockdevDriver' },
  'discriminator': 'driver',
  'data': {
      'file': 'BlockDriverStatsFile',
      ... } }

However: 

>> +
>> +##
>>  # @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',

You're adding a union of driver-specific stats to a struct of generic
stats.  That's unnecessarily complicated.  Instead, turn the struct of
generic stats into a flat union, like this:

{ 'union': 'BlockStats',
  'base': { ... the generic stats, i.e. the members of BlockStats
            before this patch ...
            'driver': 'BlockdevDriver' }
  'discriminator': 'driver',
  'data': {
      'file': 'BlockDriverStatsFile',
      ... } }

> ...[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.

My proposal peels off yet another level of nesting.