Add test coverage to opts and refactor

- Refactor opts.ValidatePath and add an opts.ValidateDevice
  ValidePath will now accept : containerPath:mode, hostPath:containerPath:mode
  and hostPath:containerPath.
  ValidateDevice will have the same behavior as current.

- Refactor opts.ValidateEnv, opts.ParseEnvFile
  Environment variables will now be validated with the following
  definition :
  > Environment variables set by the user must have a name consisting
  > solely of alphabetics, numerics, and underscores - the first of
  > which must not be numeric.

Signed-off-by: Vincent Demeester <vincent@sbr.pm>

daemon/config.go

@@ -54,7 +54,7 @@ func (config *Config) InstallCommonFlags() {
 	flag.StringVar(&config.CorsHeaders, []string{"-api-cors-header"}, "", "Set CORS headers in the remote API")
 	// FIXME: why the inconsistency between "hosts" and "sockets"?
 	opts.IPListVar(&config.Dns, []string{"#dns", "-dns"}, "DNS server to use")
-	opts.DnsSearchListVar(&config.DnsSearch, []string{"-dns-search"}, "DNS search domains to use")
+	opts.DNSSearchListVar(&config.DnsSearch, []string{"-dns-search"}, "DNS search domains to use")
 	opts.LabelListVar(&config.Labels, []string{"-label"}, "Set key=value labels to the daemon")
 	flag.StringVar(&config.LogConfig.Type, []string{"-log-driver"}, "json-file", "Default driver for container logs")
 	opts.LogOptsVar(config.LogConfig.Config, []string{"-log-opt"}, "Set log driver options")

daemon/volumes.go

@@ -73,10 +73,11 @@ func parseBindMount(spec string, mountLabel string, config *runconfig.Config) (*
 	case 3:
 		bind.Destination = arr[1]
 		mode := arr[2]
-		if !validMountMode(mode) {
+		isValid, isRw := volume.ValidateMountMode(mode)
+		if !isValid {
 			return nil, fmt.Errorf("invalid mode for volumes-from: %s", mode)
 		}
-		bind.RW = rwModes[mode]
+		bind.RW = isRw
 		// Relabel will apply a SELinux label, if necessary
 		bind.Relabel = mode
 	default:
@@ -113,37 +114,13 @@ func parseVolumesFrom(spec string) (string, string, error) {
 
 	if len(specParts) == 2 {
 		mode = specParts[1]
-		if !validMountMode(mode) {
+		if isValid, _ := volume.ValidateMountMode(mode); !isValid {
 			return "", "", fmt.Errorf("invalid mode for volumes-from: %s", mode)
 		}
 	}
 	return id, mode, nil
 }
 
-// read-write modes
-var rwModes = map[string]bool{
-	"rw":   true,
-	"rw,Z": true,
-	"rw,z": true,
-	"z,rw": true,
-	"Z,rw": true,
-	"Z":    true,
-	"z":    true,
-}
-
-// read-only modes
-var roModes = map[string]bool{
-	"ro":   true,
-	"ro,Z": true,
-	"ro,z": true,
-	"z,ro": true,
-	"Z,ro": true,
-}
-
-func validMountMode(mode string) bool {
-	return roModes[mode] || rwModes[mode]
-}
-
 func copyExistingContents(source, destination string) error {
 	volList, err := ioutil.ReadDir(source)
 	if err != nil {

opts/envfile.go

@@ -4,12 +4,18 @@ import (
 	"bufio"
 	"fmt"
 	"os"
+	"regexp"
 	"strings"
 )
 
-/*
+var (
-Read in a line delimited file with environment variables enumerated
+	// EnvironmentVariableRegexp A regexp to validate correct environment variables
-*/
+	// Environment variables set by the user must have a name consisting solely of
+	// alphabetics, numerics, and underscores - the first of which must not be numeric.
+	EnvironmentVariableRegexp = regexp.MustCompile("^[[:alpha:]_][[:alpha:][:digit:]_]*$")
+)
+
+// ParseEnvFile Read in a line delimited file with environment variables enumerated
 func ParseEnvFile(filename string) ([]string, error) {
 	fh, err := os.Open(filename)
 	if err != nil {
@@ -23,14 +29,15 @@ func ParseEnvFile(filename string) ([]string, error) {
 		line := scanner.Text()
 		// line is not empty, and not starting with '#'
 		if len(line) > 0 && !strings.HasPrefix(line, "#") {
-			if strings.Contains(line, "=") {
+			data := strings.SplitN(line, "=", 2)
-				data := strings.SplitN(line, "=", 2)
 
-				// trim the front of a variable, but nothing else
+			// trim the front of a variable, but nothing else
-				variable := strings.TrimLeft(data[0], whiteSpaces)
+			variable := strings.TrimLeft(data[0], whiteSpaces)
-				if strings.ContainsAny(variable, whiteSpaces) {
+
-					return []string{}, ErrBadEnvVariable{fmt.Sprintf("variable '%s' has white spaces", variable)}
+			if !EnvironmentVariableRegexp.MatchString(variable) {
-				}
+				return []string{}, ErrBadEnvVariable{fmt.Sprintf("variable '%s' is not a valid environment variable", variable)}
+			}
+			if len(data) > 1 {
 
 				// pass the value through, no trimming
 				lines = append(lines, fmt.Sprintf("%s=%s", variable, data[1]))
@@ -45,6 +52,7 @@ func ParseEnvFile(filename string) ([]string, error) {
 
 var whiteSpaces = " \t"
 
+// ErrBadEnvVariable typed error for bad environment variable
 type ErrBadEnvVariable struct {
 	msg string
 }

opts/envfile_test.go

@@ -10,15 +10,15 @@ import (
 	"testing"
 )
 
-func tmpFileWithContent(content string) (string, error) {
+func tmpFileWithContent(content string, t *testing.T) string {
 	tmpFile, err := ioutil.TempFile("", "envfile-test")
 	if err != nil {
-		return "", err
+		t.Fatal(err)
 	}
 	defer tmpFile.Close()
 
 	tmpFile.WriteString(content)
-	return tmpFile.Name(), nil
+	return tmpFile.Name()
 }
 
 // Test ParseEnvFile for a file with a few well formatted lines
@@ -27,42 +27,36 @@ func TestParseEnvFileGoodFile(t *testing.T) {
     baz=quux
 # comment
 
-foobar=foobaz
+_foobar=foobaz
 `
 
-	tmpFile, err := tmpFileWithContent(content)
+	tmpFile := tmpFileWithContent(content, t)
-	if err != nil {
-		t.Fatal("failed to create test data file")
-	}
 	defer os.Remove(tmpFile)
 
 	lines, err := ParseEnvFile(tmpFile)
 	if err != nil {
-		t.Fatal("ParseEnvFile failed; expected success")
+		t.Fatal(err)
 	}
 
-	expected_lines := []string{
+	expectedLines := []string{
 		"foo=bar",
 		"baz=quux",
-		"foobar=foobaz",
+		"_foobar=foobaz",
 	}
 
-	if !reflect.DeepEqual(lines, expected_lines) {
+	if !reflect.DeepEqual(lines, expectedLines) {
 		t.Fatal("lines not equal to expected_lines")
 	}
 }
 
 // Test ParseEnvFile for an empty file
 func TestParseEnvFileEmptyFile(t *testing.T) {
-	tmpFile, err := tmpFileWithContent("")
+	tmpFile := tmpFileWithContent("", t)
-	if err != nil {
-		t.Fatal("failed to create test data file")
-	}
 	defer os.Remove(tmpFile)
 
 	lines, err := ParseEnvFile(tmpFile)
 	if err != nil {
-		t.Fatal("ParseEnvFile failed; expected success")
+		t.Fatal(err)
 	}
 
 	if len(lines) != 0 {
@@ -76,6 +70,9 @@ func TestParseEnvFileNonExistentFile(t *testing.T) {
 	if err == nil {
 		t.Fatal("ParseEnvFile succeeded; expected failure")
 	}
+	if _, ok := err.(*os.PathError); !ok {
+		t.Fatalf("Expected a PathError, got [%v]", err)
+	}
 }
 
 // Test ParseEnvFile for a badly formatted file
@@ -84,15 +81,19 @@ func TestParseEnvFileBadlyFormattedFile(t *testing.T) {
     f   =quux
 `
 
-	tmpFile, err := tmpFileWithContent(content)
+	tmpFile := tmpFileWithContent(content, t)
-	if err != nil {
-		t.Fatal("failed to create test data file")
-	}
 	defer os.Remove(tmpFile)
 
-	_, err = ParseEnvFile(tmpFile)
+	_, err := ParseEnvFile(tmpFile)
 	if err == nil {
-		t.Fatal("ParseEnvFile succeeded; expected failure")
+		t.Fatalf("Expected a ErrBadEnvVariable, got nothing")
+	}
+	if _, ok := err.(ErrBadEnvVariable); !ok {
+		t.Fatalf("Expected a ErrBadEnvVariable, got [%v]", err)
+	}
+	expectedMessage := "poorly formatted environment: variable 'f   ' is not a valid environment variable"
+	if err.Error() != expectedMessage {
+		t.Fatalf("Expected [%v], got [%v]", expectedMessage, err.Error())
 	}
 }
 
@@ -101,14 +102,32 @@ func TestParseEnvFileLineTooLongFile(t *testing.T) {
 	content := strings.Repeat("a", bufio.MaxScanTokenSize+42)
 	content = fmt.Sprint("foo=", content)
 
-	tmpFile, err := tmpFileWithContent(content)
+	tmpFile := tmpFileWithContent(content, t)
-	if err != nil {
-		t.Fatal("failed to create test data file")
-	}
 	defer os.Remove(tmpFile)
 
-	_, err = ParseEnvFile(tmpFile)
+	_, err := ParseEnvFile(tmpFile)
 	if err == nil {
 		t.Fatal("ParseEnvFile succeeded; expected failure")
 	}
 }
+
+// ParseEnvFile with a random file, pass through
+func TestParseEnvFileRandomFile(t *testing.T) {
+	content := `first line
+another invalid line`
+	tmpFile := tmpFileWithContent(content, t)
+	defer os.Remove(tmpFile)
+
+	_, err := ParseEnvFile(tmpFile)
+
+	if err == nil {
+		t.Fatalf("Expected a ErrBadEnvVariable, got nothing")
+	}
+	if _, ok := err.(ErrBadEnvVariable); !ok {
+		t.Fatalf("Expected a ErrBadEnvvariable, got [%v]", err)
+	}
+	expectedMessage := "poorly formatted environment: variable 'first line' is not a valid environment variable"
+	if err.Error() != expectedMessage {
+		t.Fatalf("Expected [%v], got [%v]", expectedMessage, err.Error())
+	}
+}

opts/ip.go

@@ -5,6 +5,7 @@ import (
 	"net"
 )
 
+// IpOpt type that hold an IP
 type IpOpt struct {
 	*net.IP
 }

opts/ip_test.go

@@ -0,0 +1,54 @@
+package opts
+
+import (
+	"net"
+	"testing"
+)
+
+func TestIpOptString(t *testing.T) {
+	addresses := []string{"", "0.0.0.0"}
+	var ip net.IP
+
+	for _, address := range addresses {
+		stringAddress := NewIpOpt(&ip, address).String()
+		if stringAddress != address {
+			t.Fatalf("IpOpt string should be `%s`, not `%s`", address, stringAddress)
+		}
+	}
+}
+
+func TestNewIpOptInvalidDefaultVal(t *testing.T) {
+	ip := net.IPv4(127, 0, 0, 1)
+	defaultVal := "Not an ip"
+
+	ipOpt := NewIpOpt(&ip, defaultVal)
+
+	expected := "127.0.0.1"
+	if ipOpt.String() != expected {
+		t.Fatalf("Expected [%v], got [%v]", expected, ipOpt.String())
+	}
+}
+
+func TestNewIpOptValidDefaultVal(t *testing.T) {
+	ip := net.IPv4(127, 0, 0, 1)
+	defaultVal := "192.168.1.1"
+
+	ipOpt := NewIpOpt(&ip, defaultVal)
+
+	expected := "192.168.1.1"
+	if ipOpt.String() != expected {
+		t.Fatalf("Expected [%v], got [%v]", expected, ipOpt.String())
+	}
+}
+
+func TestIpOptSetInvalidVal(t *testing.T) {
+	ip := net.IPv4(127, 0, 0, 1)
+	ipOpt := &IpOpt{IP: &ip}
+
+	invalidIp := "invalid ip"
+	expectedError := "invalid ip is not an ip address"
+	err := ipOpt.Set(invalidIp)
+	if err == nil || err.Error() != expectedError {
+		t.Fatalf("Expected an Error with [%v], got [%v]", expectedError, err.Error())
+	}
+}

opts/opts.go

@@ -11,61 +11,85 @@ import (
 	flag "github.com/docker/docker/pkg/mflag"
 	"github.com/docker/docker/pkg/parsers"
 	"github.com/docker/docker/pkg/ulimit"
+	"github.com/docker/docker/volume"
 )
 
 var (
-	alphaRegexp     = regexp.MustCompile(`[a-zA-Z]`)
+	alphaRegexp  = regexp.MustCompile(`[a-zA-Z]`)
-	domainRegexp    = regexp.MustCompile(`^(:?(:?[a-zA-Z0-9]|(:?[a-zA-Z0-9][a-zA-Z0-9\-]*[a-zA-Z0-9]))(:?\.(:?[a-zA-Z0-9]|(:?[a-zA-Z0-9][a-zA-Z0-9\-]*[a-zA-Z0-9])))*)\.?\s*$`)
+	domainRegexp = regexp.MustCompile(`^(:?(:?[a-zA-Z0-9]|(:?[a-zA-Z0-9][a-zA-Z0-9\-]*[a-zA-Z0-9]))(:?\.(:?[a-zA-Z0-9]|(:?[a-zA-Z0-9][a-zA-Z0-9\-]*[a-zA-Z0-9])))*)\.?\s*$`)
-	DefaultHTTPHost = "127.0.0.1" // Default HTTP Host used if only port is provided to -H flag e.g. docker -d -H tcp://:8080
+	// DefaultHTTPHost Default HTTP Host used if only port is provided to -H flag e.g. docker -d -H tcp://:8080
+	DefaultHTTPHost = "127.0.0.1"
+	// DefaultHTTPPort Default HTTP Port used if only the protocol is provided to -H flag e.g. docker -d -H tcp://
 	// TODO Windows. DefaultHTTPPort is only used on Windows if a -H parameter
 	// is not supplied. A better longer term solution would be to use a named
 	// pipe as the default on the Windows daemon.
-	DefaultHTTPPort   = 2375                   // Default HTTP Port
+	DefaultHTTPPort = 2375 // Default HTTP Port
-	DefaultUnixSocket = "/var/run/docker.sock" // Docker daemon by default always listens on the default unix socket
+	// DefaultUnixSocket Path for the unix socket.
+	// Docker daemon by default always listens on the default unix socket
+	DefaultUnixSocket = "/var/run/docker.sock"
 )
 
+// ListVar Defines a flag with the specified names and usage, and put the value
+// list into ListOpts that will hold the values.
 func ListVar(values *[]string, names []string, usage string) {
 	flag.Var(newListOptsRef(values, nil), names, usage)
 }
 
+// MapVar Defines a flag with the specified names and usage, and put the value
+// map into MapOpt that will hold the values (key,value).
 func MapVar(values map[string]string, names []string, usage string) {
 	flag.Var(newMapOpt(values, nil), names, usage)
 }
 
+// LogOptsVar Defines a flag with the specified names and usage for --log-opts,
+// and put the value map into MapOpt that will hold the values (key,value).
 func LogOptsVar(values map[string]string, names []string, usage string) {
 	flag.Var(newMapOpt(values, nil), names, usage)
 }
 
+// HostListVar Defines a flag with the specified names and usage and put the
+// value into a ListOpts that will hold the values, validating the Host format.
 func HostListVar(values *[]string, names []string, usage string) {
 	flag.Var(newListOptsRef(values, ValidateHost), names, usage)
 }
 
+// IPListVar Defines a flag with the specified names and usage and put the
+// value into a ListOpts that will hold the values, validating the IP format.
 func IPListVar(values *[]string, names []string, usage string) {
 	flag.Var(newListOptsRef(values, ValidateIPAddress), names, usage)
 }
 
-func DnsSearchListVar(values *[]string, names []string, usage string) {
+// DNSSearchListVar Defines a flag with the specified names and usage and put the
-	flag.Var(newListOptsRef(values, ValidateDnsSearch), names, usage)
+// value into a ListOpts that will hold the values, validating the DNS search format.
+func DNSSearchListVar(values *[]string, names []string, usage string) {
+	flag.Var(newListOptsRef(values, ValidateDNSSearch), names, usage)
 }
 
+// IPVar Defines a flag with the specified names and usage for IP and will use
+// the specified defaultValue if the specified value is not valid.
 func IPVar(value *net.IP, names []string, defaultValue, usage string) {
 	flag.Var(NewIpOpt(value, defaultValue), names, usage)
 }
 
+// LabelListVar Defines a flag with the specified names and usage and put the
+// value into a ListOpts that will hold the values, validating the label format.
 func LabelListVar(values *[]string, names []string, usage string) {
 	flag.Var(newListOptsRef(values, ValidateLabel), names, usage)
 }
 
+// UlimitMapVar Defines a flag with the specified names and usage for --ulimit,
+// and put the value map into a UlimitOpt that will hold the values.
 func UlimitMapVar(values map[string]*ulimit.Ulimit, names []string, usage string) {
 	flag.Var(NewUlimitOpt(values), names, usage)
 }
 
-// ListOpts type
+// ListOpts type that hold a list of values and a validation function.
 type ListOpts struct {
 	values    *[]string
 	validator ValidatorFctType
 }
 
+// NewListOpts Create a new ListOpts with the specified validator.
 func NewListOpts(validator ValidatorFctType) ListOpts {
 	var values []string
 	return *newListOptsRef(&values, validator)
@@ -138,12 +162,14 @@ func (opts *ListOpts) Len() int {
 	return len((*opts.values))
 }
 
-//MapOpts type
+//MapOpts type that holds a map of values and a validation function.
 type MapOpts struct {
 	values    map[string]string
 	validator ValidatorFctType
 }
 
+// Set validates if needed the input value and add it to the
+// internal map, by splitting on '='.
 func (opts *MapOpts) Set(value string) error {
 	if opts.validator != nil {
 		v, err := opts.validator(value)
@@ -172,10 +198,13 @@ func newMapOpt(values map[string]string, validator ValidatorFctType) *MapOpts {
 	}
 }
 
-// Validators
+// ValidatorFctType validator that return a validate string and/or an error
 type ValidatorFctType func(val string) (string, error)
+
+// ValidatorFctListType validator that return a validate list of string and/or an error
 type ValidatorFctListType func(val string) ([]string, error)
 
+// ValidateAttach Validates that the specified string is a valid attach option.
 func ValidateAttach(val string) (string, error) {
 	s := strings.ToLower(val)
 	for _, str := range []string{"stdin", "stdout", "stderr"} {
@@ -183,9 +212,10 @@ func ValidateAttach(val string) (string, error) {
 			return s, nil
 		}
 	}
-	return val, fmt.Errorf("valid streams are STDIN, STDOUT and STDERR.")
+	return val, fmt.Errorf("valid streams are STDIN, STDOUT and STDERR")
 }
 
+// ValidateLink Validates that the specified string has a valid link format (containerName:alias).
 func ValidateLink(val string) (string, error) {
 	if _, _, err := parsers.ParseLink(val); err != nil {
 		return val, err
@@ -193,22 +223,53 @@ func ValidateLink(val string) (string, error) {
 	return val, nil
 }
 
-// ValidatePath will make sure 'val' is in the form:
+// ValidateDevice Validate a path for devices
-//    [host-dir:]container-path[:rw|ro]  - but doesn't validate the mode part
+// It will make sure 'val' is in the form:
+//    [host-dir:]container-path[:mode]
+func ValidateDevice(val string) (string, error) {
+	return validatePath(val, false)
+}
+
+// ValidatePath Validate a path for volumes
+// It will make sure 'val' is in the form:
+//    [host-dir:]container-path[:rw|ro]
+// It will also validate the mount mode.
 func ValidatePath(val string) (string, error) {
+	return validatePath(val, true)
+}
+
+func validatePath(val string, validateMountMode bool) (string, error) {
 	var containerPath string
+	var mode string
 
 	if strings.Count(val, ":") > 2 {
 		return val, fmt.Errorf("bad format for volumes: %s", val)
 	}
 
-	splited := strings.SplitN(val, ":", 2)
+	splited := strings.SplitN(val, ":", 3)
-	if len(splited) == 1 {
+	if splited[0] == "" {
+		return val, fmt.Errorf("bad format for volumes: %s", val)
+	}
+	switch len(splited) {
+	case 1:
 		containerPath = splited[0]
-		val = path.Clean(splited[0])
+		val = path.Clean(containerPath)
-	} else {
+	case 2:
+		if isValid, _ := volume.ValidateMountMode(splited[1]); validateMountMode && isValid {
+			containerPath = splited[0]
+			mode = splited[1]
+			val = fmt.Sprintf("%s:%s", path.Clean(containerPath), mode)
+		} else {
+			containerPath = splited[1]
+			val = fmt.Sprintf("%s:%s", splited[0], path.Clean(containerPath))
+		}
+	case 3:
 		containerPath = splited[1]
-		val = fmt.Sprintf("%s:%s", splited[0], path.Clean(splited[1]))
+		mode = splited[2]
+		if isValid, _ := volume.ValidateMountMode(splited[2]); validateMountMode && !isValid {
+			return val, fmt.Errorf("bad mount mode specified : %s", mode)
+		}
+		val = fmt.Sprintf("%s:%s:%s", splited[0], containerPath, mode)
 	}
 
 	if !path.IsAbs(containerPath) {
@@ -217,17 +278,24 @@ func ValidatePath(val string) (string, error) {
 	return val, nil
 }
 
+// ValidateEnv Validate an environment variable and returns it
+// It will use EnvironmentVariableRegexp to ensure the name of the environment variable is valid.
+// If no value is specified, it returns the current value using os.Getenv.
 func ValidateEnv(val string) (string, error) {
 	arr := strings.Split(val, "=")
 	if len(arr) > 1 {
 		return val, nil
 	}
+	if !EnvironmentVariableRegexp.MatchString(arr[0]) {
+		return val, ErrBadEnvVariable{fmt.Sprintf("variable '%s' is not a valid environment variable", val)}
+	}
 	if !doesEnvExist(val) {
 		return val, nil
 	}
 	return fmt.Sprintf("%s=%s", val, os.Getenv(val)), nil
 }
 
+// ValidateIPAddress Validates an Ip address
 func ValidateIPAddress(val string) (string, error) {
 	var ip = net.ParseIP(strings.TrimSpace(val))
 	if ip != nil {
@@ -236,6 +304,7 @@ func ValidateIPAddress(val string) (string, error) {
 	return "", fmt.Errorf("%s is not an ip address", val)
 }
 
+// ValidateMACAddress Validates a MAC address
 func ValidateMACAddress(val string) (string, error) {
 	_, err := net.ParseMAC(strings.TrimSpace(val))
 	if err != nil {
@@ -244,9 +313,9 @@ func ValidateMACAddress(val string) (string, error) {
 	return val, nil
 }
 
-// Validates domain for resolvconf search configuration.
+// ValidateDNSSearch Validates domain for resolvconf search configuration.
 // A zero length domain is represented by .
-func ValidateDnsSearch(val string) (string, error) {
+func ValidateDNSSearch(val string) (string, error) {
 	if val = strings.Trim(val, " "); val == "." {
 		return val, nil
 	}
@@ -264,6 +333,8 @@ func validateDomain(val string) (string, error) {
 	return "", fmt.Errorf("%s is not a valid domain", val)
 }
 
+// ValidateExtraHost Validate that the given string is a valid extrahost and returns it
+// ExtraHost are in the form of name:ip where the ip has to be a valid ip (ipv4 or ipv6)
 func ValidateExtraHost(val string) (string, error) {
 	// allow for IPv6 addresses in extra hosts by only splitting on first ":"
 	arr := strings.SplitN(val, ":", 2)
@@ -276,13 +347,16 @@ func ValidateExtraHost(val string) (string, error) {
 	return val, nil
 }
 
+// ValidateLabel Validate that the given string is a valid label, and returns it
+// Labels are in the form on key=value
 func ValidateLabel(val string) (string, error) {
-	if strings.Count(val, "=") != 1 {
+	if strings.Count(val, "=") < 1 {
 		return "", fmt.Errorf("bad attribute format: %s", val)
 	}
 	return val, nil
 }
 
+// ValidateHost Validate that the given string is a valid host and returns it
 func ValidateHost(val string) (string, error) {
 	host, err := parsers.ParseHost(DefaultHTTPHost, DefaultUnixSocket, val)
 	if err != nil {

opts/opts_test.go

@@ -2,7 +2,7 @@ package opts
 
 import (
 	"fmt"
-	"net"
+	"os"
 	"strings"
 	"testing"
 )
@@ -69,7 +69,7 @@ func TestValidateMACAddress(t *testing.T) {
 	}
 }
 
-func TestListOpts(t *testing.T) {
+func TestListOptsWithoutValidator(t *testing.T) {
 	o := NewListOpts(nil)
 	o.Set("foo")
 	if o.String() != "[foo]" {
@@ -79,6 +79,10 @@ func TestListOpts(t *testing.T) {
 	if o.Len() != 2 {
 		t.Errorf("%d != 2", o.Len())
 	}
+	o.Set("bar")
+	if o.Len() != 3 {
+		t.Errorf("%d != 3", o.Len())
+	}
 	if !o.Get("bar") {
 		t.Error("o.Get(\"bar\") == false")
 	}
@@ -86,12 +90,48 @@ func TestListOpts(t *testing.T) {
 		t.Error("o.Get(\"baz\") == true")
 	}
 	o.Delete("foo")
-	if o.String() != "[bar]" {
+	if o.String() != "[bar bar]" {
-		t.Errorf("%s != [bar]", o.String())
+		t.Errorf("%s != [bar bar]", o.String())
+	}
+	listOpts := o.GetAll()
+	if len(listOpts) != 2 || listOpts[0] != "bar" || listOpts[1] != "bar" {
+		t.Errorf("Expected [[bar bar]], got [%v]", listOpts)
+	}
+	mapListOpts := o.GetMap()
+	if len(mapListOpts) != 1 {
+		t.Errorf("Expected [map[bar:{}]], got [%v]", mapListOpts)
+	}
+
+}
+
+func TestListOptsWithValidator(t *testing.T) {
+	// Re-using logOptsvalidator (used by MapOpts)
+	o := NewListOpts(logOptsValidator)
+	o.Set("foo")
+	if o.String() != "[]" {
+		t.Errorf("%s != []", o.String())
+	}
+	o.Set("foo=bar")
+	if o.String() != "[]" {
+		t.Errorf("%s != []", o.String())
+	}
+	o.Set("max-file=2")
+	if o.Len() != 1 {
+		t.Errorf("%d != 1", o.Len())
+	}
+	if !o.Get("max-file=2") {
+		t.Error("o.Get(\"max-file=2\") == false")
+	}
+	if o.Get("baz") {
+		t.Error("o.Get(\"baz\") == true")
+	}
+	o.Delete("max-file=2")
+	if o.String() != "[]" {
+		t.Errorf("%s != []", o.String())
 	}
 }
 
-func TestValidateDnsSearch(t *testing.T) {
+func TestValidateDNSSearch(t *testing.T) {
 	valid := []string{
 		`.`,
 		`a`,
@@ -136,14 +176,14 @@ func TestValidateDnsSearch(t *testing.T) {
 	}
 
 	for _, domain := range valid {
-		if ret, err := ValidateDnsSearch(domain); err != nil || ret == "" {
+		if ret, err := ValidateDNSSearch(domain); err != nil || ret == "" {
-			t.Fatalf("ValidateDnsSearch(`"+domain+"`) got %s %s", ret, err)
+			t.Fatalf("ValidateDNSSearch(`"+domain+"`) got %s %s", ret, err)
 		}
 	}
 
 	for _, domain := range invalid {
-		if ret, err := ValidateDnsSearch(domain); err == nil || ret != "" {
+		if ret, err := ValidateDNSSearch(domain); err == nil || ret != "" {
-			t.Fatalf("ValidateDnsSearch(`"+domain+"`) got %s %s", ret, err)
+			t.Fatalf("ValidateDNSSearch(`"+domain+"`) got %s %s", ret, err)
 		}
 	}
 }
@@ -180,14 +220,251 @@ func TestValidateExtraHosts(t *testing.T) {
 	}
 }
 
-func TestIpOptString(t *testing.T) {
+func TestValidateAttach(t *testing.T) {
-	addresses := []string{"", "0.0.0.0"}
+	valid := []string{
-	var ip net.IP
+		"stdin",
+		"stdout",
+		"stderr",
+		"STDIN",
+		"STDOUT",
+		"STDERR",
+	}
+	if _, err := ValidateAttach("invalid"); err == nil {
+		t.Fatalf("Expected error with [valid streams are STDIN, STDOUT and STDERR], got nothing")
+	}
 
-	for _, address := range addresses {
+	for _, attach := range valid {
-		stringAddress := NewIpOpt(&ip, address).String()
+		value, err := ValidateAttach(attach)
-		if stringAddress != address {
+		if err != nil {
-			t.Fatalf("IpOpt string should be `%s`, not `%s`", address, stringAddress)
+			t.Fatal(err)
+		}
+		if value != strings.ToLower(attach) {
+			t.Fatalf("Expected [%v], got [%v]", attach, value)
+		}
+	}
+}
+
+func TestValidateLink(t *testing.T) {
+	valid := []string{
+		"name",
+		"dcdfbe62ecd0:alias",
+		"7a67485460b7642516a4ad82ecefe7f57d0c4916f530561b71a50a3f9c4e33da",
+		"angry_torvalds:linus",
+	}
+	invalid := map[string]string{
+		"":               "empty string specified for links",
+		"too:much:of:it": "bad format for links: too:much:of:it",
+	}
+
+	for _, link := range valid {
+		if _, err := ValidateLink(link); err != nil {
+			t.Fatalf("ValidateLink(`%q`) should succeed: error %q", link, err)
+		}
+	}
+
+	for link, expectedError := range invalid {
+		if _, err := ValidateLink(link); err == nil {
+			t.Fatalf("ValidateLink(`%q`) should have failed validation", link)
+		} else {
+			if !strings.Contains(err.Error(), expectedError) {
+				t.Fatalf("ValidateLink(`%q`) error should contain %q", link, expectedError)
+			}
+		}
+	}
+}
+
+func TestValidatePath(t *testing.T) {
+	valid := []string{
+		"/home",
+		"/home:/home",
+		"/home:/something/else",
+		"/with space",
+		"/home:/with space",
+		"relative:/absolute-path",
+		"hostPath:/containerPath:ro",
+		"/hostPath:/containerPath:rw",
+		"/rw:/ro",
+		"/path:rw",
+		"/path:ro",
+		"/rw:rw",
+	}
+	invalid := map[string]string{
+		"":                "bad format for volumes: ",
+		"./":              "./ is not an absolute path",
+		"../":             "../ is not an absolute path",
+		"/:../":           "../ is not an absolute path",
+		"/:path":          "path is not an absolute path",
+		":":               "bad format for volumes: :",
+		"/tmp:":           " is not an absolute path",
+		":test":           "bad format for volumes: :test",
+		":/test":          "bad format for volumes: :/test",
+		"tmp:":            " is not an absolute path",
+		":test:":          "bad format for volumes: :test:",
+		"::":              "bad format for volumes: ::",
+		":::":             "bad format for volumes: :::",
+		"/tmp:::":         "bad format for volumes: /tmp:::",
+		":/tmp::":         "bad format for volumes: :/tmp::",
+		"path:ro":         "path is not an absolute path",
+		"/path:/path:sw":  "bad mount mode specified : sw",
+		"/path:/path:rwz": "bad mount mode specified : rwz",
+	}
+
+	for _, path := range valid {
+		if _, err := ValidatePath(path); err != nil {
+			t.Fatalf("ValidatePath(`%q`) should succeed: error %q", path, err)
+		}
+	}
+
+	for path, expectedError := range invalid {
+		if _, err := ValidatePath(path); err == nil {
+			t.Fatalf("ValidatePath(`%q`) should have failed validation", path)
+		} else {
+			if err.Error() != expectedError {
+				t.Fatalf("ValidatePath(`%q`) error should contain %q, got %q", path, expectedError, err.Error())
+			}
+		}
+	}
+}
+func TestValidateDevice(t *testing.T) {
+	valid := []string{
+		"/home",
+		"/home:/home",
+		"/home:/something/else",
+		"/with space",
+		"/home:/with space",
+		"relative:/absolute-path",
+		"hostPath:/containerPath:ro",
+		"/hostPath:/containerPath:rw",
+		"/hostPath:/containerPath:mrw",
+	}
+	invalid := map[string]string{
+		"":        "bad format for volumes: ",
+		"./":      "./ is not an absolute path",
+		"../":     "../ is not an absolute path",
+		"/:../":   "../ is not an absolute path",
+		"/:path":  "path is not an absolute path",
+		":":       "bad format for volumes: :",
+		"/tmp:":   " is not an absolute path",
+		":test":   "bad format for volumes: :test",
+		":/test":  "bad format for volumes: :/test",
+		"tmp:":    " is not an absolute path",
+		":test:":  "bad format for volumes: :test:",
+		"::":      "bad format for volumes: ::",
+		":::":     "bad format for volumes: :::",
+		"/tmp:::": "bad format for volumes: /tmp:::",
+		":/tmp::": "bad format for volumes: :/tmp::",
+		"path:ro": "ro is not an absolute path",
+	}
+
+	for _, path := range valid {
+		if _, err := ValidateDevice(path); err != nil {
+			t.Fatalf("ValidateDevice(`%q`) should succeed: error %q", path, err)
+		}
+	}
+
+	for path, expectedError := range invalid {
+		if _, err := ValidateDevice(path); err == nil {
+			t.Fatalf("ValidateDevice(`%q`) should have failed validation", path)
+		} else {
+			if err.Error() != expectedError {
+				t.Fatalf("ValidateDevice(`%q`) error should contain %q, got %q", path, expectedError, err.Error())
+			}
+		}
+	}
+}
+
+func TestValidateEnv(t *testing.T) {
+	invalids := map[string]string{
+		"some spaces": "poorly formatted environment: variable 'some spaces' is not a valid environment variable",
+		"asd!qwe":     "poorly formatted environment: variable 'asd!qwe' is not a valid environment variable",
+		"1asd":        "poorly formatted environment: variable '1asd' is not a valid environment variable",
+		"123":         "poorly formatted environment: variable '123' is not a valid environment variable",
+	}
+	valids := map[string]string{
+		"a":                  "a",
+		"something":          "something",
+		"_=a":                "_=a",
+		"env1=value1":        "env1=value1",
+		"_env1=value1":       "_env1=value1",
+		"env2=value2=value3": "env2=value2=value3",
+		"env3=abc!qwe":       "env3=abc!qwe",
+		"env_4=value 4":      "env_4=value 4",
+		"PATH":               fmt.Sprintf("PATH=%v", os.Getenv("PATH")),
+		"PATH=something":     "PATH=something",
+	}
+	for value, expectedError := range invalids {
+		_, err := ValidateEnv(value)
+		if err == nil {
+			t.Fatalf("Expected ErrBadEnvVariable, got nothing")
+		}
+		if _, ok := err.(ErrBadEnvVariable); !ok {
+			t.Fatalf("Expected ErrBadEnvVariable, got [%s]", err)
+		}
+		if err.Error() != expectedError {
+			t.Fatalf("Expected ErrBadEnvVariable with message [%s], got [%s]", expectedError, err.Error())
+		}
+	}
+	for value, expected := range valids {
+		actual, err := ValidateEnv(value)
+		if err != nil {
+			t.Fatal(err)
+		}
+		if actual != expected {
+			t.Fatalf("Expected [%v], got [%v]", expected, actual)
+		}
+	}
+}
+
+func TestValidateLabel(t *testing.T) {
+	if _, err := ValidateLabel("label"); err == nil || err.Error() != "bad attribute format: label" {
+		t.Fatalf("Expected an error [bad attribute format: label], go %v", err)
+	}
+	if actual, err := ValidateLabel("key1=value1"); err != nil || actual != "key1=value1" {
+		t.Fatalf("Expected [key1=value1], got [%v,%v]", actual, err)
+	}
+	// Validate it's working with more than one =
+	if actual, err := ValidateLabel("key1=value1=value2"); err != nil {
+		t.Fatalf("Expected [key1=value1=value2], got [%v,%v]", actual, err)
+	}
+	// Validate it's working with one more
+	if actual, err := ValidateLabel("key1=value1=value2=value3"); err != nil {
+		t.Fatalf("Expected [key1=value1=value2=value2], got [%v,%v]", actual, err)
+	}
+}
+
+func TestValidateHost(t *testing.T) {
+	invalid := map[string]string{
+		"anything":              "Invalid bind address format: anything",
+		"something with spaces": "Invalid bind address format: something with spaces",
+		"://":                "Invalid bind address format: ://",
+		"unknown://":         "Invalid bind address format: unknown://",
+		"tcp://":             "Invalid proto, expected tcp: ",
+		"tcp://:port":        "Invalid bind address format: :port",
+		"tcp://invalid":      "Invalid bind address format: invalid",
+		"tcp://invalid:port": "Invalid bind address format: invalid:port",
+	}
+	valid := map[string]string{
+		"fd://":                    "fd://",
+		"fd://something":           "fd://something",
+		"tcp://:2375":              "tcp://127.0.0.1:2375", // default ip address
+		"tcp://:2376":              "tcp://127.0.0.1:2376", // default ip address
+		"tcp://0.0.0.0:8080":       "tcp://0.0.0.0:8080",
+		"tcp://192.168.0.0:12000":  "tcp://192.168.0.0:12000",
+		"tcp://192.168:8080":       "tcp://192.168:8080",
+		"tcp://0.0.0.0:1234567890": "tcp://0.0.0.0:1234567890", // yeah it's valid :P
+		"tcp://docker.com:2375":    "tcp://docker.com:2375",
+		"unix://":                  "unix:///var/run/docker.sock", // default unix:// value
+		"unix://path/to/socket":    "unix://path/to/socket",
+	}
+
+	for value, errorMessage := range invalid {
+		if _, err := ValidateHost(value); err == nil || err.Error() != errorMessage {
+			t.Fatalf("Expected an error for %v with [%v], got [%v]", value, errorMessage, err)
+		}
+	}
+	for value, expected := range valid {
+		if actual, err := ValidateHost(value); err != nil || actual != expected {
+			t.Fatalf("Expected for %v [%v], got [%v, %v]", value, expected, actual, err)
 		}
 	}
 }

opts/ulimit_test.go

@@ -0,0 +1,42 @@
+package opts
+
+import (
+	"testing"
+
+	"github.com/docker/docker/pkg/ulimit"
+)
+
+func TestUlimitOpt(t *testing.T) {
+	ulimitMap := map[string]*ulimit.Ulimit{
+		"nofile": {"nofile", 1024, 512},
+	}
+
+	ulimitOpt := NewUlimitOpt(ulimitMap)
+
+	expected := "[nofile=512:1024]"
+	if ulimitOpt.String() != expected {
+		t.Fatalf("Expected %v, got %v", expected, ulimitOpt)
+	}
+
+	// Valid ulimit append to opts
+	if err := ulimitOpt.Set("core=1024:1024"); err != nil {
+		t.Fatal(err)
+	}
+
+	// Invalid ulimit type returns an error and do not append to opts
+	if err := ulimitOpt.Set("notavalidtype=1024:1024"); err == nil {
+		t.Fatalf("Expected error on invalid ulimit type")
+	}
+	expected = "[nofile=512:1024 core=1024:1024]"
+	expected2 := "[core=1024:1024 nofile=512:1024]"
+	result := ulimitOpt.String()
+	if result != expected && result != expected2 {
+		t.Fatalf("Expected %v or %v, got %v", expected, expected2, ulimitOpt)
+	}
+
+	// And test GetList
+	ulimits := ulimitOpt.GetList()
+	if len(ulimits) != 2 {
+		t.Fatalf("Expected a ulimit list of 2, got %v", ulimits)
+	}
+}

runconfig/parse.go

@@ -45,7 +45,7 @@ func Parse(cmd *flag.FlagSet, args []string) (*Config, *HostConfig, *flag.FlagSe
 		flLinks   = opts.NewListOpts(opts.ValidateLink)
 		flEnv     = opts.NewListOpts(opts.ValidateEnv)
 		flLabels  = opts.NewListOpts(opts.ValidateEnv)
-		flDevices = opts.NewListOpts(opts.ValidatePath)
+		flDevices = opts.NewListOpts(opts.ValidateDevice)
 
 		ulimits   = make(map[string]*ulimit.Ulimit)
 		flUlimits = opts.NewUlimitOpt(ulimits)
@@ -53,7 +53,7 @@ func Parse(cmd *flag.FlagSet, args []string) (*Config, *HostConfig, *flag.FlagSe
 		flPublish     = opts.NewListOpts(nil)
 		flExpose      = opts.NewListOpts(nil)
 		flDns         = opts.NewListOpts(opts.ValidateIPAddress)
-		flDnsSearch   = opts.NewListOpts(opts.ValidateDnsSearch)
+		flDnsSearch   = opts.NewListOpts(opts.ValidateDNSSearch)
 		flExtraHosts  = opts.NewListOpts(opts.ValidateExtraHost)
 		flVolumesFrom = opts.NewListOpts(nil)
 		flLxcOpts     = opts.NewListOpts(nil)

runconfig/parse_test.go

@@ -124,8 +124,12 @@ func TestParseRunVolumes(t *testing.T) {
 		t.Fatalf("Error parsing volume flags, `-v /hostTmp:/containerTmp:ro -v /hostVar:/containerVar:rw` should mount-bind /hostTmp into /containeTmp and /hostVar into /hostContainer. Received %v", hostConfig.Binds)
 	}
 
-	if _, hostConfig := mustParse(t, "-v /hostTmp:/containerTmp:roZ -v /hostVar:/containerVar:rwZ"); hostConfig.Binds == nil || compareRandomizedStrings(hostConfig.Binds[0], hostConfig.Binds[1], "/hostTmp:/containerTmp:roZ", "/hostVar:/containerVar:rwZ") != nil {
+	if _, hostConfig := mustParse(t, "-v /containerTmp:ro -v /containerVar:rw"); hostConfig.Binds == nil || compareRandomizedStrings(hostConfig.Binds[0], hostConfig.Binds[1], "/containerTmp:ro", "/containerVar:rw") != nil {
-		t.Fatalf("Error parsing volume flags, `-v /hostTmp:/containerTmp:roZ -v /hostVar:/containerVar:rwZ` should mount-bind /hostTmp into /containeTmp and /hostVar into /hostContainer. Received %v", hostConfig.Binds)
+		t.Fatalf("Error parsing volume flags, `-v /hostTmp:/containerTmp:ro -v /hostVar:/containerVar:rw` should mount-bind /hostTmp into /containeTmp and /hostVar into /hostContainer. Received %v", hostConfig.Binds)
+	}
+
+	if _, hostConfig := mustParse(t, "-v /hostTmp:/containerTmp:ro,Z -v /hostVar:/containerVar:rw,Z"); hostConfig.Binds == nil || compareRandomizedStrings(hostConfig.Binds[0], hostConfig.Binds[1], "/hostTmp:/containerTmp:ro,Z", "/hostVar:/containerVar:rw,Z") != nil {
+		t.Fatalf("Error parsing volume flags, `-v /hostTmp:/containerTmp:ro,Z -v /hostVar:/containerVar:rw,Z` should mount-bind /hostTmp into /containeTmp and /hostVar into /hostContainer. Received %v", hostConfig.Binds)
 	}
 
 	if _, hostConfig := mustParse(t, "-v /hostTmp:/containerTmp:Z -v /hostVar:/containerVar:z"); hostConfig.Binds == nil || compareRandomizedStrings(hostConfig.Binds[0], hostConfig.Binds[1], "/hostTmp:/containerTmp:Z", "/hostVar:/containerVar:z") != nil {
@@ -157,9 +161,6 @@ func TestParseRunVolumes(t *testing.T) {
 	if _, _, err := parse(t, "-v /tmp:"); err == nil {
 		t.Fatalf("Error parsing volume flags, `-v /tmp:` should fail but didn't")
 	}
-	if _, _, err := parse(t, "-v /tmp:ro"); err == nil {
-		t.Fatalf("Error parsing volume flags, `-v /tmp:ro` should fail but didn't")
-	}
 	if _, _, err := parse(t, "-v /tmp::"); err == nil {
 		t.Fatalf("Error parsing volume flags, `-v /tmp::` should fail but didn't")
 	}

volume/volume.go

@@ -24,3 +24,29 @@ type Volume interface {
 	// Unmount unmounts the volume when it is no longer in use.
 	Unmount() error
 }
+
+// read-write modes
+var rwModes = map[string]bool{
+	"rw":   true,
+	"rw,Z": true,
+	"rw,z": true,
+	"z,rw": true,
+	"Z,rw": true,
+	"Z":    true,
+	"z":    true,
+}
+
+// read-only modes
+var roModes = map[string]bool{
+	"ro":   true,
+	"ro,Z": true,
+	"ro,z": true,
+	"z,ro": true,
+	"Z,ro": true,
+}
+
+// ValidateMountMode will make sure the mount mode is valid.
+// returns if it's a valid mount mode and if it's read-write or not.
+func ValidateMountMode(mode string) (bool, bool) {
+	return roModes[mode] || rwModes[mode], rwModes[mode]
+}