Anton Nefedov <[email protected]> writes: > On 8/6/2018 8:29 AM, Markus Armbruster wrote: >> Eric Blake <[email protected]> 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 :)
