On 07/18/2017 12:08 PM, anton.iva...@cambridgegreys.com wrote: > From: Anton Ivanov <anton.iva...@cambridgegreys.com> > > This adds GRETAP support to the unified socket driver. > > Signed-off-by: Anton Ivanov <anton.iva...@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
signature.asc
Description: OpenPGP digital signature
