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).
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
