Merge pull request #32 from digitalocean/mdl-ovs-flowstats14

ovs: extend FlowStats parsing for OpenFlow 1.4

Author: mdlayher

ovs/flowstats.go

@@ -34,55 +34,63 @@ type FlowStats struct {
 	ByteCount   uint64
 }
 
-// UnmarshalText unmarshals a FlowStats from textual form as output by
-// 'ovs-ofctl dump-aggregate <bridge> table=<tablename>,cookie=<cookie>/-1':
-//    NXST_AGGREGATE reply (xid=0x4): packet_count=642800 byte_count=141379644 flow_count=3
+// UnmarshalText unmarshals a FlowStats from textual form.
 func (f *FlowStats) UnmarshalText(b []byte) error {
 	// Make a copy per documentation for encoding.TextUnmarshaler.
 	s := string(b)
 
 	// Constants only needed within this method, to avoid polluting the
 	// package namespace with generic names
 	const (
-		nxstAggregate = "NXST_AGGREGATE"
-		packetCount   = "packet_count"
-		byteCount     = "byte_count"
+		packetCount = "packet_count"
+		byteCount   = "byte_count"
+		flowCount   = "flow_count"
 	)
 
-	ss := strings.Fields(s)
-	// Assumption here is that string s should have 6 fields of text separated by
-	// (5) spaces
-	if len(ss) != 6 {
+	// Find the index of packet count to find stats.
+	idx := strings.Index(s, packetCount)
+	if idx == -1 {
 		return ErrInvalidFlowStats
 	}
 
-	if ss[0] != nxstAggregate {
-		return ErrInvalidFlowStats
+	// Assume the last three fields are packets, bytes, and flows, in that order.
+	ss := strings.Fields(s[idx:])
+	fields := []string{
+		packetCount,
+		byteCount,
+		flowCount,
 	}
 
-	packetCountPair := strings.Split(ss[3], "=")
-	if len(packetCountPair) != 2 || packetCountPair[0] != packetCount {
+	if len(ss) != len(fields) {
 		return ErrInvalidFlowStats
 	}
 
-	n, err := strconv.ParseUint(packetCountPair[1], 10, 64)
-	if err != nil {
-		return ErrInvalidFlowStats
-	}
+	var values []uint64
+	for i := range ss {
+		// Split key from its integer value.
+		kv := strings.Split(ss[i], "=")
+		if len(kv) != 2 {
+			return ErrInvalidFlowStats
+		}
 
-	f.PacketCount = n
+		// Verify keys appear in expected order.
+		if kv[0] != fields[i] {
+			return ErrInvalidFlowStats
+		}
 
-	byteCountPair := strings.Split(ss[4], "=")
-	if len(byteCountPair) != 2 || byteCountPair[0] != byteCount {
-		return ErrInvalidFlowStats
-	}
+		n, err := strconv.ParseUint(kv[1], 10, 64)
+		if err != nil {
+			return err
+		}
 
-	n, err = strconv.ParseUint(byteCountPair[1], 10, 64)
-	if err != nil {
-		return ErrInvalidFlowStats
+		values = append(values, n)
 	}
 
-	f.ByteCount = n
+	*f = FlowStats{
+		PacketCount: values[0],
+		ByteCount:   values[1],
+		// Flow count unused.
+	}
 
 	return nil
 }

ovs/flowstats_test.go

@@ -15,77 +15,90 @@
 package ovs
 
 import (
-	"reflect"
 	"testing"
+
+	"github.com/google/go-cmp/cmp"
 )
 
 func TestFlowStatsUnmarshalText(t *testing.T) {
 	var tests = []struct {
-		desc string
-		s    string
-		p    *FlowStats
-		err  error
+		desc  string
+		s     string
+		stats *FlowStats
+		ok    bool
 	}{
 		{
 			desc: "empty string",
-			err:  ErrInvalidFlowStats,
 		},
 		{
-			desc: "incorrect number of fields",
-			s:    "NXST_AGGREGATE reply (xid=0x4): packet_count=642800 byte_count=141379644 flow_count=2, flow_count=3",
-			err:  ErrInvalidFlowStats,
+			desc: "too few fields",
+			s:    "NXST_AGGREGATE reply (xid=0x4): packet_count=642800 byte_count=141379644",
 		},
 		{
-			desc: "first field is not NXST_AGGREGATE",
-			s:    "NXST_REPLY reply (xid=0x4): packet_count=642800 byte_count=141379644 flow_count=2",
-			err:  ErrInvalidFlowStats,
+			desc: "too many fields",
+			s:    "NXST_AGGREGATE reply (xid=0x4): packet_count=642800 byte_count=141379644 flow_count=2, flow_count=3",
 		},
 		{
-			desc: "packet_count string is missing",
+			desc: "packet_count missing",
 			s:    "NXST_AGGREGATE reply (xid=0x4): frame_count=642800 byte_count=141379644 flow_count=2",
-			err:  ErrInvalidFlowStats,
 		},
 		{
-			desc: "byte_count string is missing",
+			desc: "byte_count missing",
 			s:    "NXST_AGGREGATE reply (xid=0x4): packet_count=642800 bits*8_count=141379644 flow_count=2",
-			err:  ErrInvalidFlowStats,
 		},
 		{
-			desc: "broken packet_count=value pair",
+			desc: "bad key=value",
+			s:    "NXST_AGGREGATE reply (xid=0x4): packet_count=1=foo byte_count=141379644 flow_count=2",
+		},
+		{
+			desc: "bad packet count",
 			s:    "NXST_AGGREGATE reply (xid=0x4): packet_count=toosmall byte_count=141379644 flow_count=2",
-			err:  ErrInvalidFlowStats,
 		},
 		{
-			desc: "broken byte_count=value pair",
+			desc: "bad byte count",
 			s:    "NXST_AGGREGATE reply (xid=0x4): packet_count=642800 byte_count=toolarge flow_count=2",
-			err:  ErrInvalidFlowStats,
+		},
+		{
+			desc: "bad flow count",
+			s:    "NXST_AGGREGATE reply (xid=0x4): packet_count=642800 byte_count=1 FLOW_count=2",
 		},
 		{
 			desc: "OK",
 			s:    "NXST_AGGREGATE reply (xid=0x4): packet_count=642800 byte_count=141379644 flow_count=2",
-			p: &FlowStats{
+			stats: &FlowStats{
 				PacketCount: 642800,
 				ByteCount:   141379644,
 			},
+			ok: true,
+		},
+		{
+			desc: "OK, OpenFlow 1.4",
+			s:    "OFPST_AGGREGATE reply (OF1.4) (xid=0x2): packet_count=1207 byte_count=101673 flow_count=1",
+			stats: &FlowStats{
+				PacketCount: 1207,
+				ByteCount:   101673,
+			},
+			ok: true,
 		},
 	}
 
 	for _, tt := range tests {
 		t.Run(tt.desc, func(t *testing.T) {
-			p := new(FlowStats)
-			err := p.UnmarshalText([]byte(tt.s))
+			stats := new(FlowStats)
+			err := stats.UnmarshalText([]byte(tt.s))
 
-			if want, got := errStr(tt.err), errStr(err); want != got {
-				t.Fatalf("unexpected error:\n- want: %v\n-  got: %v",
-					want, got)
+			if err != nil && tt.ok {
+				t.Fatalf("unexpected error: %v", err)
+			}
+			if err == nil && !tt.ok {
+				t.Fatal("expected an error, but none occurred")
 			}
 			if err != nil {
 				return
 			}
 
-			if want, got := tt.p, p; !reflect.DeepEqual(want, got) {
-				t.Fatalf("unexpected PortStats:\n- want: %#v\n-  got: %#v",
-					want, got)
+			if diff := cmp.Diff(tt.stats, stats); diff != "" {
+				t.Fatalf("unexpected FlowStats (-want +got):\n%s", diff)
 			}
 		})
 	}