[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-block] [Qemu-devel] [PATCH v2 8/8] qapi: query-blockstat: add

From: Eric Blake
Subject: Re: [Qemu-block] [Qemu-devel] [PATCH v2 8/8] qapi: query-blockstat: add driver specific file-posix stats
Date: Thu, 15 Feb 2018 08:32:19 -0600
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.6.0

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

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:


{ '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

reply via email to

[Prev in Thread] Current Thread [Next in Thread]