From bc89af99292abcf7fe14db72f7d1e07d23f2bb83 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Mon, 25 Sep 2017 12:39:36 -0700 Subject: [PATCH] Simplify/fix MkdirAll usage This subtle bug keeps lurking in because error checking for `Mkdir()` and `MkdirAll()` is slightly different wrt to `EEXIST`/`IsExist`: - for `Mkdir()`, `IsExist` error should (usually) be ignored (unless you want to make sure directory was not there before) as it means "the destination directory was already there" - for `MkdirAll()`, `IsExist` error should NEVER be ignored. Mostly, this commit just removes ignoring the IsExist error, as it should not be ignored. Also, there are a couple of cases then IsExist is handled as "directory already exist" which is wrong. As a result, some code that never worked as intended is now removed. NOTE that `idtools.MkdirAndChown()` behaves like `os.MkdirAll()` rather than `os.Mkdir()` -- so its description is amended accordingly, and its usage is handled as such (i.e. IsExist error is not ignored). For more details, a quote from my runc commit 6f82d4b (July 2015): 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. [1] https://github.com/golang/go/blob/f9ed2f75/src/os/path.go Signed-off-by: Kir Kolyshkin Upstream-commit: 516010e92d56cfcd6d1e343bdc02b6f04bc43039 Component: engine --- components/engine/daemon/checkpoint.go | 3 --- components/engine/daemon/daemon.go | 6 +++--- components/engine/daemon/daemon_unix.go | 2 +- components/engine/daemon/daemon_windows.go | 3 +-- components/engine/daemon/graphdriver/aufs/aufs.go | 7 +------ .../daemon/graphdriver/devmapper/deviceset.go | 6 +++--- .../engine/daemon/graphdriver/devmapper/driver.go | 4 ++-- .../engine/daemon/graphdriver/overlay/overlay.go | 2 +- .../engine/daemon/graphdriver/overlay2/overlay.go | 2 +- components/engine/pkg/idtools/idtools.go | 4 +++- components/engine/pkg/idtools/idtools_unix.go | 4 ++-- components/engine/pkg/idtools/idtools_windows.go | 2 +- components/engine/volume/local/local.go | 13 ------------- components/engine/volume/volume.go | 1 - 14 files changed, 19 insertions(+), 40 deletions(-) diff --git a/components/engine/daemon/checkpoint.go b/components/engine/daemon/checkpoint.go index 5765af7c5a..482e3b848e 100644 --- a/components/engine/daemon/checkpoint.go +++ b/components/engine/daemon/checkpoint.go @@ -34,9 +34,6 @@ func getCheckpointDir(checkDir, checkpointID, ctrName, ctrID, ctrCheckpointDir s err2 = fmt.Errorf("checkpoint with name %s already exists for container %s", checkpointID, ctrName) case err != nil && os.IsNotExist(err): err2 = os.MkdirAll(checkpointAbsDir, 0700) - if os.IsExist(err2) { - err2 = nil - } case err != nil: err2 = err case err == nil: diff --git a/components/engine/daemon/daemon.go b/components/engine/daemon/daemon.go index 96b39e8820..e63e2097f1 100644 --- a/components/engine/daemon/daemon.go +++ b/components/engine/daemon/daemon.go @@ -675,14 +675,14 @@ func NewDaemon(config *config.Config, registryService registry.Service, containe } daemonRepo := filepath.Join(config.Root, "containers") - if err := idtools.MkdirAllAndChown(daemonRepo, 0700, rootIDs); err != nil && !os.IsExist(err) { + if err := idtools.MkdirAllAndChown(daemonRepo, 0700, rootIDs); err != nil { return nil, err } // Create the directory where we'll store the runtime scripts (i.e. in // order to support runtimeArgs) daemonRuntimes := filepath.Join(config.Root, "runtimes") - if err := system.MkdirAll(daemonRuntimes, 0700, ""); err != nil && !os.IsExist(err) { + if err := system.MkdirAll(daemonRuntimes, 0700, ""); err != nil { return nil, err } if err := d.loadRuntimes(); err != nil { @@ -690,7 +690,7 @@ func NewDaemon(config *config.Config, registryService registry.Service, containe } if runtime.GOOS == "windows" { - if err := system.MkdirAll(filepath.Join(config.Root, "credentialspecs"), 0, ""); err != nil && !os.IsExist(err) { + if err := system.MkdirAll(filepath.Join(config.Root, "credentialspecs"), 0, ""); err != nil { return nil, err } } diff --git a/components/engine/daemon/daemon_unix.go b/components/engine/daemon/daemon_unix.go index b0b624772a..1f9885d176 100644 --- a/components/engine/daemon/daemon_unix.go +++ b/components/engine/daemon/daemon_unix.go @@ -1514,7 +1514,7 @@ func (daemon *Daemon) initCgroupsPath(path string) error { func maybeCreateCPURealTimeFile(sysinfoPresent bool, configValue int64, file string, path string) error { if sysinfoPresent && configValue != 0 { - if err := os.MkdirAll(path, 0755); err != nil && !os.IsExist(err) { + if err := os.MkdirAll(path, 0755); err != nil { return err } if err := ioutil.WriteFile(filepath.Join(path, file), []byte(strconv.FormatInt(configValue, 10)), 0700); err != nil { diff --git a/components/engine/daemon/daemon_windows.go b/components/engine/daemon/daemon_windows.go index 8545a2f651..8029bbf0ef 100644 --- a/components/engine/daemon/daemon_windows.go +++ b/components/engine/daemon/daemon_windows.go @@ -3,7 +3,6 @@ package daemon import ( "context" "fmt" - "os" "path/filepath" "strings" @@ -485,7 +484,7 @@ func setupRemappedRoot(config *config.Config) (*idtools.IDMappings, error) { func setupDaemonRoot(config *config.Config, rootDir string, rootIDs idtools.IDPair) error { config.Root = rootDir // Create the root directory if it doesn't exists - if err := system.MkdirAllWithACL(config.Root, 0, system.SddlAdministratorsLocalSystem); err != nil && !os.IsExist(err) { + if err := system.MkdirAllWithACL(config.Root, 0, system.SddlAdministratorsLocalSystem); err != nil { return err } return nil diff --git a/components/engine/daemon/graphdriver/aufs/aufs.go b/components/engine/daemon/graphdriver/aufs/aufs.go index 5a1f3d1fdd..11e763d025 100644 --- a/components/engine/daemon/graphdriver/aufs/aufs.go +++ b/components/engine/daemon/graphdriver/aufs/aufs.go @@ -122,13 +122,8 @@ func Init(root string, options []string, uidMaps, gidMaps []idtools.IDMap) (grap if err != nil { return nil, err } - // 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 := idtools.MkdirAllAndChown(root, 0700, idtools.IDPair{UID: rootUID, GID: rootGID}); err != nil { - if os.IsExist(err) { - return a, nil - } return nil, err } diff --git a/components/engine/daemon/graphdriver/devmapper/deviceset.go b/components/engine/daemon/graphdriver/devmapper/deviceset.go index 160ba46407..db41f05097 100644 --- a/components/engine/daemon/graphdriver/devmapper/deviceset.go +++ b/components/engine/daemon/graphdriver/devmapper/deviceset.go @@ -268,7 +268,7 @@ func (devices *DeviceSet) ensureImage(name string, size int64) (string, error) { if err != nil { return "", err } - if err := idtools.MkdirAllAndChown(dirname, 0700, idtools.IDPair{UID: uid, GID: gid}); err != nil && !os.IsExist(err) { + if err := idtools.MkdirAllAndChown(dirname, 0700, idtools.IDPair{UID: uid, GID: gid}); err != nil { return "", err } @@ -1697,10 +1697,10 @@ func (devices *DeviceSet) initDevmapper(doInit bool) (retErr error) { if err != nil { return err } - if err := idtools.MkdirAndChown(devices.root, 0700, idtools.IDPair{UID: uid, GID: gid}); err != nil && !os.IsExist(err) { + if err := idtools.MkdirAndChown(devices.root, 0700, idtools.IDPair{UID: uid, GID: 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/components/engine/daemon/graphdriver/devmapper/driver.go b/components/engine/daemon/graphdriver/devmapper/driver.go index 288f50126f..707094af3a 100644 --- a/components/engine/daemon/graphdriver/devmapper/driver.go +++ b/components/engine/daemon/graphdriver/devmapper/driver.go @@ -189,7 +189,7 @@ func (d *Driver) Get(id, mountLabel string) (containerfs.ContainerFS, error) { } // Create the target directories if they don't exist - if err := idtools.MkdirAllAndChown(path.Join(d.home, "mnt"), 0755, idtools.IDPair{UID: uid, GID: gid}); err != nil && !os.IsExist(err) { + if err := idtools.MkdirAllAndChown(path.Join(d.home, "mnt"), 0755, idtools.IDPair{UID: uid, GID: gid}); err != nil { d.ctr.Decrement(mp) return nil, err } @@ -204,7 +204,7 @@ func (d *Driver) Get(id, mountLabel string) (containerfs.ContainerFS, error) { return nil, err } - if err := idtools.MkdirAllAndChown(rootFs, 0755, idtools.IDPair{UID: uid, GID: gid}); err != nil && !os.IsExist(err) { + if err := idtools.MkdirAllAndChown(rootFs, 0755, idtools.IDPair{UID: uid, GID: gid}); err != nil { d.ctr.Decrement(mp) d.DeviceSet.UnmountDevice(id, mp) return nil, err diff --git a/components/engine/daemon/graphdriver/overlay/overlay.go b/components/engine/daemon/graphdriver/overlay/overlay.go index 853318de59..a6ac8cdd7e 100644 --- a/components/engine/daemon/graphdriver/overlay/overlay.go +++ b/components/engine/daemon/graphdriver/overlay/overlay.go @@ -136,7 +136,7 @@ func Init(home string, options []string, uidMaps, gidMaps []idtools.IDMap) (grap return nil, err } // Create the driver home dir - if err := idtools.MkdirAllAndChown(home, 0700, idtools.IDPair{rootUID, rootGID}); err != nil && !os.IsExist(err) { + if err := idtools.MkdirAllAndChown(home, 0700, idtools.IDPair{rootUID, rootGID}); err != nil { return nil, err } diff --git a/components/engine/daemon/graphdriver/overlay2/overlay.go b/components/engine/daemon/graphdriver/overlay2/overlay.go index c2023c7252..9046dc9e3d 100644 --- a/components/engine/daemon/graphdriver/overlay2/overlay.go +++ b/components/engine/daemon/graphdriver/overlay2/overlay.go @@ -171,7 +171,7 @@ func Init(home string, options []string, uidMaps, gidMaps []idtools.IDMap) (grap return nil, err } // Create the driver home dir - if err := idtools.MkdirAllAndChown(path.Join(home, linkDir), 0700, idtools.IDPair{rootUID, rootGID}); err != nil && !os.IsExist(err) { + if err := idtools.MkdirAllAndChown(path.Join(home, linkDir), 0700, idtools.IDPair{rootUID, rootGID}); err != nil { return nil, err } diff --git a/components/engine/pkg/idtools/idtools.go b/components/engine/pkg/idtools/idtools.go index 49cc97c3d6..6108ae3f43 100644 --- a/components/engine/pkg/idtools/idtools.go +++ b/components/engine/pkg/idtools/idtools.go @@ -42,7 +42,9 @@ func MkdirAllAndChown(path string, mode os.FileMode, owner IDPair) error { } // MkdirAndChown creates a directory and then modifies ownership to the requested uid/gid. -// If the directory already exists, this function still changes ownership +// If the directory already exists, this function still changes ownership. +// Note that unlike os.Mkdir(), this function does not return IsExist error +// in case path already exists. func MkdirAndChown(path string, mode os.FileMode, owner IDPair) error { return mkdirAs(path, mode, owner.UID, owner.GID, false, true) } diff --git a/components/engine/pkg/idtools/idtools_unix.go b/components/engine/pkg/idtools/idtools_unix.go index b5e9c84075..aedf8ad343 100644 --- a/components/engine/pkg/idtools/idtools_unix.go +++ b/components/engine/pkg/idtools/idtools_unix.go @@ -31,7 +31,7 @@ func mkdirAs(path string, mode os.FileMode, ownerUID, ownerGID int, mkAll, chown stat, err := system.Stat(path) if err == nil { if !stat.IsDir() { - return &os.PathError{"mkdir", path, syscall.ENOTDIR} + return &os.PathError{Op: "mkdir", Path: path, Err: syscall.ENOTDIR} } if !chownExisting { return nil @@ -58,7 +58,7 @@ func mkdirAs(path string, mode os.FileMode, ownerUID, ownerGID int, mkAll, chown paths = append(paths, dirPath) } } - if err := system.MkdirAll(path, mode, ""); err != nil && !os.IsExist(err) { + if err := system.MkdirAll(path, mode, ""); err != nil { return err } } else { diff --git a/components/engine/pkg/idtools/idtools_windows.go b/components/engine/pkg/idtools/idtools_windows.go index 45d2878e38..94ca33afb1 100644 --- a/components/engine/pkg/idtools/idtools_windows.go +++ b/components/engine/pkg/idtools/idtools_windows.go @@ -11,7 +11,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 := system.MkdirAll(path, mode, ""); err != nil && !os.IsExist(err) { + if err := system.MkdirAll(path, mode, ""); err != nil { return err } return nil diff --git a/components/engine/volume/local/local.go b/components/engine/volume/local/local.go index b37c45e61e..c11a18c3dc 100644 --- a/components/engine/volume/local/local.go +++ b/components/engine/volume/local/local.go @@ -139,16 +139,6 @@ func (r *Root) Name() string { return volume.DefaultDriverName } -type alreadyExistsError struct { - path string -} - -func (e alreadyExistsError) Error() string { - return "local volume already exists under " + e.path -} - -func (e alreadyExistsError) Conflict() {} - type systemError struct { err error } @@ -181,9 +171,6 @@ func (r *Root) Create(name string, opts map[string]string) (volume.Volume, error path := r.DataPath(name) if err := idtools.MkdirAllAndChown(path, 0755, r.rootIDs); err != nil { - if os.IsExist(err) { - return nil, alreadyExistsError{filepath.Dir(path)} - } return nil, errors.Wrapf(systemError{err}, "error while creating volume path '%s'", path) } diff --git a/components/engine/volume/volume.go b/components/engine/volume/volume.go index b8ec1e5a00..207dc560c3 100644 --- a/components/engine/volume/volume.go +++ b/components/engine/volume/volume.go @@ -192,7 +192,6 @@ func (m *MountPoint) Setup(mountLabel string, rootIDs idtools.IDPair, checkFun f return "", fmt.Errorf("Unable to setup mount point, neither source nor volume defined") } - // system.MkdirAll() produces an error if m.Source exists and is a file (not a directory), if m.Type == mounttypes.TypeBind { // Before creating the source directory on the host, invoke checkFun if it's not nil. One of // the use case is to forbid creating the daemon socket as a directory if the daemon is in