Skip to content

Commit

Permalink
Simplify and fix os.MkdirAll() usage
Browse files Browse the repository at this point in the history
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>
  • Loading branch information
kolyshkin committed Jul 30, 2015
1 parent 868f85b commit a83a769
Show file tree
Hide file tree
Showing 9 changed files with 15 additions and 19 deletions.
6 changes: 3 additions & 3 deletions daemon/daemon.go
Original file line number Diff line number Diff line change
Expand Up @@ -582,7 +582,7 @@ func NewDaemon(config *Config, registryService *registry.Service) (daemon *Daemo
}
config.Root = realRoot
// Create the root directory if it doesn't exists
if err := system.MkdirAll(config.Root, 0700); err != nil && !os.IsExist(err) {
if err := system.MkdirAll(config.Root, 0700); err != nil {
return nil, err
}

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

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

if err := system.MkdirAll(daemonRepo, 0700); err != nil && !os.IsExist(err) {
if err := system.MkdirAll(daemonRepo, 0700); err != nil {
return nil, err
}

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

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

if err := system.MkdirAll(trustDir, 0700); err != nil && !os.IsExist(err) {
if err := system.MkdirAll(trustDir, 0700); err != nil {
return nil, err
}
trustService, err := trust.NewTrustStore(trustDir)
Expand Down
8 changes: 2 additions & 6 deletions daemon/graphdriver/aufs/aufs.go
Original file line number Diff line number Diff line change
Expand Up @@ -104,20 +104,16 @@ func Init(root string, options []string) (graphdriver.Driver, error) {
active: make(map[string]int),
}

// Create the root aufs driver dir and return
// if it already exists
// If not populate the dir structure
// Create the root aufs driver dir
if err := os.MkdirAll(root, 0755); err != nil {
if os.IsExist(err) {
return a, nil
}
return nil, err
}

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

// Populate the dir structure
for _, p := range paths {
if err := os.MkdirAll(path.Join(root, p), 0755); err != nil {
return nil, err
Expand Down
4 changes: 2 additions & 2 deletions daemon/graphdriver/devmapper/deviceset.go
Original file line number Diff line number Diff line change
Expand Up @@ -238,7 +238,7 @@ func (devices *DeviceSet) ensureImage(name string, size int64) (string, error) {
dirname := devices.loopbackDir()
filename := path.Join(dirname, name)

if err := os.MkdirAll(dirname, 0700); err != nil && !os.IsExist(err) {
if err := os.MkdirAll(dirname, 0700); err != nil {
return "", err
}

Expand Down Expand Up @@ -1260,7 +1260,7 @@ func (devices *DeviceSet) initDevmapper(doInit bool) error {
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")
}

if err := os.MkdirAll(devices.metadataDir(), 0700); err != nil && !os.IsExist(err) {
if err := os.MkdirAll(devices.metadataDir(), 0700); err != nil {
return err
}

Expand Down
4 changes: 2 additions & 2 deletions daemon/graphdriver/devmapper/driver.go
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,7 @@ func (d *Driver) Get(id, mountLabel string) (string, error) {
mp := path.Join(d.home, "mnt", id)

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

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

rootFs := path.Join(mp, "rootfs")
if err := os.MkdirAll(rootFs, 0755); err != nil && !os.IsExist(err) {
if err := os.MkdirAll(rootFs, 0755); err != nil {
d.DeviceSet.UnmountDevice(id)
return "", err
}
Expand Down
2 changes: 1 addition & 1 deletion daemon/graphdriver/overlay/overlay.go
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,7 @@ func Init(home string, options []string) (graphdriver.Driver, error) {
}

// Create the driver home dir
if err := os.MkdirAll(home, 0755); err != nil && !os.IsExist(err) {
if err := os.MkdirAll(home, 0755); err != nil {
return nil, err
}

Expand Down
2 changes: 1 addition & 1 deletion daemon/graphdriver/zfs/zfs.go
Original file line number Diff line number Diff line change
Expand Up @@ -280,7 +280,7 @@ func (d *Driver) Get(id, mountLabel string) (string, error) {
logrus.Debugf(`[zfs] mount("%s", "%s", "%s")`, filesystem, mountpoint, options)

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

Expand Down
2 changes: 1 addition & 1 deletion graph/graph.go
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ func NewGraph(root string, driver graphdriver.Driver) (*Graph, error) {
return nil, err
}
// Create the root directory if it doesn't exists
if err := system.MkdirAll(root, 0700); err != nil && !os.IsExist(err) {
if err := system.MkdirAll(root, 0700); err != nil {
return nil, err
}

Expand Down
2 changes: 1 addition & 1 deletion integration-cli/docker_utils.go
Original file line number Diff line number Diff line change
Expand Up @@ -1187,7 +1187,7 @@ func newFakeGit(name string, files map[string]string, enforceLocalServer bool) (
// Call c.Fatal() at the first error.
func writeFile(dst, content string, c *check.C) {
// Create subdirectories if necessary
if err := os.MkdirAll(path.Dir(dst), 0700); err != nil && !os.IsExist(err) {
if err := os.MkdirAll(path.Dir(dst), 0700); err != nil {
c.Fatal(err)
}
f, err := os.OpenFile(dst, os.O_CREATE|os.O_RDWR|os.O_TRUNC, 0700)
Expand Down
4 changes: 2 additions & 2 deletions pkg/archive/archive.go
Original file line number Diff line number Diff line change
Expand Up @@ -714,7 +714,7 @@ func (archiver *Archiver) CopyWithTar(src, dst string) error {
}
// Create dst, copy src's content into it
logrus.Debugf("Creating dest directory: %s", dst)
if err := system.MkdirAll(dst, 0755); err != nil && !os.IsExist(err) {
if err := system.MkdirAll(dst, 0755); err != nil {
return err
}
logrus.Debugf("Calling TarUntar(%s, %s)", src, dst)
Expand Down Expand Up @@ -746,7 +746,7 @@ func (archiver *Archiver) CopyFileWithTar(src, dst string) (err error) {
dst = filepath.Join(dst, filepath.Base(src))
}
// Create the holding directory if necessary
if err := system.MkdirAll(filepath.Dir(dst), 0700); err != nil && !os.IsExist(err) {
if err := system.MkdirAll(filepath.Dir(dst), 0700); err != nil {
return err
}

Expand Down

0 comments on commit a83a769

Please sign in to comment.