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

[Markus Armbruster]

Anton Nefedov <address@hidden> writes:

> On 8/6/2018 8:29 AM, Markus Armbruster wrote:
>> 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.
>>
>
> 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?

Works for me, but block maintainers have veto powers :)