Browse code

Merge pull request #35618 from kolyshkin/mkdir-all

Fix MkdirAll* and its usage

Michael Crosby authored on 2017/12/01 01:19:29
Showing 16 changed files
... ...
@@ -34,9 +34,6 @@ func getCheckpointDir(checkDir, checkpointID, ctrName, ctrID, ctrCheckpointDir s
34 34
 			err2 = fmt.Errorf("checkpoint with name %s already exists for container %s", checkpointID, ctrName)
35 35
 		case err != nil && os.IsNotExist(err):
36 36
 			err2 = os.MkdirAll(checkpointAbsDir, 0700)
37
-			if os.IsExist(err2) {
38
-				err2 = nil
39
-			}
40 37
 		case err != nil:
41 38
 			err2 = err
42 39
 		case err == nil:
... ...
@@ -675,14 +675,14 @@ func NewDaemon(config *config.Config, registryService registry.Service, containe
675 675
 	}
676 676
 
677 677
 	daemonRepo := filepath.Join(config.Root, "containers")
678
-	if err := idtools.MkdirAllAndChown(daemonRepo, 0700, rootIDs); err != nil && !os.IsExist(err) {
678
+	if err := idtools.MkdirAllAndChown(daemonRepo, 0700, rootIDs); err != nil {
679 679
 		return nil, err
680 680
 	}
681 681
 
682 682
 	// Create the directory where we'll store the runtime scripts (i.e. in
683 683
 	// order to support runtimeArgs)
684 684
 	daemonRuntimes := filepath.Join(config.Root, "runtimes")
685
-	if err := system.MkdirAll(daemonRuntimes, 0700, ""); err != nil && !os.IsExist(err) {
685
+	if err := system.MkdirAll(daemonRuntimes, 0700, ""); err != nil {
686 686
 		return nil, err
687 687
 	}
688 688
 	if err := d.loadRuntimes(); err != nil {
... ...
@@ -690,7 +690,7 @@ func NewDaemon(config *config.Config, registryService registry.Service, containe
690 690
 	}
691 691
 
692 692
 	if runtime.GOOS == "windows" {
693
-		if err := system.MkdirAll(filepath.Join(config.Root, "credentialspecs"), 0, ""); err != nil && !os.IsExist(err) {
693
+		if err := system.MkdirAll(filepath.Join(config.Root, "credentialspecs"), 0, ""); err != nil {
694 694
 			return nil, err
695 695
 		}
696 696
 	}
... ...
@@ -1514,7 +1514,7 @@ func (daemon *Daemon) initCgroupsPath(path string) error {
1514 1514
 
1515 1515
 func maybeCreateCPURealTimeFile(sysinfoPresent bool, configValue int64, file string, path string) error {
1516 1516
 	if sysinfoPresent && configValue != 0 {
1517
-		if err := os.MkdirAll(path, 0755); err != nil && !os.IsExist(err) {
1517
+		if err := os.MkdirAll(path, 0755); err != nil {
1518 1518
 			return err
1519 1519
 		}
1520 1520
 		if err := ioutil.WriteFile(filepath.Join(path, file), []byte(strconv.FormatInt(configValue, 10)), 0700); err != nil {
... ...
@@ -3,7 +3,6 @@ package daemon
3 3
 import (
4 4
 	"context"
5 5
 	"fmt"
6
-	"os"
7 6
 	"path/filepath"
8 7
 	"strings"
9 8
 
... ...
@@ -485,7 +484,7 @@ func setupRemappedRoot(config *config.Config) (*idtools.IDMappings, error) {
485 485
 func setupDaemonRoot(config *config.Config, rootDir string, rootIDs idtools.IDPair) error {
486 486
 	config.Root = rootDir
487 487
 	// Create the root directory if it doesn't exists
488
-	if err := system.MkdirAllWithACL(config.Root, 0, system.SddlAdministratorsLocalSystem); err != nil && !os.IsExist(err) {
488
+	if err := system.MkdirAllWithACL(config.Root, 0, system.SddlAdministratorsLocalSystem); err != nil {
489 489
 		return err
490 490
 	}
491 491
 	return nil
... ...
@@ -122,13 +122,8 @@ func Init(root string, options []string, uidMaps, gidMaps []idtools.IDMap) (grap
122 122
 	if err != nil {
123 123
 		return nil, err
124 124
 	}
125
-	// Create the root aufs driver dir and return
126
-	// if it already exists
127
-	// If not populate the dir structure
125
+	// Create the root aufs driver dir
128 126
 	if err := idtools.MkdirAllAndChown(root, 0700, idtools.IDPair{UID: rootUID, GID: rootGID}); err != nil {
129
-		if os.IsExist(err) {
130
-			return a, nil
131
-		}
132 127
 		return nil, err
133 128
 	}
134 129
 
... ...
@@ -268,7 +268,7 @@ func (devices *DeviceSet) ensureImage(name string, size int64) (string, error) {
268 268
 	if err != nil {
269 269
 		return "", err
270 270
 	}
271
-	if err := idtools.MkdirAllAndChown(dirname, 0700, idtools.IDPair{UID: uid, GID: gid}); err != nil && !os.IsExist(err) {
271
+	if err := idtools.MkdirAllAndChown(dirname, 0700, idtools.IDPair{UID: uid, GID: gid}); err != nil {
272 272
 		return "", err
273 273
 	}
274 274
 
... ...
@@ -1697,10 +1697,10 @@ func (devices *DeviceSet) initDevmapper(doInit bool) (retErr error) {
1697 1697
 	if err != nil {
1698 1698
 		return err
1699 1699
 	}
1700
-	if err := idtools.MkdirAndChown(devices.root, 0700, idtools.IDPair{UID: uid, GID: gid}); err != nil && !os.IsExist(err) {
1700
+	if err := idtools.MkdirAndChown(devices.root, 0700, idtools.IDPair{UID: uid, GID: gid}); err != nil {
1701 1701
 		return err
1702 1702
 	}
1703
-	if err := os.MkdirAll(devices.metadataDir(), 0700); err != nil && !os.IsExist(err) {
1703
+	if err := os.MkdirAll(devices.metadataDir(), 0700); err != nil {
1704 1704
 		return err
1705 1705
 	}
1706 1706
 
... ...
@@ -189,7 +189,7 @@ func (d *Driver) Get(id, mountLabel string) (containerfs.ContainerFS, error) {
189 189
 	}
190 190
 
191 191
 	// Create the target directories if they don't exist
192
-	if err := idtools.MkdirAllAndChown(path.Join(d.home, "mnt"), 0755, idtools.IDPair{UID: uid, GID: gid}); err != nil && !os.IsExist(err) {
192
+	if err := idtools.MkdirAllAndChown(path.Join(d.home, "mnt"), 0755, idtools.IDPair{UID: uid, GID: gid}); err != nil {
193 193
 		d.ctr.Decrement(mp)
194 194
 		return nil, err
195 195
 	}
... ...
@@ -204,7 +204,7 @@ func (d *Driver) Get(id, mountLabel string) (containerfs.ContainerFS, error) {
204 204
 		return nil, err
205 205
 	}
206 206
 
207
-	if err := idtools.MkdirAllAndChown(rootFs, 0755, idtools.IDPair{UID: uid, GID: gid}); err != nil && !os.IsExist(err) {
207
+	if err := idtools.MkdirAllAndChown(rootFs, 0755, idtools.IDPair{UID: uid, GID: gid}); err != nil {
208 208
 		d.ctr.Decrement(mp)
209 209
 		d.DeviceSet.UnmountDevice(id, mp)
210 210
 		return nil, err
... ...
@@ -138,7 +138,7 @@ func Init(home string, options []string, uidMaps, gidMaps []idtools.IDMap) (grap
138 138
 		return nil, err
139 139
 	}
140 140
 	// Create the driver home dir
141
-	if err := idtools.MkdirAllAndChown(home, 0700, idtools.IDPair{rootUID, rootGID}); err != nil && !os.IsExist(err) {
141
+	if err := idtools.MkdirAllAndChown(home, 0700, idtools.IDPair{rootUID, rootGID}); err != nil {
142 142
 		return nil, err
143 143
 	}
144 144
 
... ...
@@ -177,7 +177,7 @@ func Init(home string, options []string, uidMaps, gidMaps []idtools.IDMap) (grap
177 177
 		return nil, err
178 178
 	}
179 179
 	// Create the driver home dir
180
-	if err := idtools.MkdirAllAndChown(path.Join(home, linkDir), 0700, idtools.IDPair{rootUID, rootGID}); err != nil && !os.IsExist(err) {
180
+	if err := idtools.MkdirAllAndChown(path.Join(home, linkDir), 0700, idtools.IDPair{rootUID, rootGID}); err != nil {
181 181
 		return nil, err
182 182
 	}
183 183
 
... ...
@@ -42,7 +42,9 @@ func MkdirAllAndChown(path string, mode os.FileMode, owner IDPair) error {
42 42
 }
43 43
 
44 44
 // MkdirAndChown creates a directory and then modifies ownership to the requested uid/gid.
45
-// If the directory already exists, this function still changes ownership
45
+// If the directory already exists, this function still changes ownership.
46
+// Note that unlike os.Mkdir(), this function does not return IsExist error
47
+// in case path already exists.
46 48
 func MkdirAndChown(path string, mode os.FileMode, owner IDPair) error {
47 49
 	return mkdirAs(path, mode, owner.UID, owner.GID, false, true)
48 50
 }
... ...
@@ -10,6 +10,7 @@ import (
10 10
 	"path/filepath"
11 11
 	"strings"
12 12
 	"sync"
13
+	"syscall"
13 14
 
14 15
 	"github.com/docker/docker/pkg/system"
15 16
 	"github.com/opencontainers/runc/libcontainer/user"
... ...
@@ -29,6 +30,9 @@ func mkdirAs(path string, mode os.FileMode, ownerUID, ownerGID int, mkAll, chown
29 29
 
30 30
 	stat, err := system.Stat(path)
31 31
 	if err == nil {
32
+		if !stat.IsDir() {
33
+			return &os.PathError{Op: "mkdir", Path: path, Err: syscall.ENOTDIR}
34
+		}
32 35
 		if !chownExisting {
33 36
 			return nil
34 37
 		}
... ...
@@ -54,7 +58,7 @@ func mkdirAs(path string, mode os.FileMode, ownerUID, ownerGID int, mkAll, chown
54 54
 				paths = append(paths, dirPath)
55 55
 			}
56 56
 		}
57
-		if err := system.MkdirAll(path, mode, ""); err != nil && !os.IsExist(err) {
57
+		if err := system.MkdirAll(path, mode, ""); err != nil {
58 58
 			return err
59 59
 		}
60 60
 	} else {
... ...
@@ -378,6 +378,20 @@ func TestLookupUserAndGroupThatDoesNotExist(t *testing.T) {
378 378
 	assert.Error(t, err)
379 379
 }
380 380
 
381
+// TestMkdirIsNotDir checks that mkdirAs() function (used by MkdirAll...)
382
+// returns a correct error in case a directory which it is about to create
383
+// already exists but is a file (rather than a directory).
384
+func TestMkdirIsNotDir(t *testing.T) {
385
+	file, err := ioutil.TempFile("", t.Name())
386
+	if err != nil {
387
+		t.Fatalf("Couldn't create temp dir: %v", err)
388
+	}
389
+	defer os.Remove(file.Name())
390
+
391
+	err = mkdirAs(file.Name(), 0755, 0, 0, false, false)
392
+	assert.EqualError(t, err, "mkdir "+file.Name()+": not a directory")
393
+}
394
+
381 395
 func RequiresRoot(t *testing.T) {
382 396
 	skip.IfCondition(t, os.Getuid() != 0, "skipping test that requires root")
383 397
 }
... ...
@@ -11,7 +11,7 @@ import (
11 11
 // Platforms such as Windows do not support the UID/GID concept. So make this
12 12
 // just a wrapper around system.MkdirAll.
13 13
 func mkdirAs(path string, mode os.FileMode, ownerUID, ownerGID int, mkAll, chownExisting bool) error {
14
-	if err := system.MkdirAll(path, mode, ""); err != nil && !os.IsExist(err) {
14
+	if err := system.MkdirAll(path, mode, ""); err != nil {
15 15
 		return err
16 16
 	}
17 17
 	return nil
... ...
@@ -47,6 +47,11 @@ func (s StatT) Mtim() syscall.Timespec {
47 47
 	return s.mtim
48 48
 }
49 49
 
50
+// IsDir reports whether s describes a directory.
51
+func (s StatT) IsDir() bool {
52
+	return s.mode&syscall.S_IFDIR != 0
53
+}
54
+
50 55
 // Stat takes a path to a file and returns
51 56
 // a system.StatT type pertaining to that file.
52 57
 //
... ...
@@ -139,16 +139,6 @@ func (r *Root) Name() string {
139 139
 	return volume.DefaultDriverName
140 140
 }
141 141
 
142
-type alreadyExistsError struct {
143
-	path string
144
-}
145
-
146
-func (e alreadyExistsError) Error() string {
147
-	return "local volume already exists under " + e.path
148
-}
149
-
150
-func (e alreadyExistsError) Conflict() {}
151
-
152 142
 type systemError struct {
153 143
 	err error
154 144
 }
... ...
@@ -181,9 +171,6 @@ func (r *Root) Create(name string, opts map[string]string) (volume.Volume, error
181 181
 
182 182
 	path := r.DataPath(name)
183 183
 	if err := idtools.MkdirAllAndChown(path, 0755, r.rootIDs); err != nil {
184
-		if os.IsExist(err) {
185
-			return nil, alreadyExistsError{filepath.Dir(path)}
186
-		}
187 184
 		return nil, errors.Wrapf(systemError{err}, "error while creating volume path '%s'", path)
188 185
 	}
189 186
 
... ...
@@ -192,7 +192,6 @@ func (m *MountPoint) Setup(mountLabel string, rootIDs idtools.IDPair, checkFun f
192 192
 		return "", fmt.Errorf("Unable to setup mount point, neither source nor volume defined")
193 193
 	}
194 194
 
195
-	// system.MkdirAll() produces an error if m.Source exists and is a file (not a directory),
196 195
 	if m.Type == mounttypes.TypeBind {
197 196
 		// Before creating the source directory on the host, invoke checkFun if it's not nil. One of
198 197
 		// the use case is to forbid creating the daemon socket as a directory if the daemon is in