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>
Upstream-commit: a83a76934787a20e96389d33bd56a09369f9b808
Component: engine
This commit is contained in:
Kir Kolyshkin
2015-07-29 16:49:05 -07:00
parent 7efd312a12
commit 3c8e934faf
9 changed files with 15 additions and 19 deletions

View File

@ -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
}
@ -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
}

View File

@ -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
}
@ -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
}