From 6f82d4b5449e7966dc7dbf0a8d0bcc70b9e26678 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Wed, 29 Jul 2015 18:01:41 -0700 Subject: [PATCH] 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. [1] https://github.com/golang/go/blob/f9ed2f75/src/os/path.go Signed-off-by: Kir Kolyshkin --- libcontainer/cgroups/fs/apply_raw.go | 2 +- libcontainer/cgroups/fs/cpuset.go | 2 +- libcontainer/cgroups/fs/memory.go | 2 +- libcontainer/cgroups/systemd/apply_systemd.go | 4 ++-- libcontainer/rootfs_linux.go | 8 ++++---- 5 files changed, 9 insertions(+), 9 deletions(-) diff --git a/libcontainer/cgroups/fs/apply_raw.go b/libcontainer/cgroups/fs/apply_raw.go index c8ea761d..f75dab1b 100644 --- a/libcontainer/cgroups/fs/apply_raw.go +++ b/libcontainer/cgroups/fs/apply_raw.go @@ -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 { diff --git a/libcontainer/cgroups/fs/cpuset.go b/libcontainer/cgroups/fs/cpuset.go index b6c04b0c..f3ec2c3e 100644 --- a/libcontainer/cgroups/fs/cpuset.go +++ b/libcontainer/cgroups/fs/cpuset.go @@ -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) diff --git a/libcontainer/cgroups/fs/memory.go b/libcontainer/cgroups/fs/memory.go index 8206b147..1fa8bd74 100644 --- a/libcontainer/cgroups/fs/memory.go +++ b/libcontainer/cgroups/fs/memory.go @@ -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 { diff --git a/libcontainer/cgroups/systemd/apply_systemd.go b/libcontainer/cgroups/systemd/apply_systemd.go index 8c27f11a..1472ba32 100644 --- a/libcontainer/cgroups/systemd/apply_systemd.go +++ b/libcontainer/cgroups/systemd/apply_systemd.go @@ -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 { @@ -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 } diff --git a/libcontainer/rootfs_linux.go b/libcontainer/rootfs_linux.go index 78e51224..88aa77d5 100644 --- a/libcontainer/rootfs_linux.go +++ b/libcontainer/rootfs_linux.go @@ -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 { @@ -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 } } @@ -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)