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

[Anton Nefedov <anton.nefedov@virtuozzo.com>]

On 3/2/2018 6:59 PM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
> 
>> On 01/19/2018 06:50 AM, Anton Nefedov wrote:
>>> +
>>> +##
>>> +# @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.
> 

The output is better indeed, thanks; a little drawback is now we need to
pass the whole BlockStats to the driver so it fills its stats.

e.g. the interface:

     void (*bdrv_get_stats)(BlockDriverState *bs, BlockStats *stats);

And that BlockdevDriver subset type (or a generator patch) still seems
to be needed

   { 'enum' : 'BlockdevDriverWithStats',
     'data' : [ 'file' ] }