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

[Eric Blake]

On 02/15/2018 04:23 AM, Anton Nefedov wrote:
> > The output is better indeed, thanks; a little drawback is now we need to
> > pass the whole BlockStats to the driver so it fills its stats.
> >
> > e.g. the interface:
> >
> >      void (*bdrv_get_stats)(BlockDriverState *bs, BlockStats *stats);
> >
> > And that BlockdevDriver subset type (or a generator patch) still seems
> > to be needed
> >
> >    { 'enum' : 'BlockdevDriverWithStats',
> >      'data' : [ 'file' ] }
>
> Hmm, actually it seems the driver-specific-stats should either be
> optional, or we need to handle the rest of the drivers in the generated
> code.

No, we don't want a new enum.  Rather, use the existing enum, and use an 
empty type for all the branches of the union that have no additional 
stats to add.

> (i.e. with the dummy enum as above, what should the mandatory 'driver'
> field be when there's e.g. nbd driver? It will default to 0, and that is
> BLOCKDEV_DRIVER_WITH_STATS_FILE, so it will be interpreted as 'file' in
> qmp output)
>
>
> If we patch the generator, I guess we could add smth like a new tag to
> the union that has no data for some discriminators?

Right now, the way to write a union where only some branches add fields is:

{ 'struct': 'SomeEmptyType', 'data': {} }
{ 'union': 'MyUnion', 'base':'Base', 'discriminator':'foo',
  'data': { 'file': 'ExtraFields',
            'nbd': 'SomeEmptyType',
            ... } }

I also have a patch that I can revive (posted months ago) that would allow:

  'data': { 'file': 'ExtraFields',
            'nbd': {},
            ... } }

so that we don't have to explicitly create a pointless empty type for 
the branches that don't care.

Meanwhile, here's an example post that uses the explicit empty type idiom:
https://lists.gnu.org/archive/html/qemu-devel/2018-02/msg03991.html

> e.g.
>
> { 'union': 'BlockStats',
>    'base': {'*device': 'str', '*node-name': 'str',
>             'stats': 'BlockDeviceStats',
>             '*parent': 'BlockStats',
>             '*backing': 'BlockStats',
>             'driver': 'BlockdevDriver'},
>    'discriminator': 'driver',
>    'data-optional': {                     <---- instead of 'data'
>        'file': 'BlockDriverStatsFile',
>    } }
>
> Then the generator would need to:
>    - pick either 'data-optional' or 'data' members

I'd rather keep it 'data': {} for the list of branches that have 
additional members, and add a new boolean, maybe 'partial-data':true, as 
the witness of whether the generator requires coverage for all enum 
values, or will implicitly allow the uncovered enum values as though 
they had an empty type.

>    - skip 'data missing branch' check for such unions
>    - do not abort() when visiting the union and discriminator pointing
>      to no data type
>
> I can try and implement this if there's no other suggestion?

Or I could fold that into my patch from months ago.  Let's see what I 
can come up with this week.

--
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org