Skip to content

Commit a83a769

Browse files
committed
Simplify and fix os.MkdirAll() usage
TL;DR: check for IsExist(err) after a failed MkdirAll() is both redundant and wrong -- so two reasons to remove it. Quoting MkdirAll documentation: > MkdirAll creates a directory named path, along with any necessary > parents, and returns nil, or else returns an error. If path > is already a directory, MkdirAll does nothing and returns nil. This means two things: 1. If a directory to be created already exists, no error is returned. 2. If the error returned is IsExist (EEXIST), it means there exists a non-directory with the same name as MkdirAll need to use for directory. Example: we want to MkdirAll("a/b"), but file "a" (or "a/b") already exists, so MkdirAll fails. The above is a theory, based on quoted documentation and my UNIX knowledge. 3. In practice, though, current MkdirAll implementation [1] returns ENOTDIR in most of cases described in #2, with the exception when there is a race between MkdirAll and someone else creating the last component of MkdirAll argument as a file. In this very case MkdirAll() will indeed return EEXIST. Because of #1, IsExist check after MkdirAll is not needed. Because of #2 and #3, ignoring IsExist error is just plain wrong, as directory we require is not created. It's cleaner to report the error now. Note this error is all over the tree, I guess due to copy-paste, or trying to follow the same usage pattern as for Mkdir(), or some not quite correct examples on the Internet. [v2: a separate aufs commit is merged into this one] [1] https://github.com/golang/go/blob/f9ed2f75/src/os/path.go Signed-off-by: Kir Kolyshkin <kir@openvz.org>
1 parent 868f85b commit a83a769

9 files changed

Lines changed: 15 additions & 19 deletions

File tree

