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 <kolyshkin@gmail.com>
Kir Kolyshkin authored on 2017/09/26 04:39:36Showing 14 changed files
- daemon/checkpoint.go
- daemon/daemon.go
- daemon/daemon_unix.go
- daemon/daemon_windows.go
- daemon/graphdriver/aufs/aufs.go
- daemon/graphdriver/devmapper/deviceset.go
- daemon/graphdriver/devmapper/driver.go
- daemon/graphdriver/overlay/overlay.go
- daemon/graphdriver/overlay2/overlay.go
- pkg/idtools/idtools.go
- pkg/idtools/idtools_unix.go
- pkg/idtools/idtools_windows.go
- volume/local/local.go
- volume/volume.go
| ... | ... |
@@ -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 |
| ... | ... |
@@ -136,7 +136,7 @@ func Init(home string, options []string, uidMaps, gidMaps []idtools.IDMap) (grap |
| 136 | 136 |
return nil, err |
| 137 | 137 |
} |
| 138 | 138 |
// Create the driver home dir |
| 139 |
- if err := idtools.MkdirAllAndChown(home, 0700, idtools.IDPair{rootUID, rootGID}); err != nil && !os.IsExist(err) {
|
|
| 139 |
+ if err := idtools.MkdirAllAndChown(home, 0700, idtools.IDPair{rootUID, rootGID}); err != nil {
|
|
| 140 | 140 |
return nil, err |
| 141 | 141 |
} |
| 142 | 142 |
|
| ... | ... |
@@ -171,7 +171,7 @@ func Init(home string, options []string, uidMaps, gidMaps []idtools.IDMap) (grap |
| 171 | 171 |
return nil, err |
| 172 | 172 |
} |
| 173 | 173 |
// Create the driver home dir |
| 174 |
- if err := idtools.MkdirAllAndChown(path.Join(home, linkDir), 0700, idtools.IDPair{rootUID, rootGID}); err != nil && !os.IsExist(err) {
|
|
| 174 |
+ if err := idtools.MkdirAllAndChown(path.Join(home, linkDir), 0700, idtools.IDPair{rootUID, rootGID}); err != nil {
|
|
| 175 | 175 |
return nil, err |
| 176 | 176 |
} |
| 177 | 177 |
|
| ... | ... |
@@ -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 |
} |
| ... | ... |
@@ -31,7 +31,7 @@ func mkdirAs(path string, mode os.FileMode, ownerUID, ownerGID int, mkAll, chown |
| 31 | 31 |
stat, err := system.Stat(path) |
| 32 | 32 |
if err == nil {
|
| 33 | 33 |
if !stat.IsDir() {
|
| 34 |
- return &os.PathError{"mkdir", path, syscall.ENOTDIR}
|
|
| 34 |
+ return &os.PathError{Op: "mkdir", Path: path, Err: syscall.ENOTDIR}
|
|
| 35 | 35 |
} |
| 36 | 36 |
if !chownExisting {
|
| 37 | 37 |
return nil |
| ... | ... |
@@ -58,7 +58,7 @@ func mkdirAs(path string, mode os.FileMode, ownerUID, ownerGID int, mkAll, chown |
| 58 | 58 |
paths = append(paths, dirPath) |
| 59 | 59 |
} |
| 60 | 60 |
} |
| 61 |
- if err := system.MkdirAll(path, mode, ""); err != nil && !os.IsExist(err) {
|
|
| 61 |
+ if err := system.MkdirAll(path, mode, ""); err != nil {
|
|
| 62 | 62 |
return err |
| 63 | 63 |
} |
| 64 | 64 |
} else {
|
| ... | ... |
@@ -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 |
| ... | ... |
@@ -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 |