Clean NewVM func

qemucli/kv_arg.go

@@ -7,13 +7,18 @@ import (
 	"github.com/pkg/errors"
 )
 
-type MapArg struct {
-	key    string
-	values map[string]string
+type KeyValueArgItem struct {
+	Key   string
+	Value string
 }
 
-func MustNewMapArg(key string, values map[string]string) *MapArg {
-	a, err := NewMapArg(key, values)
+type KeyValueArg struct {
+	key   string
+	items []KeyValueArgItem
+}
+
+func MustNewKeyValueArg(key string, items []KeyValueArgItem) *KeyValueArg {
+	a, err := NewKeyValueArg(key, items)
 	if err != nil {
 		panic(err)
 	}
@@ -21,10 +26,13 @@ func MustNewMapArg(key string, values map[string]string) *MapArg {
 	return a
 }
 
-func NewMapArg(key string, values map[string]string) (*MapArg, error) {
-	a := &MapArg{
-		key:    key,
-		values: make(map[string]string),
+func NewKeyValueArg(key string, items []KeyValueArgItem) (*KeyValueArg, error) {
+	a := &KeyValueArg{
+		key: key,
+		// We're creating a copy here because we do not
+		// want to reference to any external source
+		// that can be modified after we've done checks.
+		items: make([]KeyValueArgItem, len(items)),
 	}
 
 	// Preflight arg key/type check.
@@ -33,60 +41,71 @@ func NewMapArg(key string, values map[string]string) (*MapArg, error) {
 		return nil, errors.Wrap(err, "validate arg key")
 	}
 
-	for k, v := range values {
-		// The reason why we're making copies here and creating
-		// a whole other copy of the entire map is because maps
-		// are pointers, and we do not want to reference anything
-		// that we will not be able to validate except at this
-		// stage of MapArg creation.
-		k := k
-		v := v
+	for i, item := range items {
+		// We're making a copy here because we don't want to
+		// leave the possibility to modify the value remotely
+		// after the checks are done. Slices are pointers, and
+		// no copies are made when passing a slice is passed
+		// through to a function.
+		item := item
 
-		if len(k) == 0 {
-			return nil, fmt.Errorf("empty map key not allowed")
+		if len(item.Key) == 0 {
+			return nil, fmt.Errorf("empty key not allowed")
 		}
 
-		if len(v) == 1 {
+		if len(item.Value) == 0 {
 			// Values *can* be empty, though. We do not allow them for consistency.
-			return nil, fmt.Errorf("empty map value for key '%v' is not allowed", k)
+			return nil, fmt.Errorf("empty value for key '%v' is not allowed", item.Key)
 		}
 
-		err := validateArgStrValue(k)
+		err := validateArgStrValue(item.Key)
 		if err != nil {
-			return nil, errors.Wrapf(err, "validate map key '%v'", k)
+			return nil, errors.Wrapf(err, "validate key '%v'", item.Key)
 		}
 
-		err = validateArgStrValue(v)
+		err = validateArgStrValue(item.Value)
 		if err != nil {
-			return nil, errors.Wrapf(err, "validate map value '%v'", v)
+			return nil, errors.Wrapf(err, "validate map value '%v'", item.Value)
 		}
 
-		a.values[k] = v
+		a.items[i] = item
 	}
 
 	return a, nil
 }
 
-func (a *MapArg) StringKey() string {
+func (a *KeyValueArg) StringKey() string {
 	return a.key
 }
 
-func (a *MapArg) StringValue() string {
+func (a *KeyValueArg) StringValue() string {
 	sb := new(strings.Builder)
-	for k, v := range a.values {
+	for i, item := range a.items {
 		// We're not validating anything here because
 		// we expect that the keys/values were validated
 		// at the creation of the MapArg.
 
-		sb.WriteString(k)
-		if len(v) > 0 {
-			sb.WriteString("=" + v)
+		if item.Key == "" {
+			// But if for whatever reason it happens that
+			// a key is blank, we skip the entire item because
+			// otherwise it's bad syntax.
+			continue
+		}
+
+		if i != 0 {
+			sb.WriteString(",")
+		}
+
+		sb.WriteString(item.Key)
+		if len(item.Value) > 0 {
+			// Item values can theoretically be empty.
+			sb.WriteString("=" + item.Value)
 		}
 	}
 
 	return sb.String()
 }
 
-func (a *MapArg) ValueType() ArgAcceptedValue {
-	return ArgAcceptedValueMap
+func (a *KeyValueArg) ValueType() ArgAcceptedValue {
+	return ArgAcceptedValueKeyValue
 }