From fbf2208e800a535a452244c5c437ca6dc8f85874 Mon Sep 17 00:00:00 2001
From: Matthias Radestock <matthias.radestock@gmail.com>
Date: Wed, 10 Jun 2015 07:41:37 +0100
Subject: [PATCH] simplify fragmentation test

This is how fragmentation tests used to happen:

    A: conn.stackFrag = false
    A->B: StartFragmentationTest
    B->A: send large non-DF UDP packet
    A->B: FragmentationReceived
    B: conn.stackFrag = true

conn.stackFrag = true means "UDP packets sent to the other peer get
correctly fragmented and re-assembled".

Note how A clears its own stackFrag flag, but the test establishes
whether fragmentation works for B->A, and ends up setting stackFrag on
B. Not only is this weird, it also means that if A previously had
stackFrag=true, it will now start fragmenting packets until B
initiates a fragmentation test.

The new sequence is:

    A: conn.stackFrag = false
    A->B: send large non-DF UDP packet
    B->A: FragmentationReceived
    A: conn.stackFrag = true

Closes #746.
---
 router/connection.go | 23 +++++++++++------------
 router/protocol.go   |  3 +--
 2 files changed, 12 insertions(+), 14 deletions(-)

diff --git a/router/connection.go b/router/connection.go
index 980de454..9b597d5c 100644
--- a/router/connection.go
+++ b/router/connection.go
@@ -262,10 +262,7 @@ func (conn *LocalConnection) SetEstablished() {
 		conn.fragTest = time.NewTicker(FragTestInterval)
 		// avoid initial waits for timers to fire
 		conn.Forward(true, conn.heartbeatFrame, nil)
-		conn.setStackFrag(false)
-		if err := conn.sendSimpleProtocolMsg(ProtocolStartFragmentationTest); err != nil {
-			return err
-		}
+		conn.performFragTest()
 		return nil
 	})
 }
@@ -386,8 +383,7 @@ func (conn *LocalConnection) actorLoop(actionChan <-chan ConnectionAction) (err
 		case <-tickerChan(conn.heartbeat):
 			conn.Forward(true, conn.heartbeatFrame, nil)
 		case <-tickerChan(conn.fragTest):
-			conn.setStackFrag(false)
-			err = conn.sendSimpleProtocolMsg(ProtocolStartFragmentationTest)
+			conn.performFragTest()
 		}
 	}
 	return
@@ -473,12 +469,6 @@ func (conn *LocalConnection) handleProtocolMsg(tag ProtocolTag, payload []byte)
 		// We can now consider the connection as established from our
 		// end.
 		conn.SetEstablished()
-	case ProtocolStartFragmentationTest:
-		conn.Forward(false, &ForwardedFrame{
-			srcPeer: conn.local,
-			dstPeer: conn.remote,
-			frame:   FragTest},
-			nil)
 	case ProtocolFragmentationReceived:
 		conn.setStackFrag(true)
 	case ProtocolPMTUVerified:
@@ -504,6 +494,15 @@ func (conn *LocalConnection) sendFastHeartbeats() error {
 	return err
 }
 
+func (conn *LocalConnection) performFragTest() {
+	conn.setStackFrag(false)
+	conn.Forward(false, &ForwardedFrame{
+		srcPeer: conn.local,
+		dstPeer: conn.remote,
+		frame:   FragTest},
+		nil)
+}
+
 func tickerChan(ticker *time.Ticker) <-chan time.Time {
 	if ticker != nil {
 		return ticker.C
diff --git a/router/protocol.go b/router/protocol.go
index 5e9c7036..fcf8237a 100644
--- a/router/protocol.go
+++ b/router/protocol.go
@@ -2,7 +2,7 @@ package router
 
 const (
 	Protocol        = "weave"
-	ProtocolVersion = 18
+	ProtocolVersion = 19
 )
 
 type ProtocolTag byte
@@ -11,7 +11,6 @@ const (
 	ProtocolHeartbeat ProtocolTag = iota
 	ProtocolConnectionEstablished
 	ProtocolFragmentationReceived
-	ProtocolStartFragmentationTest
 	ProtocolPMTUVerified
 	ProtocolGossip
 	ProtocolGossipUnicast