ovsdb: reduce direct usage of OvsSet{}

In preparation for making internal struct members private,
convert usage of OvsSet{} in most places to accessors. The
accessors now also enforce set length restrictions.

Signed-off-by: Dan Williams <[email protected]>

ovsdb/bindings.go

@@ -116,18 +116,20 @@ func OvsToNativeSlice(baseType string, maxSize int, ovsElem interface{}) (interf
 	var nativeSet reflect.Value
 	switch ovsSet := ovsElem.(type) {
 	case OvsSet:
-		if maxSize > 0 && len(ovsSet.GoSet) > maxSize {
-			return nil, fmt.Errorf("set size %d exceeds max size %d", len(ovsSet.GoSet), maxSize)
+		if maxSize > 0 && ovsSet.Len() > maxSize {
+			return nil, fmt.Errorf("set size %d exceeds max size %d", ovsSet.Len(), maxSize)
 		}
-		nativeSet = reflect.MakeSlice(reflect.SliceOf(naType), 0, len(ovsSet.GoSet))
-		for _, v := range ovsSet.GoSet {
+		nativeSet = reflect.MakeSlice(reflect.SliceOf(naType), 0, ovsSet.Len())
+		if err := ovsSet.Range(func(i int, v interface{}) (bool, error) {
 			nv, err := OvsToNativeAtomic(baseType, v)
 			if err != nil {
-				return nil, err
+				return true, err
 			}
 			nativeSet = reflect.Append(nativeSet, reflect.ValueOf(nv))
+			return false, nil
+		}); err != nil {
+			return nil, err
 		}
-
 	default:
 		nativeSet = reflect.MakeSlice(reflect.SliceOf(naType), 0, 1)
 		nv, err := OvsToNativeAtomic(baseType, ovsElem)
@@ -155,9 +157,9 @@ func OvsToNative(column *ColumnSchema, ovsElem interface{}) (interface{}, error)
 		case reflect.Ptr:
 			switch ovsSet := ovsElem.(type) {
 			case OvsSet:
-				if len(ovsSet.GoSet) > 1 {
-					return nil, fmt.Errorf("expected a slice of len =< 1, but got a slice with %d elements", len(ovsSet.GoSet))
-				} else if len(ovsSet.GoSet) == 0 {
+				if ovsSet.Len() > 1 {
+					return nil, fmt.Errorf("expected a slice of len =< 1, but got a slice with %d elements", ovsSet.Len())
+				} else if ovsSet.Len() == 0 {
 					return reflect.Zero(naType).Interface(), nil
 				}
 				native, err := OvsToNativeAtomic(column.TypeObj.Key.Type, ovsSet.GoSet[0])

ovsdb/condition_test.go

@@ -9,6 +9,11 @@ import (
 )
 
 func TestConditionMarshalUnmarshalJSON(t *testing.T) {
+	stringSet, err := newOvsSetRaw(TypeString, &Unlimited, []interface{}{"foo", "bar", "baz"})
+	assert.NoError(t, err)
+	uuidSet, err := newOvsSetRaw(TypeUUID, &Unlimited, []interface{}{UUID{GoUUID: "foo"}, UUID{GoUUID: "bar"}})
+	assert.NoError(t, err)
+
 	tests := []struct {
 		name      string
 		condition Condition
@@ -71,7 +76,7 @@ func TestConditionMarshalUnmarshalJSON(t *testing.T) {
 		},
 		{
 			"test set",
-			Condition{"foo", ConditionExcludes, OvsSet{GoSet: []interface{}{"foo", "bar", "baz"}}},
+			Condition{"foo", ConditionExcludes, stringSet},
 			`[ "foo", "excludes", ["set",["foo", "bar", "baz"]] ]`,
 			false,
 		},
@@ -83,7 +88,7 @@ func TestConditionMarshalUnmarshalJSON(t *testing.T) {
 		},
 		{
 			"test uuid set",
-			Condition{"foo", ConditionExcludes, OvsSet{GoSet: []interface{}{UUID{GoUUID: "foo"}, UUID{GoUUID: "bar"}}}},
+			Condition{"foo", ConditionExcludes, uuidSet},
 			`[ "foo", "excludes", ["set",[["named-uuid", "foo"], ["named-uuid", "bar"]]] ]`,
 			false,
 		},

ovsdb/mutation_test.go

@@ -9,6 +9,11 @@ import (
 )
 
 func TestMutationMarshalUnmarshalJSON(t *testing.T) {
+	stringSet, err := newOvsSetRaw(TypeString, &Unlimited, []interface{}{"foo", "bar", "baz"})
+	assert.NoError(t, err)
+	uuidSet, err := newOvsSetRaw(TypeUUID, &Unlimited, []interface{}{UUID{GoUUID: "foo"}, UUID{GoUUID: "bar"}})
+	assert.NoError(t, err)
+
 	tests := []struct {
 		name     string
 		mutation Mutation
@@ -65,7 +70,7 @@ func TestMutationMarshalUnmarshalJSON(t *testing.T) {
 		},
 		{
 			"test set",
-			Mutation{"foo", MutateOperationInsert, OvsSet{GoSet: []interface{}{"foo", "bar", "baz"}}},
+			Mutation{"foo", MutateOperationInsert, stringSet},
 			`[ "foo", "insert", ["set",["foo", "bar", "baz"]] ]`,
 			false,
 		},
@@ -77,7 +82,7 @@ func TestMutationMarshalUnmarshalJSON(t *testing.T) {
 		},
 		{
 			"test uuid set",
-			Mutation{"foo", MutateOperationInsert, OvsSet{GoSet: []interface{}{UUID{GoUUID: "foo"}, UUID{GoUUID: "bar"}}}},
+			Mutation{"foo", MutateOperationInsert, uuidSet},
 			`[ "foo", "insert", ["set",[["named-uuid", "foo"], ["named-uuid", "bar"]]] ]`,
 			false,
 		},

ovsdb/notation_test.go

@@ -194,6 +194,13 @@ func TestOperationsMarshalUnmarshalJSON(t *testing.T) {
 }
 
 func TestOvsSliceToGoNotation(t *testing.T) {
+	emptySet, err := newOvsSetRaw(TypeString, &Unlimited, []interface{}{})
+	assert.NoError(t, err)
+	stringSet, err := newOvsSetRaw(TypeString, &Unlimited, []interface{}{"foo", "bar", "baz"})
+	assert.NoError(t, err)
+	uuidSet, err := newOvsSetRaw(TypeUUID, &Unlimited, []interface{}{UUID{GoUUID: "foo"}, UUID{GoUUID: "bar"}})
+	assert.NoError(t, err)
+
 	tests := []struct {
 		name    string
 		value   interface{}
@@ -209,19 +216,19 @@ func TestOvsSliceToGoNotation(t *testing.T) {
 		{
 			"empty set",
 			[]interface{}{"set", []interface{}{}},
-			OvsSet{GoSet: []interface{}{}},
+			emptySet,
 			false,
 		},
 		{
 			"set",
 			[]interface{}{"set", []interface{}{"foo", "bar", "baz"}},
-			OvsSet{GoSet: []interface{}{"foo", "bar", "baz"}},
+			stringSet,
 			false,
 		},
 		{
 			"uuid set",
 			[]interface{}{"set", []interface{}{[]interface{}{"named-uuid", "foo"}, []interface{}{"named-uuid", "bar"}}},
-			OvsSet{GoSet: []interface{}{UUID{GoUUID: "foo"}, UUID{GoUUID: "bar"}}},
+			uuidSet,
 			false,
 		},
 		{

ovsdb/set.go

@@ -15,7 +15,8 @@ import (
 // values in the set.  All of the <atom>s must have the same type, and all
 // values must be unique within the set.
 type OvsSet struct {
-	GoSet []interface{}
+	GoSet   []interface{}
+	maxSize int
 }
 
 // NewOvsSetRaw creates a new OVSDB style set from a Go interface (object)
@@ -49,7 +50,10 @@ func newOvsSetRaw(keyType string, maxSizePtr *int, obj interface{}) (OvsSet, err
 		v = reflect.ValueOf(obj).Elem()
 		if v.Kind() == reflect.Invalid {
 			// must be a nil pointer, so just return an empty set
-			return OvsSet{ovsSet}, nil
+			return OvsSet{
+				GoSet:   ovsSet,
+				maxSize: maxSize,
+			}, nil
 		}
 	} else {
 		v = reflect.ValueOf(obj)
@@ -94,7 +98,10 @@ func newOvsSetRaw(keyType string, maxSizePtr *int, obj interface{}) (OvsSet, err
 	default:
 		return OvsSet{}, fmt.Errorf("ovsset supports only go slice/string/numbers/uuid or pointers to those types")
 	}
-	return OvsSet{ovsSet}, nil
+	return OvsSet{
+		GoSet:   ovsSet,
+		maxSize: maxSize,
+	}, nil
 }
 
 // MarshalJSON wil marshal an OVSDB style Set in to a JSON byte array
@@ -114,6 +121,9 @@ func (o OvsSet) MarshalJSON() ([]byte, error) {
 // UnmarshalJSON will unmarshal a JSON byte array to an OVSDB style Set
 func (o *OvsSet) UnmarshalJSON(b []byte) (err error) {
 	o.GoSet = make([]interface{}, 0)
+	if o.maxSize == 0 {
+		o.maxSize = Unlimited
+	}
 	addToSet := func(o *OvsSet, v interface{}) error {
 		goVal, err := ovsSliceToGoNotation(v)
 		if err == nil {
@@ -138,6 +148,9 @@ func (o *OvsSet) UnmarshalJSON(b []byte) (err error) {
 			// it is a slice, but is not a set
 			return &json.UnmarshalTypeError{Value: reflect.ValueOf(inter).String(), Type: reflect.TypeOf(*o)}
 		}
+		if o.maxSize > 0 && len(oSet) > o.maxSize {
+			return fmt.Errorf("ovsset size %d exceeds max size %d", len(oSet), o.maxSize)
+		}
 		innerSet := oSet[1].([]interface{})
 		for _, val := range innerSet {
 			err := addToSet(o, val)
@@ -151,3 +164,49 @@ func (o *OvsSet) UnmarshalJSON(b []byte) (err error) {
 		return addToSet(o, inter)
 	}
 }
+
+func (o *OvsSet) Append(newVal ...interface{}) error {
+	if o.maxSize > 0 && len(o.GoSet)+len(newVal) > o.maxSize {
+		return fmt.Errorf("appending new value would exceed max set size %d", o.maxSize)
+	}
+	o.GoSet = append(o.GoSet, newVal...)
+	return nil
+}
+
+func (o *OvsSet) Len() int {
+	return len(o.GoSet)
+}
+
+func (o *OvsSet) Replace(idx int, newVal interface{}) error {
+	if idx > len(o.GoSet)-1 {
+		return fmt.Errorf("attempted to access element %d beyond end of array (length %d)", idx, len(o.GoSet))
+	}
+	o.GoSet[idx] = newVal
+	return nil
+}
+
+// HasElementType matches the given value's type with the set's element type.
+// It returns true if the set has at least one element, and that element is
+// of the given type, otherwise false.
+func (o *OvsSet) HasElementType(checkVal interface{}) bool {
+	if len(o.GoSet) == 0 {
+		return false
+	}
+	return reflect.ValueOf(checkVal).Type() == reflect.ValueOf(o.GoSet[0]).Type()
+}
+
+// Range iterates over elements of the set and calls the given function for
+// each element. The function should return true if iteration should terminate,
+// a value to return to the caller of Range(), and/or an error (which also
+// terminates iteration).
+func (o *OvsSet) Range(elemFn func(int, interface{}) (bool, error)) error {
+	for i, v := range o.GoSet {
+		done, err := elemFn(i, v)
+		if err != nil {
+			return err
+		} else if done {
+			return nil
+		}
+	}
+	return nil
+}

ovsdb/updates2.go

@@ -1,5 +1,7 @@
 package ovsdb
 
+import "fmt"
+
 // TableUpdates2 is an object that maps from a table name to a
 // TableUpdate2
 type TableUpdates2 map[string]TableUpdate2
@@ -19,47 +21,55 @@ type RowUpdate2 struct {
 }
 
 // AddTableUpdate adds a new TableUpdate to a TableUpdates
-func (t TableUpdates2) AddTableUpdate(table string, update TableUpdate2) {
+func (t TableUpdates2) AddTableUpdate(table string, update TableUpdate2) error {
 	if _, ok := t[table]; !ok {
 		t[table] = update
-	} else {
-		for uuid, row := range update {
-			t[table].AddRowUpdate(uuid, row)
+		return nil
+	}
+
+	for uuid, row := range update {
+		if err := t[table].AddRowUpdate(uuid, row); err != nil {
+			return err
 		}
 	}
+	return nil
 }
 
-func (t TableUpdates2) Merge(update TableUpdates2) {
+func (t TableUpdates2) Merge(update TableUpdates2) error {
 	for k, v := range update {
-		t.AddTableUpdate(k, v)
+		if err := t.AddTableUpdate(k, v); err != nil {
+			return err
+		}
 	}
+	return nil
 }
 
-func (t TableUpdate2) AddRowUpdate(uuid string, update *RowUpdate2) {
+func (t TableUpdate2) AddRowUpdate(uuid string, update *RowUpdate2) error {
 	if _, ok := t[uuid]; !ok {
 		t[uuid] = update
-	} else {
-		t[uuid].Merge(update)
+		return nil
 	}
+
+	return t[uuid].Merge(update)
 }
 
-func (r *RowUpdate2) Merge(new *RowUpdate2) {
+func (r *RowUpdate2) Merge(new *RowUpdate2) error {
 	// old.Delete() cannot be true, as we can't modify a row after it was deleted
 	if r.Delete != nil {
-		return
+		return fmt.Errorf("can't modify row after it's been deleted")
 	}
 	// we should never get two insert events
 	if r.Insert != nil && new.Insert != nil {
-		return
+		return fmt.Errorf("should never get two insert events")
 	}
 	if r.Insert != nil && new.Modify != nil {
 		r.Insert = new.Modify
-		return
+		return nil
 	}
 	if r.Insert != nil && new.Delete != nil {
 		r.Insert = nil
 		r.Delete = new.Delete
-		return
+		return nil
 	}
 	if r.Modify != nil && new.Modify != nil {
 		currentRowData := *r.Modify
@@ -72,7 +82,9 @@ func (r *RowUpdate2) Merge(new *RowUpdate2) {
 				case OvsSet:
 					oSet := currentRowData[k].(OvsSet)
 					newSet := v.(OvsSet)
-					oSet.GoSet = append(oSet.GoSet, newSet.GoSet...)
+					if err := oSet.Append(newSet.GoSet...); err != nil {
+						return err
+					}
 					// copy new appended set back to currentRowData
 					currentRowData[k] = oSet
 				case OvsMap:
@@ -90,11 +102,12 @@ func (r *RowUpdate2) Merge(new *RowUpdate2) {
 		}
 		r.Modify = &currentRowData
 		r.New = new.New
-		return
+		return nil
 	}
 	if r.Modify != nil && new.Delete != nil {
 		r.Modify = nil
 		r.Delete = new.Delete
-		return
+		return nil
 	}
+	return nil
 }