Repository: moby/moby
File: daemon/container.go (Go)

Code before:
	}
	if err := translateWorkingDir(config); err != nil {
		return err
	}
	if len(config.StopSignal) > 0 {
		if _, err := signal.ParseSignal(config.StopSignal); err != nil {
			return err
		}
	}
	// Validate if Env contains empty variable or not (e.g., ``, `=foo`)
	for _, env := range config.Env {
		if _, err := opts.ValidateEnv(env); err != nil {
			return err
		}
	}
	return validateHealthCheck(config.Healthcheck)
}

func validateHostConfig(hostConfig *containertypes.HostConfig) ([]string, error) {
	if hostConfig == nil {
		return nil, nil
	}

	var warnings []string
	if hostConfig.AutoRemove && !hostConfig.RestartPolicy.IsNone() {
		return nil, errors.Errorf("can't create 'AutoRemove' container with restart policy")
	}
	// Validate mounts; check if host directories still exist
	parser := volumemounts.NewParser()
	for _, c := range hostConfig.Mounts {
		cfg := c
		if err := parser.ValidateMountConfig(&cfg); err != nil {
			return warnings, err
		}
	}
	for _, extraHost := range hostConfig.ExtraHosts {
		if _, err := opts.ValidateExtraHost(extraHost); err != nil {
			return warnings, err
		}
	}
	if err := validatePortBindings(hostConfig.PortBindings); err != nil {
		return warnings, err
	}
	if err := containertypes.ValidateRestartPolicy(hostConfig.RestartPolicy); err != nil {
		return warnings, err
	}
	if err := validateCapabilities(hostConfig); err != nil {
		return warnings, err
	}
	if !hostConfig.Isolation.IsValid() {
		return warnings, errors.Errorf("invalid isolation '%s' on %s", hostConfig.Isolation, runtime.GOOS)

Review comment (suggestion):
I'm slightly on the fence what's more robust here; usually, I'm a big fan of being explicit (return `nil` if there's nothing to return). However, in this case, the `warnings` array is something that's aggregated / added to in multiple places. Returning `nil` here makes it error-prone for situations where (e.g.) another check is added further up that appends to `warnings` (which we would discard if we return `nil` here).

So _probably_ returning `warnings` here is the right thing to do (i..e., always return whatever we collected until now, which may be "nothing")

```go
return warnings, errors.Errorf("can't create 'AutoRemove' container with restart policy")
```


Code after:
	if len(config.StopSignal) > 0 {
		if _, err := signal.ParseSignal(config.StopSignal); err != nil {
			return err
		}
	}
	// Validate if Env contains empty variable or not (e.g., ``, `=foo`)
	for _, env := range config.Env {
		if _, err := opts.ValidateEnv(env); err != nil {
			return err
		}
	}
	return validateHealthCheck(config.Healthcheck)
}

func validateHostConfig(hostConfig *containertypes.HostConfig) (warnings []string, _ error) {
	if hostConfig == nil {
		return nil, nil
	}

	if hostConfig.AutoRemove && !hostConfig.RestartPolicy.IsNone() {
		return warnings, errors.Errorf("can't create 'AutoRemove' container with restart policy")
	}
	// Validate mounts; check if host directories still exist
	parser := volumemounts.NewParser()
	for _, c := range hostConfig.Mounts {
		cfg := c

		if cfg.Type == mount.TypeImage {
			warnings = append(warnings, "Image mount is an experimental feature")
		}

		if err := parser.ValidateMountConfig(&cfg); err != nil {
			return warnings, err
		}
	}
	for _, extraHost := range hostConfig.ExtraHosts {
		if _, err := opts.ValidateExtraHost(extraHost); err != nil {
			return warnings, err
		}
	}
	if err := validatePortBindings(hostConfig.PortBindings); err != nil {
		return warnings, err
	}
	if err := containertypes.ValidateRestartPolicy(hostConfig.RestartPolicy); err != nil {
		return warnings, err
	}
	if err := validateCapabilities(hostConfig); err != nil {
		return warnings, err
	}
	if !hostConfig.Isolation.IsValid() {
		return warnings, errors.Errorf("invalid isolation '%s' on %s", hostConfig.Isolation, runtime.GOOS)