Map RENAME and READLINK failures to the NFS statuses that name them

[djeebus]

Problem

Two handlers discard the reason the filesystem gave them, so conditions a client could act on arrive as NFS3ERR_IO — which a client can only pass on as EIO, indistinguishable from a failing disk, and which callers further up have nothing better to do with than a 500.

onRename tested only os.IsNotExist and os.IsPermission. Everything else fell through to NFS3ERR_IO, including every case RFC 1813 §3.3.14 gives RENAME a status for:

rename(2) reports	was	now
ENOTEMPTY (destination directory holds entries)	NFS3ERR_IO	NFS3ERR_NOTEMPTY
EEXIST	NFS3ERR_IO	NFS3ERR_EXIST
EISDIR (file onto a directory)	NFS3ERR_IO	NFS3ERR_ISDIR
ENOTDIR (directory onto a file)	NFS3ERR_IO	NFS3ERR_NOTDIR
EXDEV	NFS3ERR_IO	NFS3ERR_XDEV
EROFS / ENOSPC / EDQUOT / EMLINK / ENAMETOOLONG / EINVAL	NFS3ERR_IO	the matching status

onReadLink decided its status from a Stat of the same name before looking at the readlink error at all:

if info, err := fs.Stat(fs.Join(path...)); err == nil {
    if info.Mode()&os.ModeSymlink == 0 {
        return &NFSStatusError{NFSStatusInval, err}   // err is the (nil) Stat error
    }
}
if os.IsNotExist(err) { ... }
return &NFSStatusError{NFSStatusAccess, err}          // everything else

readlink(2) already reports EINVAL for a name that is not a symlink, so the errno was thrown away in favour of a guess — and the guess is wrong on any filesystem whose Stat follows the link, where the mode is never ModeSymlink and the handler fell through to NFS3ERR_ACCES. EINVAL, ENOTDIR, EACCES and EIO all reached the client as the wrong status; on a symlink even ENOENT did. The Inval branch also wrapped the nil Stat error instead of the readlink error, so the cause never reached the server log.

Fix

statusFromError (errors.go) names an error by its errno, and by the os sentinels that filesystems not backed by the OS return instead. It reports whether it found a status at all, so the caller picks the fallback — what a client will accept depends on the operation. Order matters in that ladder and is commented: syscall.Errno reports both EEXIST and ENOTEMPTY as os.ErrExist, and both EACCES and EPERM as os.ErrPermission, so the narrower errno is tested first. A ladder in the wrong order silently answers with the wider errno's status — which is what a naive os.IsExist check would have done here.

onRename uses it for fs.Rename and for both parent stats. onReadLink takes its status from the errno and keeps a stat only as a fallback for filesystems that report no errno at all — memfs reports "not a symlink" as an opaque fmt.Errorf, and that path still yields NFS3ERR_INVAL. That fallback uses Lstat, not Stat: Stat follows the link and reports the target's mode, so reading the symlink bit off it would call a genuine symlink the wrong file type (thanks @cursor — see the thread; fixed in 60396c6). Lstat is also already what onLookup, onGetAttr and onSetAttr use to ask a node's own type. The wrapped cause is preserved on every path, including the fallbacks, so the server log keeps it even when the wire status is generic.

statusFromWriteError is left alone: WRITE's RFC error list is narrow, and widening it would let the write path answer with statuses a client does not expect from WRITE. The comment on statusFromError says so.

One deliberate behaviour change

An error neither the errno ladder nor the Lstat fallback can name now leaves READLINK as NFS3ERR_IO rather than the NFS3ERR_ACCES it used to be. ACCES on a genuine EIO is a lie; IO is what RFC 1813 lists for the unknown case. Everything the ladder can name no longer reaches IO at all, which is the point of the change — worth a look if anything downstream keys on ACCES here.

Why this bites us

Our sandbox filesystem implements Rename with unix.Renameat rather than os.Rename, so it surfaces the real ENOTEMPTY and EISDIR. Those are exactly the errnos the old ladder flattened into NFS3ERR_IO, which is where the unhelpful 500s downstream come from.

Worth knowing for anyone reading the tests: os.Rename deliberately Lstats the destination first and returns EEXIST for any existing directory there, so a filesystem built on os.Rename can never report ENOTEMPTY or EISDIR — hence the two test functions below.

Tests

Both handlers are covered end to end: the tests drive the real server and read the status word straight off the wire (target.Call), since the client library translates a few statuses into os sentinels and collapses the rest.

• TestOnRenameStatus — a real osfs over a temp dir: what a Go-backed filesystem can actually report (EXIST, NOTDIR, INVAL for a directory into its own subtree, NOENT, plus a successful rename).
• TestOnRenameStatusFromErrno — a filesystem wrapper injecting each errno a renameat-based backend reports, through to the status on the wire.
• TestOnReadLinkStatusFromErrno — a wrapper injecting a readlink error on a name that really is a symlink, over memfs's own link-following Stat. That combination is the live risk: anything reading the symlink bit off a Stat there concludes the client asked READLINK of a non-symlink.
• TestOnReadLinkStatusWithoutErrno — plain memfs, so the fallback and the happy path are both still exercised.
• TestStatusFromError / TestStatusErrorFrom — the ladder's ordering, the os sentinels, wrapped/LinkError/PathError forms, and that the cause survives the fallback.

16 of the 21 subtests fail against master for the intended reason; the 5 that pass are the deliberate unchanged-behaviour guards. Beyond what CI covers: green with -race, gofmt clean, and go vet ./... clean cross-compiled for linux, darwin and windows on amd64 and arm64 (the errno constants used are defined on all three).

Note on CI: the Go workflow (build, lint, test) had never run on this repo before this PR — workflows from .github/workflows/ were disabled, as forks are by default, so only GitHub's managed code-scanning and Dependabot workflows ran here. It has since been enabled and now passes on these commits, lint and tests included. codeql-analysis.yml remains disabled_fork, which looks deliberate: the repo uses CodeQL default setup, and the "Analyze (go)" check comes from that rather than from the workflow file.

Portability upstream

Nothing here is fork-specific — it is written to cherry-pick cleanly onto willscott/go-nfs.

🤖 Generated with Claude Code

https://claude.ai/code/session_01AeywBLV66AeSH8WuDfvv7Q

[cursor]

PR Summary

Medium Risk
Changes NFS3 error codes returned to clients on RENAME and READLINK, including READLINK switching some failures from ACCES to IO; behavior is tested but callers that depended on the old codes could see different errno mapping.

Overview
RENAME used to turn most filesystem failures into NFS3ERR_IO, so cases like a non-empty destination directory or a type mismatch looked like a generic disk error instead of NOTEMPTY, EXIST, ISDIR, or NOTDIR. READLINK ignored the readlink error and inferred status from Stat, which follows symlinks and wrongly turned EINVAL, ENOTDIR, and EIO into NFS3ERR_ACCES while sometimes wrapping a nil Stat error in logs.

The change adds shared errno-to-status mapping (with ordering so ENOTEMPTY beats os.ErrExist) and wires RENAME and parent Stat failures through it. READLINK prefers the readlink errno, uses Lstat only when the error cannot be named, and uses NFS3ERR_IO instead of ACCES for those unnamed failures. Unit and loopback integration tests cover the mapping ladder and wire statuses.

^{Reviewed by Cursor Bugbot for commit 60396c6. Bugbot is set up for automated code reviews on this repo. Configure here.}

[cursor]

Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.

^{❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.}

^{Reviewed by Cursor Bugbot for commit 2834b15. Configure here.}