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

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> On 06/07/2018 10:23 AM, Anton Nefedov wrote:
>>>> If we introduce BlockdevDriver as a discriminator as Markus suggests
>>>> above, we need some way to define its value.
>>>> I guess one would be to check blk->bs->drv->format_name but it won't
>>>> always work; often it's even blk->bs == NULL.
>>>
>>> There is no blk->bs, at least not if blk is a BlockBackend *.
>>>
>>> I figure the problem you're trying to describe is query-blockstats
>>> running into BlockBackends that aren't associated with a
>>> BlockDriverState (blk->root is null), and thus aren't associated with a
>>> BlockDriver.  Correct?
>>>
>>
>> Sorry, yes, exactly
>
> Okay, that sounds like the driver stats have to be optional, present
> only when blk->bs is non-NULL.
>
>>
>>>> I guess we could add a default ('unspecified'?) to BlockdevDriver enum?
>>>
>>> This part I understand, but...
>>>
>>>> But I'd rather leave an optional BlockDriverStats union (but make it
>>>> flat). Only the drivers that provide these stats will need to set
>>>> BlockdevDriver field. What do you think?
>>>
>>> I'm not sure I got this part.  Care to sketch the QAPI schema snippet?
>>>
>>
>> You earlier proposed:
>>
>>  >>> 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',
>>  >>>         ... } }
>
> That would require 'driver' to always resolve to something, even when
> there is no driver (unless we create a superset enum that adds 'none'
> beyond what 'BlockdevDriver' supports).
>
>>
>> But I meant to leave it as:
>>
>> + { 'union': 'BlockDriverStats':
>> +     'base': { 'driver': 'BlockdevDriver' },
>> +     'discriminator': 'driver',
>> +     'data': {
>> +         'file': 'BlockDriverStatsFile' } }
>>
>>
>>    { 'struct': 'BlockStats',
>>      'data': {'*device': 'str', '*node-name': 'str',
>>               'stats': 'BlockDeviceStats',
>> +            '*driver-stats': 'BlockDriverStats',
>>               '*parent': 'BlockStats',
>>               '*backing': 'BlockStats'} }
>>
>> so those block backends which do not provide driver stats do not need to
>> set BlockdevDriver field.
>
> This makes the most sense to me - we're stuck with a layer of nesting,
> but that's because driver-stats truly is optional (we don't always
> have access to a driver).

Agree.

Now we have to name the thing.  You propose @driver-stats.  Servicable,
but let's review how existing driver-specific things are named.

BlockdevOptions and BlockdevCreateOptions have them inline, thus no
name.

ImageInfo has '*format-specific': 'ImageInfoSpecific'.

If we follow ImageInfo precedence, we get '*driver-specific':
'BlockStatsSpecific'.  driver-specific I like well enough.
BlockStatsSpecific less so.  BlockStatsDriverSpecific feels better, but
perhaps a bit long.