Cleanup: initializeVolume

- Use a common struct for Volumes
- Split out some functionality in intializeVolume into separate functions
- Removes some duplicate code
- In general much easier to grok the code now

Docker-DCO-1.1-Signed-off-by: Brian Goff <cpuguy83@gmail.com> (github: cpuguy83)

daemon/volumes.go

@@ -13,10 +13,24 @@ import (
 	"github.com/docker/docker/pkg/symlink"
 )
 
-type BindMap struct {
+type Volume struct {
-	SrcPath string
+	HostPath    string
-	DstPath string
+	VolPath     string
-	Mode    string
+	Mode        string
+	isBindMount bool
+}
+
+func (v *Volume) isRw() bool {
+	return v.Mode == "" || strings.ToLower(v.Mode) == "rw"
+}
+
+func (v *Volume) isDir() (bool, error) {
+	stat, err := os.Stat(v.HostPath)
+	if err != nil {
+		return false, err
+	}
+
+	return stat.IsDir(), nil
 }
 
 func prepareVolumesForContainer(container *Container) error {
@@ -122,35 +136,40 @@ func applyVolumesFrom(container *Container) error {
 	return nil
 }
 
-func parseBindVolumeSpec(spec string) (BindMap, error) {
+func parseBindVolumeSpec(spec string) (Volume, error) {
 	var (
-		arr       = strings.Split(spec, ":")
+		arr = strings.Split(spec, ":")
-		err error = nil
+		vol Volume
-		vol BindMap
 	)
 
+	vol.isBindMount = true
 	switch len(arr) {
 	case 1:
-		vol.DstPath = spec
+		vol.VolPath = spec
 		vol.Mode = "rw"
 	case 2:
-		vol.SrcPath = arr[0]
+		vol.HostPath = arr[0]
-		vol.DstPath = arr[1]
+		vol.VolPath = arr[1]
 		vol.Mode = "rw"
 	case 3:
-		vol.SrcPath = arr[0]
+		vol.HostPath = arr[0]
-		vol.DstPath = arr[1]
+		vol.VolPath = arr[1]
 		vol.Mode = arr[2]
 	default:
-		err = fmt.Errorf("Invalid volume specification: %s", spec)
+		return vol, fmt.Errorf("Invalid volume specification: %s", spec)
 	}
-	return vol, err
+
+	if !filepath.IsAbs(vol.HostPath) {
+		return vol, fmt.Errorf("cannot bind mount volume: %s volume paths must be absolute.", vol.HostPath)
+	}
+
+	return vol, nil
 }
 
-func getBindMap(container *Container) (map[string]BindMap, error) {
+func getBindMap(container *Container) (map[string]Volume, error) {
 	var (
 		// Create the requested bind mounts
-		binds = make(map[string]BindMap)
+		volumes = map[string]Volume{}
 		// Define illegal container destinations
 		illegalDsts = []string{"/", "."}
 	)
@@ -158,151 +177,134 @@ func getBindMap(container *Container) (map[string]BindMap, error) {
 	for _, bind := range container.hostConfig.Binds {
 		vol, err := parseBindVolumeSpec(bind)
 		if err != nil {
-			return binds, err
+			return volumes, err
 		}
 		// Bail if trying to mount to an illegal destination
 		for _, illegal := range illegalDsts {
-			if vol.DstPath == illegal {
+			if vol.VolPath == illegal {
-				return nil, fmt.Errorf("Illegal bind destination: %s", vol.DstPath)
+				return nil, fmt.Errorf("Illegal bind destination: %s", vol.VolPath)
 			}
 		}
 
-		binds[filepath.Clean(vol.DstPath)] = vol
+		volumes[filepath.Clean(vol.VolPath)] = vol
 	}
-	return binds, nil
+	return volumes, nil
 }
 
 func createVolumes(container *Container) error {
-	binds, err := getBindMap(container)
+	// Get all the bindmounts
+	volumes, err := getBindMap(container)
 	if err != nil {
 		return err
 	}
 
-	// Create the requested volumes if they don't exist
+	// Get all the rest of the volumes
 	for volPath := range container.Config.Volumes {
-		if err := initializeVolume(container, volPath, binds); err != nil {
+		// Make sure the the volume isn't already specified as a bindmount
+		if _, exists := volumes[volPath]; !exists {
+			volumes[volPath] = Volume{
+				VolPath:     volPath,
+				Mode:        "rw",
+				isBindMount: false,
+			}
+		}
+	}
+
+	for _, vol := range volumes {
+		if err = vol.initialize(container); err != nil {
+			return err
+		}
+	}
+	return nil
+
+}
+
+func createVolumeHostPath(container *Container) (string, error) {
+	volumesDriver := container.daemon.volumes.Driver()
+
+	// Do not pass a container as the parameter for the volume creation.
+	// The graph driver using the container's information ( Image ) to
+	// create the parent.
+	c, err := container.daemon.volumes.Create(nil, "", "", "", "", nil, nil)
+	if err != nil {
+		return "", err
+	}
+	hostPath, err := volumesDriver.Get(c.ID, "")
+	if err != nil {
+		return hostPath, fmt.Errorf("Driver %s failed to get volume rootfs %s: %s", volumesDriver, c.ID, err)
+	}
+
+	return hostPath, nil
+}
+
+func (v *Volume) initialize(container *Container) error {
+	var err error
+	v.VolPath = filepath.Clean(v.VolPath)
+
+	// Do not initialize an existing volume
+	if _, exists := container.Volumes[v.VolPath]; exists {
+		return nil
+	}
+
+	// If it's not a bindmount we need to create the dir on the host
+	if !v.isBindMount {
+		v.HostPath, err = createVolumeHostPath(container)
+		if err != nil {
 			return err
 		}
 	}
 
-	for volPath := range binds {
+	hostPath, err := filepath.EvalSymlinks(v.HostPath)
-		if err := initializeVolume(container, volPath, binds); err != nil {
+	if err != nil {
-			return err
+		return err
-		}
+	}
+
+	// Create the mountpoint
+	// This is the path to the volume within the container FS
+	// This differs from `hostPath` in that `hostPath` refers to the place where
+	// the volume data is actually stored on the host
+	fullVolPath, err := symlink.FollowSymlinkInScope(filepath.Join(container.basefs, v.VolPath), container.basefs)
+	if err != nil {
+		return err
+	}
+
+	container.Volumes[v.VolPath] = hostPath
+	container.VolumesRW[v.VolPath] = v.isRw()
+
+	volIsDir, err := v.isDir()
+	if err != nil {
+		return err
+	}
+	if err := createIfNotExists(fullVolPath, volIsDir); err != nil {
+		return err
+	}
+
+	// Do not copy or change permissions if we are mounting from the host
+	if v.isRw() && !v.isBindMount {
+		return copyExistingContents(fullVolPath, hostPath)
 	}
 	return nil
 }
 
 func createIfNotExists(destination string, isDir bool) error {
-	if _, err := os.Stat(destination); err != nil && os.IsNotExist(err) {
+	if _, err := os.Stat(destination); err == nil || !os.IsNotExist(err) {
-		if isDir {
-			if err := os.MkdirAll(destination, 0755); err != nil {
-				return err
-			}
-		} else {
-			if err := os.MkdirAll(filepath.Dir(destination), 0755); err != nil {
-				return err
-			}
-
-			f, err := os.OpenFile(destination, os.O_CREATE, 0755)
-			if err != nil {
-				return err
-			}
-			f.Close()
-		}
-	}
-
-	return nil
-}
-
-func initializeVolume(container *Container, volPath string, binds map[string]BindMap) error {
-	volumesDriver := container.daemon.volumes.Driver()
-	volPath = filepath.Clean(volPath)
-
-	// Skip existing volumes
-	if _, exists := container.Volumes[volPath]; exists {
 		return nil
 	}
 
-	var (
+	if isDir {
-		destination string
+		return os.MkdirAll(destination, 0755)
-		isBindMount bool
-		volIsDir    = true
-
-		srcRW = false
-	)
-
-	// If an external bind is defined for this volume, use that as a source
-	if bindMap, exists := binds[volPath]; exists {
-		isBindMount = true
-		destination = bindMap.SrcPath
-
-		if !filepath.IsAbs(destination) {
-			return fmt.Errorf("cannot bind mount volume: %s volume paths must be absolute.", destination)
-		}
-
-		if strings.ToLower(bindMap.Mode) == "rw" {
-			srcRW = true
-		}
-
-		if stat, err := os.Stat(bindMap.SrcPath); err != nil {
-			return err
-		} else {
-			volIsDir = stat.IsDir()
-		}
-	} else {
-		// Do not pass a container as the parameter for the volume creation.
-		// The graph driver using the container's information ( Image ) to
-		// create the parent.
-		c, err := container.daemon.volumes.Create(nil, "", "", "", "", nil, nil)
-		if err != nil {
-			return err
-		}
-
-		destination, err = volumesDriver.Get(c.ID, "")
-		if err != nil {
-			return fmt.Errorf("Driver %s failed to get volume rootfs %s: %s", volumesDriver, c.ID, err)
-		}
-
-		srcRW = true
 	}
 
-	if p, err := filepath.EvalSymlinks(destination); err != nil {
+	if err := os.MkdirAll(filepath.Dir(destination), 0755); err != nil {
 		return err
-	} else {
-		destination = p
 	}
 
-	// Create the mountpoint
+	f, err := os.OpenFile(destination, os.O_CREATE, 0755)
-	source, err := symlink.FollowSymlinkInScope(filepath.Join(container.basefs, volPath), container.basefs)
 	if err != nil {
 		return err
 	}
+	f.Close()
 
-	newVolPath, err := filepath.Rel(container.basefs, source)
-	if err != nil {
-		return err
-	}
-	newVolPath = "/" + newVolPath
-
-	if volPath != newVolPath {
-		delete(container.Volumes, volPath)
-		delete(container.VolumesRW, volPath)
-	}
-
-	container.Volumes[volPath] = destination
-	container.VolumesRW[volPath] = srcRW
-
-	if err := createIfNotExists(source, volIsDir); err != nil {
-		return err
-	}
-
-	// Do not copy or change permissions if we are mounting from the host
-	if srcRW && !isBindMount {
-		if err := copyExistingContents(source, destination); err != nil {
-			return err
-		}
-	}
 	return nil
 }