Re: [Qemu-devel] [PATCH 2/3] Unified Datagram Socket Transport - GRE support

[Eric Blake <eblake@redhat.com>]

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

On 07/18/2017 12:08 PM, anton.ivanov@cambridgegreys.com wrote:
> From: Anton Ivanov <anton.ivanov@cambridgegreys.com>
> 
> This adds GRETAP support to the unified socket driver.
> 
> Signed-off-by: Anton Ivanov <anton.ivanov@cambridgegreys.com>
> ---
>  net/Makefile.objs |   2 +-
>  net/clients.h     |   4 +
>  net/gre.c         | 313 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  net/net.c         |   5 +
>  qapi-schema.json  |  46 +++++++-
>  qemu-options.hx   |  63 ++++++++++-
>  6 files changed, 425 insertions(+), 8 deletions(-)
>  create mode 100644 net/gre.c
> 

Just an interface review:

> +++ b/qapi-schema.json
> @@ -3847,7 +3847,41 @@
>      'txsession':    'uint32',
>      '*rxsession':   'uint32',
>      '*offset':      'uint32' } }
> -
> +##
> +# @NetdevGREOptions:
> +#
> +# Connect the VLAN to Ethernet over Ethernet over GRE (GRETAP) tunnel
> +#
> +# @src: source address
> +#
> +# @dst: destination address
> +#
> +# @ipv6: force the use of ipv6

This doesn't quite match what we do with other sockets (where we have
both ipv4 and ipv6 booleans to allow IPv4-only, IPv6-only, or both).  Is
this something where we can reuse InetSocketAddress instead of inventing
yet another way of doing things?

Then again, it does match what NetdevL2TPv3Options did :(

> +#
> +# @sequence: have sequence counter
> +#
> +# @pinsequence: pin sequence counter to zero -
> +#              workaround for buggy implementations or
> +#              networks with packet reorder
> +#
> +# @txkey: 32 bit transmit key
> +#
> +# @rxkey: 32 bit receive key

Worth listing what the defaults are for these optional fields when not
present?

> +#
> +# Note - gre checksums are not supported at present
> +#
> +#
> +# Since 2.9

You've missed 2.9 by a long shot.  You've also missed 2.10 softfreeze
for a new feature, so this should read since 2.11.

> @@ -3966,7 +4000,7 @@
>  ##
>  { 'enum': 'NetClientDriver',
>    'data': [ 'none', 'nic', 'user', 'tap', 'l2tpv3', 'socket', 'vde', 'dump',
> -            'bridge', 'hubport', 'netmap', 'vhost-user' ] }
> +            'bridge', 'hubport', 'netmap', 'vhost-user', 'gre' ] }

Worth adding a comment that 'gre' is since 2.11 (look for other enums
that have had additions after the initial release of the enum, for an
example of the preferred format).

>  
>  ##
>  # @Netdev:
> @@ -3996,7 +4030,8 @@
>      'bridge':   'NetdevBridgeOptions',
>      'hubport':  'NetdevHubPortOptions',
>      'netmap':   'NetdevNetmapOptions',
> -    'vhost-user': 'NetdevVhostUserOptions' } }
> +    'vhost-user': 'NetdevVhostUserOptions',
> +    'gre':      'NetdevGREOptions' } }

Okay.

>  
>  ##
>  # @NetLegacy:
> @@ -4027,7 +4062,7 @@
>  ##
>  { 'enum': 'NetLegacyOptionsType',
>    'data': ['none', 'nic', 'user', 'tap', 'l2tpv3', 'socket', 'vde',
> -           'dump', 'bridge', 'netmap', 'vhost-user'] }
> +           'dump', 'bridge', 'netmap', 'vhost-user', 'gre'] }

Not okay.  NetLegacy should never grow again (that's the whole point of
it being legacy - we're trying to phase it out).

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


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