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

[Anton Nefedov]

On 12/2/2018 7:38 PM, Anton Nefedov wrote:
> On 3/2/2018 6:59 PM, Markus Armbruster wrote:
> > Eric Blake <address@hidden> writes:
> >
> > > On 01/19/2018 06:50 AM, Anton Nefedov wrote:
> > > > +
> > > > +##
> > > > +# @BlockDriverStats:
> > > > +#
> > > > +# Statistics of a block driver (driver-specific)
> > > > +#
> > > > +# Since: 2.12
> > > > +##
> > > > +{ 'union': 'BlockDriverStats',
> > > > +  'data': {
> > > > +      'file': 'BlockDriverStatsFile'
> > > > +  } }
> > >
> > > Markus has been adamant that we add no new "simple unions" (unions with
> > > a 'discriminator' field) - because they are anything but simple in the
> > > long run.
> >
> > Indeed.  You could make this a flat union, similar to BlockdevOptions:
> >
> > { 'union': 'BlockDriverStats':
> >    'base': { 'driver': 'BlockdevDriver' },
> >    'discriminator': 'driver',
> >    'data': {
> >        'file': 'BlockDriverStatsFile',
> >        ... } }
> >
> > However:
> >
> > > > +
> > > > +##
> > > >   # @BlockStats:
> > > >   #
> > > >   # Statistics of a virtual block device or a block backing device.
> > > > @@ -785,6 +819,8 @@
> > > >   #
> > > >   # @stats:  A @BlockDeviceStats for the device.
> > > >   #
> > > > +# @driver-stats: Optional driver-specific statistics. (Since 2.12)
> > > > +#
> > > >   # @parent: This describes the file block device if it has one.
> > > >   #          Contains recursively the statistics of the underlying
> > > >   #          protocol (e.g. the host file for a qcow2 image). If 
> > > > there is
> > > > @@ -798,6 +834,7 @@
> > > >   { 'struct': 'BlockStats',
> > > >     'data': {'*device': 'str', '*node-name': 'str',
> > > >              'stats': 'BlockDeviceStats',
> > > > +           '*driver-stats': 'BlockDriverStats',
> >
> > 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',
> >        ... } }
> >
> > > ...[1] You are using it alongside a struct that already uses '-'
> > > (node-name), so you should use dashes.
> > >
> > > So, the difference between your proposal (a simple union) and using a
> > > "flat union", on the wire, is yours:
> > >
> > > "return": { ..., "driver-stats": { "type": "file", "data": {
> > > "discard_nb_ok: ... } } }
> > >
> > > vs. a flat union:
> > >
> > > "return": { ..., "driver-stats": { "driver": "file", "discard-nb-ok":
> > > ... } }
> > >
> > > where you can benefit from less nesting and a saner discriminator name.
> >
> > My proposal peels off yet another level of nesting.
>
> 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.

(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?

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
  - 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?

/Anton