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

[Anton Nefedov]

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?