Eric Blake <ebl...@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.
