Merge pull request #629 from kolyshkin/mkdirAll

Fix MkdirAll* and its usage
containers · May 18, 2020 · 16ff38b · 16ff38b
2 parents ca24bf0 + 5124860
commit 16ff38b
Show file tree

Hide file tree

Showing 6 changed files with 26 additions and 20 deletions.
diff --git a/drivers/devmapper/deviceset.go b/drivers/devmapper/deviceset.go
@@ -272,7 +272,7 @@ func (devices *DeviceSet) ensureImage(name string, size int64) (string, error) {
 	if err != nil {
 		return "", err
 	}
-	if err := idtools.MkdirAllAs(dirname, 0700, uid, gid); err != nil && !os.IsExist(err) {
+	if err := idtools.MkdirAllAs(dirname, 0700, uid, gid); err != nil {
 		return "", err
 	}
 
@@ -1701,10 +1701,10 @@ func (devices *DeviceSet) initDevmapper(doInit bool) (retErr error) {
 	if err != nil {
 		return err
 	}
-	if err := idtools.MkdirAs(devices.root, 0700, uid, gid); err != nil && !os.IsExist(err) {
+	if err := idtools.MkdirAs(devices.root, 0700, uid, gid); err != nil {
 		return err
 	}
-	if err := os.MkdirAll(devices.metadataDir(), 0700); err != nil && !os.IsExist(err) {
+	if err := os.MkdirAll(devices.metadataDir(), 0700); err != nil {
 		return err
 	}
 

diff --git a/drivers/devmapper/driver.go b/drivers/devmapper/driver.go
@@ -183,7 +183,7 @@ func (d *Driver) Get(id string, options graphdriver.MountOpts) (string, error) {
 	}
 
 	// Create the target directories if they don't exist
-	if err := idtools.MkdirAllAs(path.Join(d.home, "mnt"), 0755, uid, gid); err != nil && !os.IsExist(err) {
+	if err := idtools.MkdirAllAs(path.Join(d.home, "mnt"), 0755, uid, gid); err != nil {
 		d.ctr.Decrement(mp)
 		return "", err
 	}
@@ -198,7 +198,7 @@ func (d *Driver) Get(id string, options graphdriver.MountOpts) (string, error) {
 		return "", err
 	}
 
-	if err := idtools.MkdirAllAs(rootFs, 0755, uid, gid); err != nil && !os.IsExist(err) {
+	if err := idtools.MkdirAllAs(rootFs, 0755, uid, gid); err != nil {
 		d.ctr.Decrement(mp)
 		d.DeviceSet.UnmountDevice(id, mp)
 		return "", err

diff --git a/drivers/overlay/overlay.go b/drivers/overlay/overlay.go
@@ -152,11 +152,11 @@ func Init(home string, options graphdriver.Options) (graphdriver.Driver, error)
 	}
 
 	// Create the driver home dir
-	if err := idtools.MkdirAllAs(path.Join(home, linkDir), 0700, rootUID, rootGID); err != nil && !os.IsExist(err) {
+	if err := idtools.MkdirAllAs(path.Join(home, linkDir), 0700, rootUID, rootGID); err != nil {
 		return nil, err
 	}
 	runhome := filepath.Join(options.RunRoot, filepath.Base(home))
-	if err := idtools.MkdirAllAs(runhome, 0700, rootUID, rootGID); err != nil && !os.IsExist(err) {
+	if err := idtools.MkdirAllAs(runhome, 0700, rootUID, rootGID); err != nil {
 		return nil, err
 	}
 
@@ -555,7 +555,7 @@ func (d *Driver) create(id, parent string, opts *graphdriver.CreateOpts) (retErr
 		return err
 	}
 	// Make the link directory if it does not exist
-	if err := idtools.MkdirAllAs(path.Join(d.home, linkDir), 0700, rootUID, rootGID); err != nil && !os.IsExist(err) {
+	if err := idtools.MkdirAllAs(path.Join(d.home, linkDir), 0700, rootUID, rootGID); err != nil {
 		return err
 	}
 	if err := idtools.MkdirAllAs(path.Dir(dir), 0700, rootUID, rootGID); err != nil {
@@ -767,7 +767,7 @@ func (d *Driver) recreateSymlinks() error {
 	if err != nil {
 		return err
 	}
-	if err := idtools.MkdirAllAs(path.Join(d.home, linkDir), 0700, rootUID, rootGID); err != nil && !os.IsExist(err) {
+	if err := idtools.MkdirAllAs(path.Join(d.home, linkDir), 0700, rootUID, rootGID); err != nil {
 		return err
 	}
 	for _, dir := range dirs {
@@ -911,7 +911,7 @@ func (d *Driver) get(id string, disableShifting bool, options graphdriver.MountO
 		return "", err
 	}
 	diffDir := path.Join(dir, "diff")
-	if err := idtools.MkdirAllAs(diffDir, 0755, rootUID, rootGID); err != nil && !os.IsExist(err) {
+	if err := idtools.MkdirAllAs(diffDir, 0755, rootUID, rootGID); err != nil {
 		return "", err
 	}
 

diff --git a/pkg/idtools/idtools_unix.go b/pkg/idtools/idtools_unix.go
@@ -10,6 +10,7 @@ import (
 	"path/filepath"
 	"strings"
 	"sync"
+	"syscall"
 
 	"github.com/containers/storage/pkg/system"
 	"github.com/opencontainers/runc/libcontainer/user"
@@ -26,13 +27,18 @@ func mkdirAs(path string, mode os.FileMode, ownerUID, ownerGID int, mkAll, chown
 	// so that we can chown all of them properly at the end.  If chownExisting is false, we won't
 	// chown the full directory path if it exists
 	var paths []string
-	if _, err := os.Stat(path); err != nil && os.IsNotExist(err) {
+	st, err := os.Stat(path)
+	if err != nil && os.IsNotExist(err) {
 		paths = []string{path}
-	} else if err == nil && chownExisting {
-		// short-circuit--we were called with an existing directory and chown was requested
-		return SafeChown(path, ownerUID, ownerGID)
 	} else if err == nil {
-		// nothing to do; directory path fully exists already and chown was NOT requested
+		if !st.IsDir() {
+			return &os.PathError{Op: "mkdir", Path: path, Err: syscall.ENOTDIR}
+		}
+		if chownExisting {
+			// short-circuit--we were called with an existing directory and chown was requested
+			return SafeChown(path, ownerUID, ownerGID)
+		}
+		// nothing to do; directory exists and chown was NOT requested
 		return nil
 	}
 
@@ -49,7 +55,7 @@ func mkdirAs(path string, mode os.FileMode, ownerUID, ownerGID int, mkAll, chown
 				paths = append(paths, dirPath)
 			}
 		}
-		if err := os.MkdirAll(path, mode); err != nil && !os.IsExist(err) {
+		if err := os.MkdirAll(path, mode); err != nil {
 			return err
 		}
 	} else {

diff --git a/pkg/idtools/idtools_windows.go b/pkg/idtools/idtools_windows.go
@@ -9,7 +9,7 @@ import (
 // Platforms such as Windows do not support the UID/GID concept. So make this
 // just a wrapper around system.MkdirAll.
 func mkdirAs(path string, mode os.FileMode, ownerUID, ownerGID int, mkAll, chownExisting bool) error {
-	if err := os.MkdirAll(path, mode); err != nil && !os.IsExist(err) {
+	if err := os.MkdirAll(path, mode); err != nil {
 		return err
 	}
 	return nil

diff --git a/store.go b/store.go
@@ -629,14 +629,14 @@ func GetStore(options StoreOptions) (Store, error) {
 		return nil, errors.Wrap(ErrIncompleteOptions, "no storage runroot specified")
 	}
 
-	if err := os.MkdirAll(options.RunRoot, 0700); err != nil && !os.IsExist(err) {
+	if err := os.MkdirAll(options.RunRoot, 0700); err != nil {
 		return nil, err
 	}
-	if err := os.MkdirAll(options.GraphRoot, 0700); err != nil && !os.IsExist(err) {
+	if err := os.MkdirAll(options.GraphRoot, 0700); err != nil {
 		return nil, err
 	}
 	for _, subdir := range []string{"mounts", "tmp", options.GraphDriverName} {
-		if err := os.MkdirAll(filepath.Join(options.GraphRoot, subdir), 0700); err != nil && !os.IsExist(err) {
+		if err := os.MkdirAll(filepath.Join(options.GraphRoot, subdir), 0700); err != nil {
 			return nil, err
 		}
 	}