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

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

On 8/6/2018 8:29 AM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> 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.
> 

following it further we will have BlockStatsDriverSpecificFile which is
even longer.

Personally, both
'*driver-specific': 'BlockDriverStats'; 'BlockDriverStatsFile'
'*driver-specific': 'BlockStatsSpecific'; 'BlockStatsSpecificFile'
look right to me.

Function signatures look ok too except that BlockDriverStats is easy to
confuse with BlockDriverState

BlockDriverStats *bdrv_get_stats(BlockDriverState *bs);
BlockStatsSpecific *bdrv_get_specific_stats(BlockDriverState *bs);

so maybe BlockStatsSpecific indeed?