qemu-devel
[Top][All Lists]
Advanced

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

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


From: Anton Ivanov
Subject: Re: [Qemu-devel] [PATCH 2/3] Unified Datagram Socket Transport - GRE support
Date: Wed, 19 Jul 2017 15:46:05 +0100
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0



On 19/07/17 15:40, Eric Blake wrote:
On 07/18/2017 12:08 PM, address@hidden wrote:
From: Anton Ivanov <address@hidden>

This adds GRETAP support to the unified socket driver.

Signed-off-by: Anton Ivanov <address@hidden>
---
  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?

I can try that in the next version.


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?

GRE header is incremental.

If there is no tx/rxkey the header is shorter and these are not present in the header.

If a key is specified the header is longer to accommodate the key.

They are optional - similar to the l2tpv3 cookie.


+#
+# 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.

The patch has been sitting in my outgoing queue for reasons outside my control for months. I apologise for that and I am re-aligning all of them for 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).

Ack.


##
  # @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).

OK, I will revise the patch before the next submission.

A.


--
Anton R. Ivanov

Cambridge Greys Limited, England and Wales company No 10273661
http://www.cambridgegreys.com/




reply via email to

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