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 opencontainers#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 opencontainers#1, IsExist check after MkdirAll is not needed.

Because of opencontainers#2 and opencontainers#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 <kir@openvz.org>
  • Loading branch information
kolyshkin authored and sallyom committed Jul 30, 2015
1 parent 6340843 commit cf4fe64
Show file tree
Hide file tree
Showing 5 changed files with 9 additions and 9 deletions.
2 changes: 1 addition & 1 deletion libcontainer/cgroups/fs/apply_raw.go
Original file line number Diff line number Diff line change
Expand Up @@ -272,7 +272,7 @@ func (raw *data) join(subsystem string) (string, error) {
if err != nil {
return "", err
}
if err := os.MkdirAll(path, 0755); err != nil && !os.IsExist(err) {
if err := os.MkdirAll(path, 0755); err != nil {
return "", err
}
if err := writeFile(path, CgroupProcesses, strconv.Itoa(raw.pid)); err != nil {
Expand Down
2 changes: 1 addition & 1 deletion libcontainer/cgroups/fs/cpuset.go
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ func (s *CpusetGroup) ensureParent(current, root string) error {
if err := s.ensureParent(parent, root); err != nil {
return err
}
if err := os.MkdirAll(current, 0755); err != nil && !os.IsExist(err) {
if err := os.MkdirAll(current, 0755); err != nil {
return err
}
return s.copyIfNeeded(current, parent)
Expand Down
2 changes: 1 addition & 1 deletion libcontainer/cgroups/fs/memory.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ func (s *MemoryGroup) Apply(d *data) error {
}
return err
}
if err := os.MkdirAll(path, 0755); err != nil && !os.IsExist(err) {
if err := os.MkdirAll(path, 0755); err != nil {
return err
}
if err := s.Set(path, d.c); err != nil {
Expand Down
4 changes: 2 additions & 2 deletions libcontainer/cgroups/systemd/apply_systemd.go
Original file line number Diff line number Diff line change
Expand Up @@ -298,7 +298,7 @@ func join(c *configs.Cgroup, subsystem string, pid int) (string, error) {
if err != nil {
return "", err
}
if err := os.MkdirAll(path, 0755); err != nil && !os.IsExist(err) {
if err := os.MkdirAll(path, 0755); err != nil {
return "", err
}
if err := writeFile(path, "cgroup.procs", strconv.Itoa(pid)); err != nil {
Expand Down Expand Up @@ -478,7 +478,7 @@ func setKernelMemory(c *configs.Cgroup) error {
return err
}

if err := os.MkdirAll(path, 0755); err != nil && !os.IsExist(err) {
if err := os.MkdirAll(path, 0755); err != nil {
return err
}

Expand Down
8 changes: 4 additions & 4 deletions libcontainer/rootfs_linux.go
Original file line number Diff line number Diff line change
Expand Up @@ -98,12 +98,12 @@ func mountToRootfs(m *configs.Mount, rootfs, mountLabel string) error {

switch m.Device {
case "proc", "sysfs":
if err := os.MkdirAll(dest, 0755); err != nil && !os.IsExist(err) {
if err := os.MkdirAll(dest, 0755); err != nil {
return err
}
return syscall.Mount(m.Source, dest, m.Device, uintptr(m.Flags), "")
case "mqueue":
if err := os.MkdirAll(dest, 0755); err != nil && !os.IsExist(err) {
if err := os.MkdirAll(dest, 0755); err != nil {
return err
}
if err := syscall.Mount(m.Source, dest, m.Device, uintptr(m.Flags), ""); err != nil {
Expand All @@ -113,7 +113,7 @@ func mountToRootfs(m *configs.Mount, rootfs, mountLabel string) error {
case "tmpfs":
stat, err := os.Stat(dest)
if err != nil {
if err := os.MkdirAll(dest, 0755); err != nil && !os.IsExist(err) {
if err := os.MkdirAll(dest, 0755); err != nil {
return err
}
}
Expand All @@ -127,7 +127,7 @@ func mountToRootfs(m *configs.Mount, rootfs, mountLabel string) error {
}
return nil
case "devpts":
if err := os.MkdirAll(dest, 0755); err != nil && !os.IsExist(err) {
if err := os.MkdirAll(dest, 0755); err != nil {
return err
}
return syscall.Mount(m.Source, dest, m.Device, uintptr(m.Flags), data)
Expand Down

0 comments on commit cf4fe64

Please sign in to comment.