Re: [Qemu-devel] [PATCH v2 06/49] qapi: change Netdev into a flat union

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 4930 bytes --]

On 08/21/2015 09:37 AM, Kővágó, Zoltán wrote:
> Except qapi-schema.json, this patch was generated by:
> 
> find . -name .git -prune -o -type f \! -name '*~' -print0 | \
>   xargs -0 sed -i \
>     -e 's/NetClientOptionsKind/NetClientDriver/g' \
>     -e 's/NET_CLIENT_OPTIONS_KIND_/NET_CLIENT_DRIVER_/g' \

Same question as in 3 about whether 'driver' is the right name for the
new enum.

>     -e 's/netdev->opts/netdev/g'
> 
> Signed-off-by: Kővágó, Zoltán <DirtY.iCE.hu@gmail.com>
> ---

>  qapi-schema.json                 | 36 +++++++++++------
>  45 files changed, 166 insertions(+), 154 deletions(-)

So the bulk is mechanical 1:1 replacement, and the qapi change adds a
net of 12 lines.

> 
> diff --git a/hw/arm/musicpal.c b/hw/arm/musicpal.c
> index 42f66b3..94bdf5f 100644
> --- a/hw/arm/musicpal.c
> +++ b/hw/arm/musicpal.c
> @@ -374,7 +374,7 @@ static void eth_cleanup(NetClientState *nc)
>  }
>  
>  static NetClientInfo net_mv88w8618_info = {
> -    .type = NET_CLIENT_OPTIONS_KIND_NIC,
> +    .type = NET_CLIENT_DRIVER_NIC,

You know, if we could teach qapi to allow a user-named type with 'Kind'
as the suffix, we wouldn't have quite as much churn :)


> +++ b/qapi-schema.json
> @@ -2474,16 +2474,31 @@
>      '*vhostforce':    'bool' } }
>  
>  ##
> -# @NetClientOptions
> +# @NetClientDriver
>  #
> -# A discriminated record of network device traits.
> +# Available netdev drivers.
> +#
> +# Since 2.5
> +##
> +{ 'enum': 'NetClientDriver',
> +  'data': [ 'none', 'nic', 'user', 'tap', 'l2tpv3', 'socket', 'vde', 'dump',
> +            'bridge', 'hubport', 'netmap', 'vhost-user' ] }

Hmm; maybe 'Driver' really is the right name for this enum.

> +
> +##
> +# @Netdev
> +#
> +# Captures the configuration of a network device.
>  #
>  # Since 1.2
>  #
>  # 'l2tpv3' - since 2.1
> +# @id #optional - since 2.5
> +# @vlan, @name - since 2.5

I'd document id and vlan prior to the initial 'Since 1.2', then leave
the only thing after that line to be just the list of when later drivers
were added to the union.  Actually, depending on what I decide about
4/49, we may not even be adding vlan and name, and id might not be
optional, since that was just NetdevLegacy.  And if we do merge
NetdevLegacy, then it's better to have separate lines for @vlan and
@name, as well as the documentation of when they can be set.

>  #
>  ##
> -{ 'union': 'NetClientOptions',
> +{ 'union': 'Netdev',
> +  'base': 'NetdevBase',
> +  'discriminator': 'type',
>    'data': {
>      'none':     'NetdevNoneOptions',
>      'nic':      'NetLegacyNicOptions',
> @@ -2499,9 +2514,9 @@
>      'vhost-user': 'NetdevVhostUserOptions' } }

QMP wise (if we were to use Netdev in QMP), this is changing:

{ "command":"foo", "arguments": {
    "id":"abc", "opts" { "type":"dump", "data": {
        "len":1024, "file":"/path/to/foo" } } } }

into

{ "command":"foo", "arguments": {
    "id":"abc", "type":"dump",
    "len":1024, "file":"/path/to/foo" } }

Wow - removing two levels of nesting in one patch.  Even cooler, this
LOOKS a lot like what QMP 'netdev_add' already parses :)

The corresponding C code loses one layer of boxing (because, as I
already pointed out on 3, converting from simple to flat union without
creating data wrappers loses one layer of nesting in QMP with no change
to the nesting in the matching generated C structure).

So, if we can _avoid_ merging in NetdevLegacy stuff here (but that has
knock-on effects, because my hack to use 5/49 without 4/49 doesn't quite
work with the sed statement you used here), then you have just created
the solution for netdev_add.  I'll use your patch as a starting point,
coupled with my pending work to allow a boxed flat union as the direct
argument to a QMP command.

>  
>  ##
> -# @Netdev
> +# @NetdevBase
>  #
> -# Captures the configuration of a network device.
> +# Captures the common configuration of a network device.
>  #
>  # @vlan: #optional vlan number (legacy, forbidden with -netdev)
>  #
> @@ -2510,19 +2525,16 @@
>  # @name: #optional identifier for monitor commands, ignored if @id is present
>  #        (legacy, forbidden with -netdev)
>  #
> -# @opts: device type specific properties (legacy)
> +# @type: the netdev driver to use
>  #
> -# Since 1.2
> -#
> -# @id #optional - since 2.5
> -# @vlan, @name - since 2.5
> +# Since 2.5
>  ##
> -{ 'struct': 'Netdev',
> +{ 'struct': 'NetdevBase',
>    'data': {
>      '*vlan': 'int32',
>      '*id':   'str',
>      '*name': 'str',
> -    'opts':  'NetClientOptions' } }
> +    'type':  'NetClientDriver' } }

This is where I'm not completely sold that the merge from NetdevLegacy
makes sense. Like I said, I'm playing with it some on my tree.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]