fix: report the status describing a filesystem error, not always EIO - #13
Conversation
66327fb to
d6057a8
Compare
arielshaqed
left a comment
arielshaqed
left a comment
There was a problem hiding this comment.
Thanks! This is really useful code. We should probably plan to push it upstream as well.
That said, the comments make the code much less readable - see my notes about them. Also I do not see the values that the largest test provides. Approving, but would gladly see less comments and test code in this PR.
| // recognizes nothing. | ||
| // | ||
| // A Filesystem's only channel to the client is the error it returns, and call | ||
| // sites used to hard-code a status for it -- almost always NFSStatusIO. That |
There was a problem hiding this comment.
Comment describe the current code. They should not describe some buggy previous code -- "used to hard-code a status". They should not describe bad things that used to happen or could happen. OTOH they should just say to use it.
Also use godoc for function comments. For instance
// statusFromError translates an error code into a suitable NFS status.| // into a single EIO, which tells the user nothing and invites clients to retry | ||
| // what cannot succeed. | ||
| // | ||
| // Cases match with errors.Is, so a *PathError or a filesystem's own added |
There was a problem hiding this comment.
- This is an implementation comment. If it belongs anywhere, it belongs inside the function.
- This particular implementation comment explains how the code could be written incorrectly but wasn't. I would just drop it.
| // | ||
| // Cases match with errors.Is, so a *PathError or a filesystem's own added | ||
| // context still resolves. The errnos come first: the io/fs sentinels are | ||
| // broader and would otherwise swallow them. |
There was a problem hiding this comment.
I don't understand which io/fs sentinels these are that extend syscall.E* errors. But - if there are any - this would be an implementation comment to put somewhere inside the switch.
| // A filesystem's error is its only channel to the client, so each error it can | ||
| // return must reach the client as the status describing it -- not as a blanket | ||
| // NFSStatusIO, which tells the user nothing and invites pointless retries. | ||
| func TestStatusFromError(t *testing.T) { |
There was a problem hiding this comment.
What does this test? It looks like a second copy of the switch statement of statusFromError - I would just lose this error.
| // A filesystem's error is its only channel to the client, so each error it can | ||
| // return must reach the client as the status describing it -- not as a blanket | ||
| // NFSStatusIO, which tells the user nothing and invites pointless retries. |
There was a problem hiding this comment.
This comment appears to justify the code. Not relevant above a test.
| // A filesystem's error must reach the client as the status describing it. The | ||
| // up-front WriteCapability check can't catch a filesystem that accepts writes | ||
| // in general and refuses this one, so the error from the operation is the only | ||
| // signal -- and reporting it as NFS3ERR_IO leaves the user with a bare EIO. |
There was a problem hiding this comment.
Again, this comment does not describe the test. I would remove it, maybe keep it in the PR description (TBH I think the description already explains it).
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 <lryahli@gmail.com>
d6057a8 to
a3de74b
Compare
2 participants
A
Filesystem's only channel to the client is the error it returns, but call sites hard-coded the status, so read-only, out-of-space, over-quota and non-empty-directory all reached the user as a bareEIO. The up-frontCapabilityCheckcan't help: it asks whether the filesystem accepts writes at all, not whether it accepts this one.statusFromErrormaps the error to the status describing it (EROFS→ROFS,ENOSPC→NoSPC,EDQUOT→DQuot,ENOTEMPTY→NotEmpty,EXDEV→XDev, plus theio/fssentinels), matching witherrors.Isso wrapped errors resolve; each call site keeps its old status as the fallback. It also replaces theos.IsNotExist/os.IsPermissionladders at 16 sites, which missed wrapped errors.go test ./...34 pass, builds for linux/windows/darwin.TestOperationErrorKeepsItsStatusreturnsEROFSfromRename:NFS3ERR_IO (5)on master,NFS3ERR_ROFS (30)with this change.Follows #12, whose one-off
EROFSmapping inonRenameis dropped in favour of covering every operation here.