os: consistently return PathError from RemoveAll

Fixes #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>

diff --git a/src/os/path.go b/src/os/path.go
index 104b7ceaf7dad37e58d852b954036ed9a790ba11..ba43ea352547b9e4018df73c55b2b8d9a6f9bc90 100644 (file)
--- a/src/os/path.go
+++ b/src/os/path.go
@@ -62,6 +62,7 @@ func MkdirAll(path string, perm FileMode) error {
 // 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 be373a50a919353b7dd590231d91600123e21608..a08ddaf6dbdcba9da55e22bd0c09b56c0293f994 100644 (file)
--- a/src/os/path_unix.go
+++ b/src/os/path_unix.go
@@ -51,7 +51,7 @@ func splitPath(path string) (string, string) {
        // 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 94232cf5564b6090d36e0d90bd6e7ce18e87afce..330963b3541159602ea7ec6ee06604b02fcb291e 100644 (file)
--- a/src/os/removeall_at.go
+++ b/src/os/removeall_at.go
@@ -46,13 +46,20 @@ func removeAll(path string) error {
        }
        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 @@ func removeAllFrom(parent *File, path string) error {
        // 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 @@ func removeAllFrom(parent *File, path string) error {
                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 @@ func removeAllFrom(parent *File, path string) error {
                        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 @@ func removeAllFrom(parent *File, path string) error {
        }
 
        // 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 @@ func removeAllFrom(parent *File, path string) error {
        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 @@ func openFdAt(dirfd int, name string) (*File, error) {
                        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 8690bb5d2af38e6279048a7cd9f159f55f78f250..2bd14979e07a632e98c63582b126c316fc3cf011 100644 (file)
--- a/src/os/removeall_test.go
+++ b/src/os/removeall_test.go
@@ -286,7 +286,7 @@ func TestRemoveReadOnlyDir(t *testing.T) {
 }
 
 // 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)
@@ -347,10 +347,21 @@ func TestRemoveAllButReadOnly(t *testing.T) {
                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) {