Simplifying ONU channels and packet responders

Change-Id: I1f3912367a96564986b4209b7864e9fd4b507e8e
diff --git a/internal/bbsim/responders/dhcp/dhcp.go b/internal/bbsim/responders/dhcp/dhcp.go
index d2a3db7..64e83dd 100644
--- a/internal/bbsim/responders/dhcp/dhcp.go
+++ b/internal/bbsim/responders/dhcp/dhcp.go
@@ -30,6 +30,8 @@
 	"reflect"
 )
 
+var GetGemPortId = omci.GetGemPortId
+
 var dhcpLogger = log.WithFields(log.Fields{
 	"module": "DHCP",
 })
@@ -194,7 +196,7 @@
 
 func sendDHCPPktIn(msg bbsim.ByteMsg, stream openolt.Openolt_EnableIndicationServer) error {
 	// FIXME unify sendDHCPPktIn and sendEapolPktIn methods
-	gemid, err := omci.GetGemPortId(msg.IntfId, msg.OnuId)
+	gemid, err := GetGemPortId(msg.IntfId, msg.OnuId)
 	if err != nil {
 		dhcpLogger.WithFields(log.Fields{
 			"OnuId":  msg.OnuId,
@@ -213,33 +215,6 @@
 	return nil
 }
 
-func sendDHCPDiscovery(ponPortId uint32, onuId uint32, serialNumber string, onuHwAddress net.HardwareAddr, cTag int, stream openolt.Openolt_EnableIndicationServer) error {
-	dhcp := createDHCPDisc(ponPortId, onuId)
-	pkt, err := serializeDHCPPacket(ponPortId, onuId, onuHwAddress, dhcp)
-	if err != nil {
-		dhcpLogger.Errorf("Cannot serializeDHCPPacket: %s", err)
-		return err
-	}
-	// NOTE I don't think we need to tag the packet
-	//taggedPkt, err := packetHandlers.PushSingleTag(cTag, pkt)
-
-	msg := bbsim.ByteMsg{
-		IntfId: ponPortId,
-		OnuId:  onuId,
-		Bytes:  pkt,
-	}
-
-	if err := sendDHCPPktIn(msg, stream); err != nil {
-		return err
-	}
-	dhcpLogger.WithFields(log.Fields{
-		"OnuId":  onuId,
-		"IntfId": ponPortId,
-		"OnuSn":  serialNumber,
-	}).Infof("DHCPDiscovery Sent")
-	return nil
-}
-
 func sendDHCPRequest(ponPortId uint32, onuId uint32, serialNumber string, onuHwAddress net.HardwareAddr, cTag int, stream openolt.Openolt_EnableIndicationServer) error {
 	dhcp := createDHCPReq(ponPortId, onuId)
 	pkt, err := serializeDHCPPacket(ponPortId, onuId, onuHwAddress, dhcp)
@@ -268,38 +243,45 @@
 	return nil
 }
 
-func CreateDHCPClient(onuId uint32, ponPortId uint32, serialNumber string, onuHwAddress net.HardwareAddr, cTag int, onuStateMachine *fsm.FSM, stream openolt.Openolt_EnableIndicationServer, pktOutCh chan *bbsim.ByteMsg) {
-	// NOTE pckOutCh is channel to listen on for packets received by VOLTHA
-	// the OLT device will publish messages on that channel
-
-	dhcpLogger.WithFields(log.Fields{
-		"OnuId":  onuId,
-		"IntfId": ponPortId,
-		"OnuSn":  serialNumber,
-	}).Infof("DHCP State Machine starting")
-
-	defer dhcpLogger.WithFields(log.Fields{
-		"OnuId":  onuId,
-		"IntfId": ponPortId,
-		"OnuSn":  serialNumber,
-	}).Infof("DHCP State machine completed")
-
-	// Send DHCP Discovery packet
-	if err := sendDHCPDiscovery(ponPortId, onuId, serialNumber, onuHwAddress, cTag, stream); err != nil {
+func updateDhcpFailed(onuId uint32, ponPortId uint32, serialNumber string, onuStateMachine *fsm.FSM) error {
+	if err := onuStateMachine.Event("dhcp_failed"); err != nil {
 		dhcpLogger.WithFields(log.Fields{
 			"OnuId":  onuId,
 			"IntfId": ponPortId,
 			"OnuSn":  serialNumber,
-		}).Errorf("Can't send DHCP Discovery: %s", err)
-		if err := onuStateMachine.Event("dhcp_failed"); err != nil {
-			dhcpLogger.WithFields(log.Fields{
-				"OnuId":  onuId,
-				"IntfId": ponPortId,
-				"OnuSn":  serialNumber,
-			}).Errorf("Error while transitioning ONU State %v", err)
-		}
-		return
+		}).Errorf("Error while transitioning ONU State %v", err)
+		return err
 	}
+	return nil
+}
+
+func SendDHCPDiscovery(ponPortId uint32, onuId uint32, serialNumber string, onuStateMachine *fsm.FSM, onuHwAddress net.HardwareAddr, cTag int, stream openolt.Openolt_EnableIndicationServer) error {
+	dhcp := createDHCPDisc(ponPortId, onuId)
+	pkt, err := serializeDHCPPacket(ponPortId, onuId, onuHwAddress, dhcp)
+	if err != nil {
+		dhcpLogger.Errorf("Cannot serializeDHCPPacket: %s", err)
+		return err
+	}
+	// NOTE I don't think we need to tag the packet
+	//taggedPkt, err := packetHandlers.PushSingleTag(cTag, pkt)
+
+	msg := bbsim.ByteMsg{
+		IntfId: ponPortId,
+		OnuId:  onuId,
+		Bytes:  pkt,
+	}
+
+	if err := sendDHCPPktIn(msg, stream); err != nil {
+		if err := updateDhcpFailed(onuId, ponPortId, serialNumber, onuStateMachine); err != nil {
+			return err
+		}
+		return err
+	}
+	dhcpLogger.WithFields(log.Fields{
+		"OnuId":  onuId,
+		"IntfId": ponPortId,
+		"OnuSn":  serialNumber,
+	}).Infof("DHCPDiscovery Sent")
 
 	if err := onuStateMachine.Event("dhcp_discovery_sent"); err != nil {
 		dhcpLogger.WithFields(log.Fields{
@@ -308,80 +290,79 @@
 			"OnuSn":  serialNumber,
 		}).Errorf("Error while transitioning ONU State %v", err)
 	}
+	return nil
+}
 
-	dhcpLogger.WithFields(log.Fields{
-		"OnuId":  onuId,
-		"IntfId": ponPortId,
-		"OnuSn":  serialNumber,
-	}).Infof("Listening on dhcpPktOutCh")
+func HandleNextPacket(onuId uint32, ponPortId uint32, serialNumber string, onuHwAddress net.HardwareAddr, cTag int, onuStateMachine *fsm.FSM, pkt gopacket.Packet, stream openolt.Openolt_EnableIndicationServer) error {
 
-	for msg := range pktOutCh {
-		dhcpLogger.Tracef("Received DHCP message %v", msg)
-
-		pkt := gopacket.NewPacket(msg.Bytes, layers.LayerTypeEthernet, gopacket.Default)
-		dhcpLayer, err := getDhcpLayer(pkt)
-		if err != nil {
-			dhcpLogger.WithFields(log.Fields{
-				"OnuId":  onuId,
-				"IntfId": ponPortId,
-				"OnuSn":  serialNumber,
-			}).Errorf("Can't get DHCP Layer from Packet: %v", err)
-			continue
+	dhcpLayer, err := getDhcpLayer(pkt)
+	if err != nil {
+		dhcpLogger.WithFields(log.Fields{
+			"OnuId":  onuId,
+			"IntfId": ponPortId,
+			"OnuSn":  serialNumber,
+		}).Errorf("Can't get DHCP Layer from Packet: %v", err)
+		if err := updateDhcpFailed(onuId, ponPortId, serialNumber, onuStateMachine); err != nil {
+			return err
 		}
-		dhcpMessageType, err := getDhcpMessageType(dhcpLayer)
-		if err != nil {
-			dhcpLogger.WithFields(log.Fields{
-				"OnuId":  onuId,
-				"IntfId": ponPortId,
-				"OnuSn":  serialNumber,
-			}).Errorf("Can't get DHCP Message Type from DHCP Layer: %v", err)
-			continue
-		}
-
-		if dhcpLayer.Operation == layers.DHCPOpReply {
-			if dhcpMessageType == layers.DHCPMsgTypeOffer {
-				if err := sendDHCPRequest(ponPortId, onuId, serialNumber, onuHwAddress, cTag, stream); err != nil {
-					dhcpLogger.WithFields(log.Fields{
-						"OnuId":  onuId,
-						"IntfId": ponPortId,
-						"OnuSn":  serialNumber,
-					}).Errorf("Can't send DHCP Request: %s", err)
-					if err := onuStateMachine.Event("dhcp_failed"); err != nil {
-						dhcpLogger.WithFields(log.Fields{
-							"OnuId":  onuId,
-							"IntfId": ponPortId,
-							"OnuSn":  serialNumber,
-						}).Errorf("Error while transitioning ONU State %v", err)
-					}
-					return
-				}
-				if err := onuStateMachine.Event("dhcp_request_sent"); err != nil {
-					dhcpLogger.WithFields(log.Fields{
-						"OnuId":  onuId,
-						"IntfId": ponPortId,
-						"OnuSn":  serialNumber,
-					}).Errorf("Error while transitioning ONU State %v", err)
-				}
-
-			} else if dhcpMessageType == layers.DHCPMsgTypeAck {
-				// NOTE once the ack is received we don't need to do anything but change the state
-				if err := onuStateMachine.Event("dhcp_ack_received"); err != nil {
-					dhcpLogger.WithFields(log.Fields{
-						"OnuId":  onuId,
-						"IntfId": ponPortId,
-						"OnuSn":  serialNumber,
-					}).Errorf("Error while transitioning ONU State %v", err)
-				}
-				return
-			}
-			// NOTE do we need to care about DHCPMsgTypeRelease??
-		} else {
-			dhcpLogger.WithFields(log.Fields{
-				"OnuId":  onuId,
-				"IntfId": ponPortId,
-				"OnuSn":  serialNumber,
-			}).Warnf("Unsupported DHCP Operation: %s", dhcpLayer.Operation.String())
-			continue
-		}
+		return err
 	}
+	dhcpMessageType, err := getDhcpMessageType(dhcpLayer)
+	if err != nil {
+		dhcpLogger.WithFields(log.Fields{
+			"OnuId":  onuId,
+			"IntfId": ponPortId,
+			"OnuSn":  serialNumber,
+		}).Errorf("Can't get DHCP Message Type from DHCP Layer: %v", err)
+		if err := updateDhcpFailed(onuId, ponPortId, serialNumber, onuStateMachine); err != nil {
+			return err
+		}
+		return err
+	}
+
+	if dhcpLayer.Operation == layers.DHCPOpReply {
+		if dhcpMessageType == layers.DHCPMsgTypeOffer {
+			if err := sendDHCPRequest(ponPortId, onuId, serialNumber, onuHwAddress, cTag, stream); err != nil {
+				dhcpLogger.WithFields(log.Fields{
+					"OnuId":  onuId,
+					"IntfId": ponPortId,
+					"OnuSn":  serialNumber,
+				}).Errorf("Can't send DHCP Request: %s", err)
+				if err := updateDhcpFailed(onuId, ponPortId, serialNumber, onuStateMachine); err != nil {
+					return err
+				}
+				return err
+			}
+			if err := onuStateMachine.Event("dhcp_request_sent"); err != nil {
+				dhcpLogger.WithFields(log.Fields{
+					"OnuId":  onuId,
+					"IntfId": ponPortId,
+					"OnuSn":  serialNumber,
+				}).Errorf("Error while transitioning ONU State %v", err)
+			}
+
+		} else if dhcpMessageType == layers.DHCPMsgTypeAck {
+			// NOTE once the ack is received we don't need to do anything but change the state
+			if err := onuStateMachine.Event("dhcp_ack_received"); err != nil {
+				dhcpLogger.WithFields(log.Fields{
+					"OnuId":  onuId,
+					"IntfId": ponPortId,
+					"OnuSn":  serialNumber,
+				}).Errorf("Error while transitioning ONU State %v", err)
+			}
+			dhcpLogger.WithFields(log.Fields{
+				"OnuId":  onuId,
+				"IntfId": ponPortId,
+				"OnuSn":  serialNumber,
+			}).Infof("DHCP State machine completed")
+		}
+		// NOTE do we need to care about DHCPMsgTypeRelease??
+	} else {
+		dhcpLogger.WithFields(log.Fields{
+			"OnuId":  onuId,
+			"IntfId": ponPortId,
+			"OnuSn":  serialNumber,
+		}).Warnf("Unsupported DHCP Operation: %s", dhcpLayer.Operation.String())
+	}
+	return nil
 }
diff --git a/internal/bbsim/responders/dhcp/dhcp_test.go b/internal/bbsim/responders/dhcp/dhcp_test.go
new file mode 100644
index 0000000..0733cc0
--- /dev/null
+++ b/internal/bbsim/responders/dhcp/dhcp_test.go
@@ -0,0 +1,120 @@
+/*
+ * Copyright 2018-present Open Networking Foundation
+
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+
+ * http://www.apache.org/licenses/LICENSE-2.0
+
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package dhcp
+
+import (
+	"errors"
+	"github.com/looplab/fsm"
+	"github.com/opencord/voltha-protos/go/openolt"
+	"google.golang.org/grpc"
+	"gotest.tools/assert"
+	"net"
+	"testing"
+)
+
+// MOCKS
+var calledSend = 0
+
+var dhcpStateMachine = fsm.NewFSM(
+	"dhcp_started",
+	fsm.Events{
+		{Name: "dhcp_discovery_sent", Src: []string{"dhcp_started"}, Dst: "dhcp_discovery_sent"},
+		{Name: "dhcp_request_sent", Src: []string{"dhcp_discovery_sent"}, Dst: "dhcp_request_sent"},
+		{Name: "dhcp_ack_received", Src: []string{"dhcp_request_sent"}, Dst: "dhcp_ack_received"},
+		{Name: "dhcp_failed", Src: []string{"dhcp_started", "dhcp_discovery_sent", "dhcp_request_sent"}, Dst: "dhcp_failed"},
+	},
+	fsm.Callbacks{},
+)
+
+type mockStreamSuccess struct {
+	grpc.ServerStream
+}
+
+func (s mockStreamSuccess) Send(ind *openolt.Indication) error {
+	calledSend++
+	return nil
+}
+
+type mockStreamError struct {
+	grpc.ServerStream
+}
+
+func (s mockStreamError) Send(ind *openolt.Indication) error {
+	calledSend++
+	return errors.New("stream-error")
+}
+
+// TESTS
+
+func TestSendDHCPDiscovery(t *testing.T) {
+	calledSend = 0
+	dhcpStateMachine.SetState("dhcp_started")
+
+	// Save current function and restore at the end:
+	old := GetGemPortId
+	defer func() { GetGemPortId = old }()
+
+	GetGemPortId = func(intfId uint32, onuId uint32) (uint16, error) {
+		return 1, nil
+	}
+
+	var onuId uint32 = 1
+	var ponPortId uint32 = 0
+	var serialNumber string = "BBSM00000001"
+	var mac = net.HardwareAddr{0x2e, 0x60, 0x70, 0x13, byte(ponPortId), byte(onuId)}
+
+	stream := mockStreamSuccess{}
+
+	if err := SendDHCPDiscovery(ponPortId, onuId, serialNumber, dhcpStateMachine, mac, 1, stream); err != nil {
+		t.Errorf("SendDHCPDiscovery returned an error: %v", err)
+		t.Fail()
+	}
+
+	assert.Equal(t, calledSend, 1)
+
+	assert.Equal(t, dhcpStateMachine.Current(), "dhcp_discovery_sent")
+}
+
+// TODO test dhcp.HandleNextPacket
+
+func TestUpdateDhcpFailed(t *testing.T) {
+
+	var onuId uint32 = 1
+	var ponPortId uint32 = 0
+	var serialNumber string = "BBSM00000001"
+
+	dhcpStateMachine.SetState("dhcp_started")
+	updateDhcpFailed(onuId, ponPortId, serialNumber, dhcpStateMachine)
+	assert.Equal(t, dhcpStateMachine.Current(), "dhcp_failed")
+
+	dhcpStateMachine.SetState("dhcp_discovery_sent")
+	updateDhcpFailed(onuId, ponPortId, serialNumber, dhcpStateMachine)
+	assert.Equal(t, dhcpStateMachine.Current(), "dhcp_failed")
+
+	dhcpStateMachine.SetState("dhcp_request_sent")
+	updateDhcpFailed(onuId, ponPortId, serialNumber, dhcpStateMachine)
+	assert.Equal(t, dhcpStateMachine.Current(), "dhcp_failed")
+
+	dhcpStateMachine.SetState("dhcp_ack_received")
+	err := updateDhcpFailed(onuId, ponPortId, serialNumber, dhcpStateMachine)
+	if err == nil {
+		t.Errorf("updateDhcpFailed did not return an error")
+		t.Fail()
+	}
+	assert.Equal(t, err.Error(), "event dhcp_failed inappropriate in current state dhcp_ack_received")
+
+}