From c39f87a47a7d5ba0251063ba27cb49c165ad9540 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Thu, 2 Apr 2020 19:38:25 -0700 Subject: [PATCH 1/2] Revert "Merge pull request #2280 from kolyshkin/errors-unwrap" Using errors.Unwrap() is not the best thing to do, since it returns nil in case of an error which was not wrapped. More to say, errors package provides more elegant ways to check for underlying errors, such as errors.As() and errors.Is(). This reverts commit f8e138855d4a630006e88fb7476d35e10e31bf22, reversing changes made to 6ca9d8e6dae9f5a8f141cf0a3d6a26d74984b2ee. Signed-off-by: Kir Kolyshkin --- libcontainer/cgroups/fs/apply_raw.go | 16 ++++++++++++---- libcontainer/cgroups/fs/kmem.go | 10 ++++++++-- libcontainer/cgroups/fs2/pids.go | 12 +++++++++++- libcontainer/cgroups/fscommon/fscommon.go | 12 +++++++++++- libcontainer/cgroups/utils.go | 11 ++++++++++- libcontainer/container_linux.go | 5 ++++- 6 files changed, 56 insertions(+), 10 deletions(-) diff --git a/libcontainer/cgroups/fs/apply_raw.go b/libcontainer/cgroups/fs/apply_raw.go index a861bf7a4ba..ec148b48905 100644 --- a/libcontainer/cgroups/fs/apply_raw.go +++ b/libcontainer/cgroups/fs/apply_raw.go @@ -110,13 +110,21 @@ func isIgnorableError(rootless bool, err error) bool { if !rootless { return false } - err = errors.Cause(err) // Is it an ordinary EPERM? - if os.IsPermission(err) { + if os.IsPermission(errors.Cause(err)) { return true } - // Handle some specific syscall errors. - errno := errors.Unwrap(err) + + // Try to handle other errnos. + var errno error + switch err := errors.Cause(err).(type) { + case *os.PathError: + errno = err.Err + case *os.LinkError: + errno = err.Err + case *os.SyscallError: + errno = err.Err + } return errno == unix.EROFS || errno == unix.EPERM || errno == unix.EACCES } diff --git a/libcontainer/cgroups/fs/kmem.go b/libcontainer/cgroups/fs/kmem.go index 4191d9770a8..69b5a1946c7 100644 --- a/libcontainer/cgroups/fs/kmem.go +++ b/libcontainer/cgroups/fs/kmem.go @@ -6,8 +6,10 @@ import ( "errors" "fmt" "io/ioutil" + "os" "path/filepath" "strconv" + "syscall" // for Errno type only "github.com/opencontainers/runc/libcontainer/cgroups" "golang.org/x/sys/unix" @@ -47,8 +49,12 @@ func setKernelMemory(path string, kernelMemoryLimit int64) error { // The EBUSY signal is returned on attempts to write to the // memory.kmem.limit_in_bytes file if the cgroup has children or // once tasks have been attached to the cgroup - if errors.Unwrap(err) == unix.EBUSY { - return fmt.Errorf("failed to set %s, because either tasks have already joined this cgroup or it has children", cgroupKernelMemoryLimit) + if pathErr, ok := err.(*os.PathError); ok { + if errNo, ok := pathErr.Err.(syscall.Errno); ok { + if errNo == unix.EBUSY { + return fmt.Errorf("failed to set %s, because either tasks have already joined this cgroup or it has children", cgroupKernelMemoryLimit) + } + } } return fmt.Errorf("failed to write %v to %v: %v", kernelMemoryLimit, cgroupKernelMemoryLimit, err) } diff --git a/libcontainer/cgroups/fs2/pids.go b/libcontainer/cgroups/fs2/pids.go index feafce756a4..99d912cf2de 100644 --- a/libcontainer/cgroups/fs2/pids.go +++ b/libcontainer/cgroups/fs2/pids.go @@ -4,6 +4,7 @@ package fs2 import ( "io/ioutil" + "os" "path/filepath" "strings" @@ -24,11 +25,20 @@ func setPids(dirPath string, cgroup *configs.Cgroup) error { return nil } +func isNOTSUP(err error) bool { + switch err := err.(type) { + case *os.PathError: + return err.Err == unix.ENOTSUP + default: + return false + } +} + func statPidsWithoutController(dirPath string, stats *cgroups.Stats) error { // if the controller is not enabled, let's read PIDS from cgroups.procs // (or threads if cgroup.threads is enabled) contents, err := ioutil.ReadFile(filepath.Join(dirPath, "cgroup.procs")) - if err != nil && errors.Unwrap(err) == unix.ENOTSUP { + if err != nil && isNOTSUP(err) { contents, err = ioutil.ReadFile(filepath.Join(dirPath, "cgroup.threads")) } if err != nil { diff --git a/libcontainer/cgroups/fscommon/fscommon.go b/libcontainer/cgroups/fscommon/fscommon.go index cd3d9d9b227..dc53987e0a2 100644 --- a/libcontainer/cgroups/fscommon/fscommon.go +++ b/libcontainer/cgroups/fscommon/fscommon.go @@ -41,10 +41,20 @@ func ReadFile(dir, file string) (string, error) { func retryingWriteFile(filename string, data []byte, perm os.FileMode) error { for { err := ioutil.WriteFile(filename, data, perm) - if errors.Unwrap(err) == syscall.EINTR { + if isInterruptedWriteFile(err) { logrus.Infof("interrupted while writing %s to %s", string(data), filename) continue } return err } } + +func isInterruptedWriteFile(err error) bool { + if patherr, ok := err.(*os.PathError); ok { + errno, ok2 := patherr.Err.(syscall.Errno) + if ok2 && errno == syscall.EINTR { + return true + } + } + return false +} diff --git a/libcontainer/cgroups/utils.go b/libcontainer/cgroups/utils.go index 8a53cb09ad4..7808c0936ba 100644 --- a/libcontainer/cgroups/utils.go +++ b/libcontainer/cgroups/utils.go @@ -576,7 +576,7 @@ func WriteCgroupProc(dir string, pid int) error { // EINVAL might mean that the task being added to cgroup.procs is in state // TASK_NEW. We should attempt to do so again. - if errors.Unwrap(err) == unix.EINVAL { + if isEINVAL(err) { time.Sleep(30 * time.Millisecond) continue } @@ -586,6 +586,15 @@ func WriteCgroupProc(dir string, pid int) error { return err } +func isEINVAL(err error) bool { + switch err := err.(type) { + case *os.PathError: + return err.Err == unix.EINVAL + default: + return false + } +} + // Since the OCI spec is designed for cgroup v1, in some cases // there is need to convert from the cgroup v1 configuration to cgroup v2 // the formula for BlkIOWeight is y = (1 + (x - 10) * 9999 / 990) diff --git a/libcontainer/container_linux.go b/libcontainer/container_linux.go index cd5a4f4ec00..43b7219ce93 100644 --- a/libcontainer/container_linux.go +++ b/libcontainer/container_linux.go @@ -1855,7 +1855,10 @@ func (c *linuxContainer) isPaused() (bool, error) { data, err := ioutil.ReadFile(filepath.Join(fcg, filename)) if err != nil { // If freezer cgroup is not mounted, the container would just be not paused. - if os.IsNotExist(err) || errors.Unwrap(err) == syscall.ENODEV { + if os.IsNotExist(err) { + return false, nil + } + if pathError, isPathError := err.(*os.PathError); isPathError && pathError.Err == syscall.ENODEV { return false, nil } return false, newSystemErrorWithCause(err, "checking if container is paused") From b2272b2cba97817be7c0f173bbed9ab1d95e5349 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Thu, 2 Apr 2020 19:48:30 -0700 Subject: [PATCH 2/2] libcontainer: use errors.Is() and errors.As() Make use of errors.Is() and errors.As() where appropriate to check the underlying error. The biggest motivation is to simplify the code. The feature requires go 1.13 but since merging #2256 we are already not supporting go 1.12 (which is an unsupported release anyway). Signed-off-by: Kir Kolyshkin --- libcontainer/cgroups/fs/apply_raw.go | 21 +++++++++------------ libcontainer/cgroups/fs/kmem.go | 10 ++-------- libcontainer/cgroups/fs2/pids.go | 12 +----------- libcontainer/cgroups/utils.go | 11 +---------- libcontainer/container_linux.go | 5 +---- 5 files changed, 14 insertions(+), 45 deletions(-) diff --git a/libcontainer/cgroups/fs/apply_raw.go b/libcontainer/cgroups/fs/apply_raw.go index ec148b48905..df83d41c353 100644 --- a/libcontainer/cgroups/fs/apply_raw.go +++ b/libcontainer/cgroups/fs/apply_raw.go @@ -8,6 +8,7 @@ import ( "os" "path/filepath" "sync" + "syscall" "github.com/opencontainers/runc/libcontainer/cgroups" "github.com/opencontainers/runc/libcontainer/configs" @@ -110,22 +111,18 @@ func isIgnorableError(rootless bool, err error) bool { if !rootless { return false } + // TODO: rm errors.Cause once we switch to %w everywhere + err = errors.Cause(err) // Is it an ordinary EPERM? - if os.IsPermission(errors.Cause(err)) { + if errors.Is(err, os.ErrPermission) { return true } - - // Try to handle other errnos. - var errno error - switch err := errors.Cause(err).(type) { - case *os.PathError: - errno = err.Err - case *os.LinkError: - errno = err.Err - case *os.SyscallError: - errno = err.Err + // Handle some specific syscall errors. + var errno syscall.Errno + if errors.As(err, &errno) { + return errno == unix.EROFS || errno == unix.EPERM || errno == unix.EACCES } - return errno == unix.EROFS || errno == unix.EPERM || errno == unix.EACCES + return false } func (m *Manager) getSubsystems() subsystemSet { diff --git a/libcontainer/cgroups/fs/kmem.go b/libcontainer/cgroups/fs/kmem.go index 69b5a1946c7..8d97a6791a4 100644 --- a/libcontainer/cgroups/fs/kmem.go +++ b/libcontainer/cgroups/fs/kmem.go @@ -6,10 +6,8 @@ import ( "errors" "fmt" "io/ioutil" - "os" "path/filepath" "strconv" - "syscall" // for Errno type only "github.com/opencontainers/runc/libcontainer/cgroups" "golang.org/x/sys/unix" @@ -49,12 +47,8 @@ func setKernelMemory(path string, kernelMemoryLimit int64) error { // The EBUSY signal is returned on attempts to write to the // memory.kmem.limit_in_bytes file if the cgroup has children or // once tasks have been attached to the cgroup - if pathErr, ok := err.(*os.PathError); ok { - if errNo, ok := pathErr.Err.(syscall.Errno); ok { - if errNo == unix.EBUSY { - return fmt.Errorf("failed to set %s, because either tasks have already joined this cgroup or it has children", cgroupKernelMemoryLimit) - } - } + if errors.Is(err, unix.EBUSY) { + return fmt.Errorf("failed to set %s, because either tasks have already joined this cgroup or it has children", cgroupKernelMemoryLimit) } return fmt.Errorf("failed to write %v to %v: %v", kernelMemoryLimit, cgroupKernelMemoryLimit, err) } diff --git a/libcontainer/cgroups/fs2/pids.go b/libcontainer/cgroups/fs2/pids.go index 99d912cf2de..0cf860f9c7c 100644 --- a/libcontainer/cgroups/fs2/pids.go +++ b/libcontainer/cgroups/fs2/pids.go @@ -4,7 +4,6 @@ package fs2 import ( "io/ioutil" - "os" "path/filepath" "strings" @@ -25,20 +24,11 @@ func setPids(dirPath string, cgroup *configs.Cgroup) error { return nil } -func isNOTSUP(err error) bool { - switch err := err.(type) { - case *os.PathError: - return err.Err == unix.ENOTSUP - default: - return false - } -} - func statPidsWithoutController(dirPath string, stats *cgroups.Stats) error { // if the controller is not enabled, let's read PIDS from cgroups.procs // (or threads if cgroup.threads is enabled) contents, err := ioutil.ReadFile(filepath.Join(dirPath, "cgroup.procs")) - if err != nil && isNOTSUP(err) { + if errors.Is(err, unix.ENOTSUP) { contents, err = ioutil.ReadFile(filepath.Join(dirPath, "cgroup.threads")) } if err != nil { diff --git a/libcontainer/cgroups/utils.go b/libcontainer/cgroups/utils.go index 7808c0936ba..51b743e2ce3 100644 --- a/libcontainer/cgroups/utils.go +++ b/libcontainer/cgroups/utils.go @@ -576,7 +576,7 @@ func WriteCgroupProc(dir string, pid int) error { // EINVAL might mean that the task being added to cgroup.procs is in state // TASK_NEW. We should attempt to do so again. - if isEINVAL(err) { + if errors.Is(err, unix.EINVAL) { time.Sleep(30 * time.Millisecond) continue } @@ -586,15 +586,6 @@ func WriteCgroupProc(dir string, pid int) error { return err } -func isEINVAL(err error) bool { - switch err := err.(type) { - case *os.PathError: - return err.Err == unix.EINVAL - default: - return false - } -} - // Since the OCI spec is designed for cgroup v1, in some cases // there is need to convert from the cgroup v1 configuration to cgroup v2 // the formula for BlkIOWeight is y = (1 + (x - 10) * 9999 / 990) diff --git a/libcontainer/container_linux.go b/libcontainer/container_linux.go index 43b7219ce93..b02df818924 100644 --- a/libcontainer/container_linux.go +++ b/libcontainer/container_linux.go @@ -1855,10 +1855,7 @@ func (c *linuxContainer) isPaused() (bool, error) { data, err := ioutil.ReadFile(filepath.Join(fcg, filename)) if err != nil { // If freezer cgroup is not mounted, the container would just be not paused. - if os.IsNotExist(err) { - return false, nil - } - if pathError, isPathError := err.(*os.PathError); isPathError && pathError.Err == syscall.ENODEV { + if os.IsNotExist(err) || errors.Is(err, syscall.ENODEV) { return false, nil } return false, newSystemErrorWithCause(err, "checking if container is paused")