From a3de74b0d27eedbceb85a4f573b0c0a9f499497b Mon Sep 17 00:00:00 2001 From: Yahli Ramberg Date: Mon, 3 Aug 2026 12:41:14 +0300 Subject: [PATCH] fix: report the status describing a filesystem error, not always EIO A Filesystem's only channel to the client is the error it returns, and call sites hard-coded the status for it: NFSStatusIO, or NFSStatusAccess for the write operations. So "read-only filesystem", "out of space", "over quota" and "directory not empty" all reached the user as a bare EIO, which says nothing about what went wrong and invites the client to retry work that cannot succeed. Map the error to its status instead. statusFromError matches with errors.Is, so a *PathError or a filesystem's own added context still resolves, and each call site keeps its previous status as the fallback for errors we don't recognize -- unrecognized errors report exactly what they did before. This also replaces the os.IsNotExist/os.IsPermission ladders repeated at 16 call sites. Those used os.Is*, which does not see through wrapped errors; the mapper does, so a wrapped ErrNotExist from a user-supplied OpenDir now reports NOENT rather than EIO. Signed-off-by: Yahli Ramberg --- errors.go | 53 ++++++++++++++++++++++++++++++++++++++++++++++++ errors_test.go | 17 ++++++++++++++++ nfs_oncreate.go | 8 ++++---- nfs_ongetattr.go | 6 +----- nfs_onlink.go | 6 +++--- nfs_onmkdir.go | 6 +++--- nfs_onread.go | 2 +- nfs_onremove.go | 46 ++++++++--------------------------------- nfs_onrename.go | 18 +++------------- nfs_onsymlink.go | 6 +++--- nfs_onwrite.go | 11 ++++------ nfs_test.go | 36 ++++++++++++++++++++++++++++++++ 12 files changed, 136 insertions(+), 79 deletions(-) create mode 100644 errors_test.go diff --git a/errors.go b/errors.go index af08be6..aa26325 100644 --- a/errors.go +++ b/errors.go @@ -5,6 +5,8 @@ import ( "encoding/binary" "errors" "fmt" + "io/fs" + "syscall" ) // RPCError provides the error interface for errors thrown by @@ -196,6 +198,57 @@ func (s *NFSStatusError) Unwrap() error { return s.WrappedErr } +// statusFromError translates err into the NFS status describing it, returning +// fallback for an error it does not recognize. +func statusFromError(err error, fallback NFSStatus) NFSStatus { + switch { + case err == nil: + return NFSStatusOk + case errors.Is(err, syscall.EROFS): + return NFSStatusROFS + case errors.Is(err, syscall.ENOSPC): + return NFSStatusNoSPC + case errors.Is(err, syscall.EDQUOT): + return NFSStatusDQuot + case errors.Is(err, syscall.ENOTEMPTY): + return NFSStatusNotEmpty + case errors.Is(err, syscall.EXDEV): + return NFSStatusXDev + case errors.Is(err, syscall.EISDIR): + return NFSStatusIsDir + case errors.Is(err, syscall.ENOTDIR): + return NFSStatusNotDir + case errors.Is(err, syscall.ENAMETOOLONG): + return NFSStatusNameTooLong + case errors.Is(err, syscall.EMLINK): + return NFSStatusMlink + case errors.Is(err, syscall.EFBIG): + return NFSStatusFBig + case errors.Is(err, syscall.ENXIO): + return NFSStatusNXIO + case errors.Is(err, syscall.EPERM): + return NFSStatusPerm + // ENOTEMPTY is also fs.ErrExist and EPERM is also fs.ErrPermission, so these + // sentinels come last. + case errors.Is(err, fs.ErrNotExist): + return NFSStatusNoEnt + case errors.Is(err, fs.ErrExist): + return NFSStatusExist + case errors.Is(err, fs.ErrPermission): + return NFSStatusAccess + case errors.Is(err, fs.ErrInvalid): + return NFSStatusInval + default: + return fallback + } +} + +// statusError wraps err in the NFSStatusError describing it, falling back to +// the given status when the error is unrecognized. +func statusError(err error, fallback NFSStatus) *NFSStatusError { + return &NFSStatusError{statusFromError(err, fallback), err} +} + // StatusErrorWithBody is an NFS error with a payload. type StatusErrorWithBody struct { NFSStatusError diff --git a/errors_test.go b/errors_test.go new file mode 100644 index 0000000..9e0016c --- /dev/null +++ b/errors_test.go @@ -0,0 +1,17 @@ +package nfs + +import ( + "errors" + "testing" +) + +func TestStatusErrorKeepsFallback(t *testing.T) { + err := errors.New("backend exploded") + statusErr := statusError(err, NFSStatusAccess) + if statusErr.NFSStatus != NFSStatusAccess { + t.Errorf("status = %v, want %v", statusErr.NFSStatus, NFSStatusAccess) + } + if !errors.Is(statusErr, err) { + t.Errorf("statusError dropped the wrapped error: %v", statusErr) + } +} diff --git a/nfs_oncreate.go b/nfs_oncreate.go index bce72b5..f18bbfc 100644 --- a/nfs_oncreate.go +++ b/nfs_oncreate.go @@ -70,7 +70,7 @@ func onCreate(ctx context.Context, w *response, userHandle Handler) error { } } else { if s, err := fs.Stat(fs.Join(path...)); err != nil { - return &NFSStatusError{NFSStatusAccess, err} + return statusError(err, NFSStatusAccess) } else if !s.IsDir() { return &NFSStatusError{NFSStatusNotDir, nil} } @@ -79,18 +79,18 @@ func onCreate(ctx context.Context, w *response, userHandle Handler) error { file, err := fs.Create(newFilePath) if err != nil { Log.Errorf("Error Creating: %v", err) - return &NFSStatusError{NFSStatusAccess, err} + return statusError(err, NFSStatusAccess) } if err := file.Close(); err != nil { Log.Errorf("Error Creating: %v", err) - return &NFSStatusError{NFSStatusAccess, err} + return statusError(err, NFSStatusAccess) } fp := userHandle.ToHandle(fs, newFile) changer := userHandle.Change(fs) if err := attrs.Apply(changer, fs, newFilePath); err != nil { Log.Errorf("Error applying attributes: %v\n", err) - return &NFSStatusError{NFSStatusIO, err} + return statusError(err, NFSStatusIO) } writer := bytes.NewBuffer([]byte{}) diff --git a/nfs_ongetattr.go b/nfs_ongetattr.go index 5ecc240..6e64b92 100644 --- a/nfs_ongetattr.go +++ b/nfs_ongetattr.go @@ -3,7 +3,6 @@ package nfs import ( "bytes" "context" - "os" "github.com/willscott/go-nfs-client/nfs/xdr" ) @@ -22,10 +21,7 @@ func onGetAttr(ctx context.Context, w *response, userHandle Handler) error { fullPath := fs.Join(path...) info, err := fs.Lstat(fullPath) if err != nil { - if os.IsNotExist(err) { - return &NFSStatusError{NFSStatusNoEnt, err} - } - return &NFSStatusError{NFSStatusIO, err} + return statusError(err, NFSStatusIO) } attr := ToFileAttribute(info, fullPath) diff --git a/nfs_onlink.go b/nfs_onlink.go index 3af44c5..2a2a1c7 100644 --- a/nfs_onlink.go +++ b/nfs_onlink.go @@ -44,7 +44,7 @@ func onLink(ctx context.Context, w *response, userHandle Handler) error { return &NFSStatusError{NFSStatusExist, os.ErrExist} } if s, err := fs.Stat(fs.Join(path...)); err != nil { - return &NFSStatusError{NFSStatusAccess, err} + return statusError(err, NFSStatusAccess) } else if !s.IsDir() { return &NFSStatusError{NFSStatusNotDir, nil} } @@ -61,10 +61,10 @@ func onLink(ctx context.Context, w *response, userHandle Handler) error { err = cos.Link(string(target), newFilePath) if err != nil { - return &NFSStatusError{NFSStatusAccess, err} + return statusError(err, NFSStatusAccess) } if err := attrs.Apply(changer, fs, newFilePath); err != nil { - return &NFSStatusError{NFSStatusIO, err} + return statusError(err, NFSStatusIO) } writer := bytes.NewBuffer([]byte{}) diff --git a/nfs_onmkdir.go b/nfs_onmkdir.go index 2bb5598..e6787a2 100644 --- a/nfs_onmkdir.go +++ b/nfs_onmkdir.go @@ -49,21 +49,21 @@ func onMkdir(ctx context.Context, w *response, userHandle Handler) error { } } else { if s, err := fs.Stat(fs.Join(path...)); err != nil { - return &NFSStatusError{NFSStatusAccess, err} + return statusError(err, NFSStatusAccess) } else if !s.IsDir() { return &NFSStatusError{NFSStatusNotDir, nil} } } if err := fs.MkdirAll(newFolderPath, attrs.Mode(mkdirDefaultMode)); err != nil { - return &NFSStatusError{NFSStatusAccess, err} + return statusError(err, NFSStatusAccess) } fp := userHandle.ToHandle(fs, newFolder) changer := userHandle.Change(fs) if changer != nil { if err := attrs.Apply(changer, fs, newFolderPath); err != nil { - return &NFSStatusError{NFSStatusIO, err} + return statusError(err, NFSStatusIO) } } diff --git a/nfs_onread.go b/nfs_onread.go index b34949e..e63bd74 100644 --- a/nfs_onread.go +++ b/nfs_onread.go @@ -69,7 +69,7 @@ func onRead(ctx context.Context, w *response, userHandle Handler) error { // todo: multiple reads if size isn't full cnt, err := fh.ReadAt(resp.Data, int64(obj.Offset)) if err != nil && !errors.Is(err, io.EOF) { - return &NFSStatusError{NFSStatusIO, err} + return statusError(err, NFSStatusIO) } resp.Count = uint32(cnt) resp.Data = resp.Data[:resp.Count] diff --git a/nfs_onremove.go b/nfs_onremove.go index bf8df12..60caf8f 100644 --- a/nfs_onremove.go +++ b/nfs_onremove.go @@ -3,7 +3,6 @@ package nfs import ( "bytes" "context" - "errors" "os" "github.com/go-git/go-billy/v6" @@ -62,14 +61,8 @@ func onRemoveObj(ctx context.Context, w *response, userHandle Handler, directory fullPath := fs.Join(path...) dirInfo, err := fs.Stat(fullPath) - if os.IsNotExist(err) { - return &NFSStatusError{NFSStatusNoEnt, err} - } - if os.IsPermission(err) { - return &NFSStatusError{NFSStatusAccess, err} - } if err != nil { - return &NFSStatusError{NFSStatusIO, err} + return statusError(err, NFSStatusIO) } if !dirInfo.IsDir() { return &NFSStatusError{NFSStatusNotDir, nil} @@ -86,16 +79,10 @@ func onRemoveObj(ctx context.Context, w *response, userHandle Handler, directory // Lstat, not Stat: POSIX rmdir()/unlink() act on the entry itself, not what it points to : // rmdir() must reject a symlink to an empty directory, unlink() must remove the symlink itself. targetInfo, err := fs.Lstat(toDelete) - if os.IsNotExist(err) { - return &NFSStatusError{NFSStatusNoEnt, err} - } - if os.IsPermission(err) { + if err != nil { // A permission error here means an ancestor directory in toDelete's path // lacks its executable permission, not that toDelete itself is inaccessible. - return &NFSStatusError{NFSStatusAccess, err} - } - if err != nil { - return &NFSStatusError{NFSStatusIO, err} + return statusError(err, NFSStatusIO) } if directory && !targetInfo.IsDir() { @@ -107,17 +94,8 @@ func onRemoveObj(ctx context.Context, w *response, userHandle Handler, directory if directory { empty, err := isEmptyDir(ctx, userHandle, fs, toDelete) - // This error can come from a user-supplied OpenDir, which may return - // a wrapped os.ErrNotExist/os.ErrPermission. errors.Is sees through the - // wrapping; the os.IsNotExist/os.IsPermission checks elsewhere do not. - if errors.Is(err, os.ErrNotExist) { - return &NFSStatusError{NFSStatusNoEnt, err} - } - if errors.Is(err, os.ErrPermission) { - return &NFSStatusError{NFSStatusAccess, err} - } if err != nil { - return &NFSStatusError{NFSStatusIO, err} + return statusError(err, NFSStatusIO) } if !empty { return &NFSStatusError{NFSStatusNotEmpty, nil} @@ -125,20 +103,12 @@ func onRemoveObj(ctx context.Context, w *response, userHandle Handler, directory } err = fs.Remove(toDelete) - if os.IsNotExist(err) { - return &NFSStatusError{NFSStatusNoEnt, err} - } - if os.IsPermission(err) { - return &NFSStatusError{NFSStatusAccess, err} - } if err != nil { - // We passed all directory/empty checks above, so an error here likely means - // the target changed underneath us (the TOCTOU window noted above) - e.g. a - // directory that just became non-empty. We report NFSStatusIO rather than - // trying to distinguish that case, since billy doesn't expose a portable way - // to recognize it. + // The checks above passed, so an error here means the target changed + // underneath us in the TOCTOU window - a directory that became non-empty, + // say. Log.Errorf("remove %q after passing directory checks: %v", toDelete, err) - return &NFSStatusError{NFSStatusIO, err} + return statusError(err, NFSStatusIO) } if err := userHandle.InvalidateHandle(fs, toDeleteHandle); err != nil { diff --git a/nfs_onrename.go b/nfs_onrename.go index 9be9c4f..a01951c 100644 --- a/nfs_onrename.go +++ b/nfs_onrename.go @@ -48,10 +48,7 @@ func onRename(ctx context.Context, w *response, userHandle Handler) error { fromDirPath := fs.Join(fromPath...) fromDirInfo, err := fs.Stat(fromDirPath) if err != nil { - if os.IsNotExist(err) { - return &NFSStatusError{NFSStatusNoEnt, err} - } - return &NFSStatusError{NFSStatusIO, err} + return statusError(err, NFSStatusIO) } if !fromDirInfo.IsDir() { return &NFSStatusError{NFSStatusNotDir, nil} @@ -61,10 +58,7 @@ func onRename(ctx context.Context, w *response, userHandle Handler) error { toDirPath := fs.Join(toPath...) toDirInfo, err := fs.Stat(toDirPath) if err != nil { - if os.IsNotExist(err) { - return &NFSStatusError{NFSStatusNoEnt, err} - } - return &NFSStatusError{NFSStatusIO, err} + return statusError(err, NFSStatusIO) } if !toDirInfo.IsDir() { return &NFSStatusError{NFSStatusNotDir, nil} @@ -78,13 +72,7 @@ func onRename(ctx context.Context, w *response, userHandle Handler) error { err = fs.Rename(fromLoc, toLoc) if err != nil { - if os.IsNotExist(err) { - return &NFSStatusError{NFSStatusNoEnt, err} - } - if os.IsPermission(err) { - return &NFSStatusError{NFSStatusAccess, err} - } - return &NFSStatusError{NFSStatusIO, err} + return statusError(err, NFSStatusIO) } if err := userHandle.InvalidateHandle(fs, oldHandle); err != nil { diff --git a/nfs_onsymlink.go b/nfs_onsymlink.go index 4495c11..730a79b 100644 --- a/nfs_onsymlink.go +++ b/nfs_onsymlink.go @@ -43,21 +43,21 @@ func onSymlink(ctx context.Context, w *response, userHandle Handler) error { return &NFSStatusError{NFSStatusExist, os.ErrExist} } if s, err := fs.Stat(fs.Join(path...)); err != nil { - return &NFSStatusError{NFSStatusAccess, err} + return statusError(err, NFSStatusAccess) } else if !s.IsDir() { return &NFSStatusError{NFSStatusNotDir, nil} } err = fs.Symlink(string(target), newFilePath) if err != nil { - return &NFSStatusError{NFSStatusAccess, err} + return statusError(err, NFSStatusAccess) } fp := userHandle.ToHandle(fs, append(path, string(obj.Filename))) changer := userHandle.Change(fs) if changer != nil { if err := attrs.Apply(changer, fs, newFilePath); err != nil { - return &NFSStatusError{NFSStatusIO, err} + return statusError(err, NFSStatusIO) } } diff --git a/nfs_onwrite.go b/nfs_onwrite.go index 31b227d..27d0bef 100644 --- a/nfs_onwrite.go +++ b/nfs_onwrite.go @@ -52,10 +52,7 @@ func onWrite(ctx context.Context, w *response, userHandle Handler) error { fullPath := fs.Join(path...) info, err := fs.Stat(fullPath) if err != nil { - if os.IsNotExist(err) { - return &NFSStatusError{NFSStatusNoEnt, err} - } - return &NFSStatusError{NFSStatusAccess, err} + return statusError(err, NFSStatusAccess) } if !info.Mode().IsRegular() { return &NFSStatusError{NFSStatusInval, os.ErrInvalid} @@ -65,7 +62,7 @@ func onWrite(ctx context.Context, w *response, userHandle Handler) error { // now the actual op. file, err := fs.OpenFile(fs.Join(path...), os.O_RDWR, info.Mode().Perm()) if err != nil { - return &NFSStatusError{NFSStatusAccess, err} + return statusError(err, NFSStatusAccess) } defer func() { if file != nil { @@ -84,13 +81,13 @@ func onWrite(ctx context.Context, w *response, userHandle Handler) error { var writtenCount int if writtenCount, err = file.WriteAt(req.Data[:end], int64(req.Offset)); err != nil { - return &NFSStatusError{NFSStatusIO, err} + return statusError(err, NFSStatusIO) } err = file.Close() file = nil // No more need to close on exit. if err != nil { Log.Errorf("error closing: %v", err) - return &NFSStatusError{NFSStatusIO, err} + return statusError(err, NFSStatusIO) } writer := bytes.NewBuffer([]byte{}) diff --git a/nfs_test.go b/nfs_test.go index 69d5c5a..8828be2 100644 --- a/nfs_test.go +++ b/nfs_test.go @@ -580,3 +580,39 @@ func readDir(target *nfsc.Target, dir string) ([]*readDirEntry, error) { return entries, nil } + +// erofsRenameFS is writable but fails every rename with EROFS. +type erofsRenameFS struct { + billy.Filesystem +} + +func (fs *erofsRenameFS) Rename(oldpath, newpath string) error { + return &os.PathError{Op: "rename", Path: oldpath, Err: syscall.EROFS} +} + +// TestOperationErrorKeepsItsStatus renames on a filesystem that reports itself +// writable but fails the rename with EROFS, and expects NFS3ERR_ROFS. +func TestOperationErrorKeepsItsStatus(t *testing.T) { + mem := memfs.New() + f, err := mem.Create("/from.txt") + if err != nil { + t.Fatal(err) + } + f.Close() + + handler := helpers.NewCachingHandler(helpers.NewNullAuthHandler(&erofsRenameFS{mem}), testCacheLimit) + target := serveAndMount(t, handler) + + err = target.Rename("/from.txt", "/to.txt") + if err == nil { + t.Fatal("expected rename to fail") + } + nfsErr, ok := err.(*nfsc.Error) + if !ok { + t.Fatalf("expected *nfsc.Error, got %T: %v", err, err) + } + if nfsErr.ErrorNum != uint32(nfs.NFSStatusROFS) { + t.Errorf("rename on read-only target reported %v (%d), want NFS3ERR_ROFS (%d)", + nfsErr.ErrorString, nfsErr.ErrorNum, nfs.NFSStatusROFS) + } +}