[release-branch.go1.12] os: consistently return PathError from RemoveAll
Fixes #30859
Updates #30491
Change-Id: If4070e5d39d8649643d7e90f6f3eb499642e25ab
Reviewed-on: https://go-review.googlesource.com/c/go/+/164720
Run-TryBot: Baokun Lee <nototon@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Reviewed-by: Emmanuel Odeke <emm.odeke@gmail.com>
(cherry picked from commit d039e12b54a73796caa913994597a9f3a73e8e87)
Reviewed-on: https://go-review.googlesource.com/c/go/+/167739
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
Reviewed-by: Baokun Lee <nototon@gmail.com>
(cherry picked from commit ff048033e4304898245d843e79ed1a0897006c6d)
diff --git a/src/os/path.go b/src/os/path.go
index 104b7ce..ba43ea3 100644
--- a/src/os/path.go
+++ b/src/os/path.go
@@ -62,6 +62,7 @@
// It removes everything it can but returns the first error
// it encounters. If the path does not exist, RemoveAll
// returns nil (no error).
+// If there is an error, it will be of type *PathError.
func RemoveAll(path string) error {
return removeAll(path)
}
diff --git a/src/os/path_unix.go b/src/os/path_unix.go
index be373a5..a08ddaf 100644
--- a/src/os/path_unix.go
+++ b/src/os/path_unix.go
@@ -51,7 +51,7 @@
// Remove leading directory path
for i--; i >= 0; i-- {
if path[i] == '/' {
- dirname = path[:i+1]
+ dirname = path[:i]
basename = path[i+1:]
break
}
diff --git a/src/os/removeall_at.go b/src/os/removeall_at.go
index 94232cf..330963b 100644
--- a/src/os/removeall_at.go
+++ b/src/os/removeall_at.go
@@ -46,13 +46,20 @@
}
defer parent.Close()
- return removeAllFrom(parent, base)
+ if err := removeAllFrom(parent, base); err != nil {
+ if pathErr, ok := err.(*PathError); ok {
+ pathErr.Path = parentDir + string(PathSeparator) + pathErr.Path
+ err = pathErr
+ }
+ return err
+ }
+ return nil
}
-func removeAllFrom(parent *File, path string) error {
+func removeAllFrom(parent *File, base string) error {
parentFd := int(parent.Fd())
// Simple case: if Unlink (aka remove) works, we're done.
- err := unix.Unlinkat(parentFd, path, 0)
+ err := unix.Unlinkat(parentFd, base, 0)
if err == nil || IsNotExist(err) {
return nil
}
@@ -64,21 +71,21 @@
// whose contents need to be removed.
// Otherwise just return the error.
if err != syscall.EISDIR && err != syscall.EPERM && err != syscall.EACCES {
- return err
+ return &PathError{"unlinkat", base, err}
}
// Is this a directory we need to recurse into?
var statInfo syscall.Stat_t
- statErr := unix.Fstatat(parentFd, path, &statInfo, unix.AT_SYMLINK_NOFOLLOW)
+ statErr := unix.Fstatat(parentFd, base, &statInfo, unix.AT_SYMLINK_NOFOLLOW)
if statErr != nil {
if IsNotExist(statErr) {
return nil
}
- return statErr
+ return &PathError{"fstatat", base, statErr}
}
if statInfo.Mode&syscall.S_IFMT != syscall.S_IFDIR {
- // Not a directory; return the error from the Remove.
- return err
+ // Not a directory; return the error from the unix.Unlinkat.
+ return &PathError{"unlinkat", base, err}
}
// Remove the directory's entries.
@@ -87,12 +94,12 @@
const request = 1024
// Open the directory to recurse into
- file, err := openFdAt(parentFd, path)
+ file, err := openFdAt(parentFd, base)
if err != nil {
if IsNotExist(err) {
return nil
}
- recurseErr = err
+ recurseErr = &PathError{"openfdat", base, err}
break
}
@@ -103,12 +110,15 @@
if IsNotExist(readErr) {
return nil
}
- return readErr
+ return &PathError{"readdirnames", base, readErr}
}
for _, name := range names {
err := removeAllFrom(file, name)
if err != nil {
+ if pathErr, ok := err.(*PathError); ok {
+ pathErr.Path = base + string(PathSeparator) + pathErr.Path
+ }
recurseErr = err
}
}
@@ -127,7 +137,7 @@
}
// Remove the directory itself.
- unlinkError := unix.Unlinkat(parentFd, path, unix.AT_REMOVEDIR)
+ unlinkError := unix.Unlinkat(parentFd, base, unix.AT_REMOVEDIR)
if unlinkError == nil || IsNotExist(unlinkError) {
return nil
}
@@ -135,7 +145,7 @@
if recurseErr != nil {
return recurseErr
}
- return unlinkError
+ return &PathError{"unlinkat", base, unlinkError}
}
// openFdAt opens path relative to the directory in fd.
@@ -157,7 +167,7 @@
continue
}
- return nil, &PathError{"openat", name, e}
+ return nil, e
}
if !supportsCloseOnExec {
diff --git a/src/os/removeall_test.go b/src/os/removeall_test.go
index 21371d8..945a38e 100644
--- a/src/os/removeall_test.go
+++ b/src/os/removeall_test.go
@@ -294,7 +294,7 @@
}
// Issue #29983.
-func TestRemoveAllButReadOnly(t *testing.T) {
+func TestRemoveAllButReadOnlyAndPathError(t *testing.T) {
switch runtime.GOOS {
case "nacl", "js", "windows":
t.Skipf("skipping test on %s", runtime.GOOS)
@@ -355,10 +355,21 @@
defer Chmod(d, 0777)
}
- if err := RemoveAll(tempDir); err == nil {
+ err = RemoveAll(tempDir)
+ if err == nil {
t.Fatal("RemoveAll succeeded unexpectedly")
}
+ // The error should be of type *PathError.
+ // see issue 30491 for details.
+ if pathErr, ok := err.(*PathError); ok {
+ if g, w := pathErr.Path, filepath.Join(tempDir, "b", "y"); g != w {
+ t.Errorf("got %q, expected pathErr.path %q", g, w)
+ }
+ } else {
+ t.Errorf("got %T, expected *os.PathError", err)
+ }
+
for _, dir := range dirs {
_, err := Stat(filepath.Join(tempDir, dir))
if inReadonly(dir) {