daemon/daemon.go

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -582,7 +582,7 @@ func NewDaemon(config *Config, registryService *registry.Service) (daemon *Daemo
582582
}
583583
config.Root = realRoot
584584
// Create the root directory if it doesn't exists
585-
if err := system.MkdirAll(config.Root, 0700); err != nil && !os.IsExist(err) {
585+
if err := system.MkdirAll(config.Root, 0700); err != nil {
586586
return nil, err
587587
}
588588

@@ -634,7 +634,7 @@ func NewDaemon(config *Config, registryService *registry.Service) (daemon *Daemo
634634

635635
daemonRepo := filepath.Join(config.Root, "containers")
636636

637-
if err := system.MkdirAll(daemonRepo, 0700); err != nil && !os.IsExist(err) {
637+
if err := system.MkdirAll(daemonRepo, 0700); err != nil {
638638
return nil, err
639639
}
640640

@@ -661,7 +661,7 @@ func NewDaemon(config *Config, registryService *registry.Service) (daemon *Daemo
661661

662662
trustDir := filepath.Join(config.Root, "trust")
663663

664-
if err := system.MkdirAll(trustDir, 0700); err != nil && !os.IsExist(err) {
664+
if err := system.MkdirAll(trustDir, 0700); err != nil {
665665
return nil, err
666666
}
667667
trustService, err := trust.NewTrustStore(trustDir)

daemon/graphdriver/aufs/aufs.go

Lines changed: 2 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -104,20 +104,16 @@ func Init(root string, options []string) (graphdriver.Driver, error) {
104104
active: make(map[string]int),
105105
}
106106

107-
// Create the root aufs driver dir and return
108-
// if it already exists
109-
// If not populate the dir structure
107+
// Create the root aufs driver dir
110108
if err := os.MkdirAll(root, 0755); err != nil {
111-
if os.IsExist(err) {
112-
return a, nil
113-
}
114109
return nil, err
115110
}
116111

117112
if err := mountpk.MakePrivate(root); err != nil {
118113
return nil, err
119114
}
120115

116+
// Populate the dir structure
121117
for _, p := range paths {
122118
if err := os.MkdirAll(path.Join(root, p), 0755); err != nil {
123119
return nil, err

daemon/graphdriver/devmapper/deviceset.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -238,7 +238,7 @@ func (devices *DeviceSet) ensureImage(name string, size int64) (string, error) {
238238
dirname := devices.loopbackDir()
239239
filename := path.Join(dirname, name)
240240

241-
if err := os.MkdirAll(dirname, 0700); err != nil && !os.IsExist(err) {
241+
if err := os.MkdirAll(dirname, 0700); err != nil {
242242
return "", err
243243
}
244244

@@ -1260,7 +1260,7 @@ func (devices *DeviceSet) initDevmapper(doInit bool) error {
12601260
logrus.Warn("Udev sync is not supported. This will lead to unexpected behavior, data loss and errors. For more information, see https://docs.docker.com/reference/commandline/cli/#daemon-storage-driver-option")
12611261
}
12621262

1263-
if err := os.MkdirAll(devices.metadataDir(), 0700); err != nil && !os.IsExist(err) {
1263+
if err := os.MkdirAll(devices.metadataDir(), 0700); err != nil {
12641264
return err
12651265
}
12661266

daemon/graphdriver/devmapper/driver.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -159,7 +159,7 @@ func (d *Driver) Get(id, mountLabel string) (string, error) {
159159
mp := path.Join(d.home, "mnt", id)
160160

161161
// Create the target directories if they don't exist
162-
if err := os.MkdirAll(mp, 0755); err != nil && !os.IsExist(err) {
162+
if err := os.MkdirAll(mp, 0755); err != nil {
163163
return "", err
164164
}
165165

@@ -169,7 +169,7 @@ func (d *Driver) Get(id, mountLabel string) (string, error) {
169169
}
170170

171171
rootFs := path.Join(mp, "rootfs")
172-
if err := os.MkdirAll(rootFs, 0755); err != nil && !os.IsExist(err) {
172+
if err := os.MkdirAll(rootFs, 0755); err != nil {
173173
d.DeviceSet.UnmountDevice(id)
174174
return "", err
175175
}

daemon/graphdriver/overlay/overlay.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -124,7 +124,7 @@ func Init(home string, options []string) (graphdriver.Driver, error) {
124124
}
125125

126126
// Create the driver home dir
127-
if err := os.MkdirAll(home, 0755); err != nil && !os.IsExist(err) {
127+
if err := os.MkdirAll(home, 0755); err != nil {
128128
return nil, err
129129
}
130130

daemon/graphdriver/zfs/zfs.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -280,7 +280,7 @@ func (d *Driver) Get(id, mountLabel string) (string, error) {
280280
logrus.Debugf(`[zfs] mount("%s", "%s", "%s")`, filesystem, mountpoint, options)
281281

282282
// Create the target directories if they don't exist
283-
if err := os.MkdirAll(mountpoint, 0755); err != nil && !os.IsExist(err) {
283+
if err := os.MkdirAll(mountpoint, 0755); err != nil {
284284
return "", err
285285
}
286286

graph/graph.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -107,7 +107,7 @@ func NewGraph(root string, driver graphdriver.Driver) (*Graph, error) {
107107
return nil, err
108108
}
109109
// Create the root directory if it doesn't exists
110-
if err := system.MkdirAll(root, 0700); err != nil && !os.IsExist(err) {
110+
if err := system.MkdirAll(root, 0700); err != nil {
111111
return nil, err
112112
}
113113

integration-cli/docker_utils.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1187,7 +1187,7 @@ func newFakeGit(name string, files map[string]string, enforceLocalServer bool) (
11871187
// Call c.Fatal() at the first error.
11881188
func writeFile(dst, content string, c *check.C) {
11891189
// Create subdirectories if necessary
1190-
if err := os.MkdirAll(path.Dir(dst), 0700); err != nil && !os.IsExist(err) {
1190+
if err := os.MkdirAll(path.Dir(dst), 0700); err != nil {
11911191
c.Fatal(err)
11921192
}
11931193
f, err := os.OpenFile(dst, os.O_CREATE|os.O_RDWR|os.O_TRUNC, 0700)

pkg/archive/archive.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -714,7 +714,7 @@ func (archiver *Archiver) CopyWithTar(src, dst string) error {
714714
}
715715
// Create dst, copy src's content into it
716716
logrus.Debugf("Creating dest directory: %s", dst)
717-
if err := system.MkdirAll(dst, 0755); err != nil && !os.IsExist(err) {
717+
if err := system.MkdirAll(dst, 0755); err != nil {
718718
return err
719719
}
720720
logrus.Debugf("Calling TarUntar(%s, %s)", src, dst)
@@ -746,7 +746,7 @@ func (archiver *Archiver) CopyFileWithTar(src, dst string) (err error) {
746746
dst = filepath.Join(dst, filepath.Base(src))
747747
}
748748
// Create the holding directory if necessary
749-
if err := system.MkdirAll(filepath.Dir(dst), 0700); err != nil && !os.IsExist(err) {
749+
if err := system.MkdirAll(filepath.Dir(dst), 0700); err != nil {
750750
return err
751751
}
752752

0 commit comments

Comments
 (0)