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

[Eric Blake]

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