Enhancements made for nfs serving#226
Merged
Merged
Conversation
EdSchouten
reviewed
May 22, 2026
EdSchouten
left a comment
EdSchouten
left a comment
Member
There was a problem hiding this comment.
Hey @visualphoenix,
First of all, glad to hear you're having fun using this code!
This code was originally designed to only serve the needs for Buildbarn. But given that it's become quite a versatile implementation (supporting macOS, Linux, and Windows) I can well imagine that people are also interested in using it outside of Buildbarn. Maybe in the long run we should move (parts of) it into a separate bb-vfs repository or something. So move the FUSE/NFSv4/WinFSP bits there, while keeping InMemoryPrepopulatedDirectory and other concrete leaf/directory implementations inside bb-remote-execution.
Anyway, let's see what we can do to get your changes in, while keeping all of Buildbarn working as intended.
Contributor
Author
|
Hello again @EdSchouten - I hope this message finds you well. Thank you again for the review on the previous diff! I appreciate the thoughtful feedback. Let me know if the cleanup here is more acceptable. I will open a new issue regarding the Permission/mode stuff. Would you like me to extend the opcode tests? Before I do I want to make sure you're okay with things. |
EdSchouten
reviewed
May 29, 2026
visualphoenix
force-pushed
the
nfs-server
branch
2 times, most recently
from
3181d78 to
3a79e5f
Compare
EdSchouten
approved these changes
May 31, 2026
Member
|
Hey! This looks great. Assuming CI is happy, I think we can merge the change as is. Thanks for working on this! |
Contributor
Author
|
lmk if you want me to fix that last commit message - idk if you prefer to squash or merge commit - i can clean that |
Member
|
Don't worry about it. My plan was to squash it, so I'll make some editorial mixups if needed. |
Contributor
Author
|
aww crud - i forgot the test files when i added the securityFlavors param |
Member
|
Whoops! Looks like the build is still failing. |
Contributor
Author
|
will fix - 1s |
Adds a new virtual.Status value for NAME_TOO_LONG. Each shim picks up the corresponding native error: toNFSv4Status returns NFS4ERR_NAMETOOLONG (shared by v4.0 and v4.1), toFUSEStatus returns ENAMETOOLONG, and the WinFSP mapping returns STATUS_NAME_TOO_LONG.
Mirrors the existing LastDataModificationTime accessor pattern for atime and ctime. Additive — existing consumers and implementations are unaffected. Lets NFSv4 GETATTR return real atime/ctime instead of the package-level deterministic fallback.
These four state-mutating ops were the only Virtual* methods on Directory and Leaf without a context.Context, despite VirtualMknod / VirtualLink / VirtualSymlink and the rest already taking one. VirtualClose / Seek / Allocate / Read are left as-is for now. Each shim sources the ctx from its existing handler plumbing: NFSv4 from the compound state, FUSE from createContext (which already carries caller credentials), WinFSP from the per-call context its handlers receive. No context.Background() is introduced — every call site has a real ctx available.
Per RFC 8881 §2.6.3, SECINFO* must return the security flavors
required to access a filehandle. The hard-coded AUTH_NONE response
was honest for bb's loopback consumer but broke any multi-user
setup: Linux's NFSv4.1 client probes SECINFO_NO_NAME during session
setup and switches every subsequent RPC to the advertised flavor,
so per-process AUTH_SYS credentials never reached the server.
Add a securityFlavors []nfsv4.Secinfo4 parameter to
NewNFS40Program / NewNFS41Program. Callers supply the list directly;
no default fallback. Existing bb consumers must pass an explicit
[]nfsv4.Secinfo4{&nfsv4.Secinfo4_default{Flavor: AUTH_NONE}} to keep
their previous behaviour.
settime4 has two arms — SET_TO_CLIENT_TIME4 (an explicit nfstime4) and SET_TO_SERVER_TIME4 (server substitutes wall-clock) — mirroring utimensat(2)'s UTIME_NOW. Decode both arms in the shared fattr4ToAttributes helper used by NFSv4.0 and NFSv4.1. SET_TO_CLIENT_TIME4 writes the supplied nfstime4 directly via SetLastAccessTime / SetLastDataModificationTime. SET_TO_SERVER_TIME4 writes a zero time.Time as a sentinel; backends substitute their wall-clock at apply time. Both writeAttributes implementations advertise the new bits in FATTR4_SUPPORTED_ATTRS.
The decoder previously rejected both with NFS4ERR_PERM, blocking SETATTR-based chown over NFSv4 entirely. The shared fattr4ToAttributes helper (used by v4.0 and v4.1) now parses the owner string with ParseUint to yield the uid/gid; non-numeric values return NFS4ERR_BADOWNER per RFC 7530 §13.1.2.3 / RFC 8881 §15.1.13. Supporting the user@domain form would need an injectable resolver; defer until a real consumer needs it. With the protocol gate removed, each VirtualSetAttributes implementation that doesn't track ownership now rejects chown explicitly. blob_access_cas_file_factory, in_memory_prepopulated_directory, and pool_backed_file_allocator return StatusErrPerm when OwnerUserID or OwnerGroupID is set. Silent acceptance would misrepresent the file's metadata.
Bundle of small fixes touching writeAttributes, opCreate, and nfsv4NewComponent in both programs: - Emit FATTR4_RAWDEV from Attributes.GetDeviceNumber. Block/char devices previously reported 0,0 on the wire because the attribute was never encoded. - Consult Attributes.GetLastAccessTime / GetLastStatusChangeTime when emitting FATTR4_TIME_ACCESS / FATTR4_TIME_METADATA, mirroring the existing FATTR4_TIME_MODIFY pattern. Both atime and ctime previously reported the package-level deterministicNfstime4 fallback regardless of the underlying filesystem's real values. - attrRequestToAttributesMask gains the corresponding mask translations and FATTR4_SUPPORTED_ATTRS in writeAttributes adds FATTR4_RAWDEV. - opCreate's NF4BLK / NF4CHR cases plumb Specdata (major/minor) through Attributes.SetDeviceNumber and call VirtualMknod, rather than returning NFS4ERR_PERM. Block/char creation typically still requires CAP_MKNOD host-side; unprivileged clients receive EPERM from the underlying syscall, which is the right POSIX behaviour. - opCreate applies args.Createattrs to the Attributes struct before dispatching to the type-specific Virtual* call. Client-supplied mode/owner/group from CREATE were previously silently dropped. - nfsv4NewComponent rejects component names longer than 255 bytes (NAME_MAX) with NFS4ERR_NAMETOOLONG. path.NewComponent does not enforce this bound; surfacing the spec-defined error gives clients better diagnostics than the eventual ENAMETOOLONG from the underlying syscall.
visualphoenix
changed the title
Contributor
Author
|
Sorry - just got bazel working - got a few more tests to fix. |
Contributor
Author
|
its going to fail that run |
visualphoenix
changed the title
Contributor
Author
|
i have to add EXPECT stuff for returning StatusErrPerm for VirtualSetAttributes for FUSE and VirtualMknod for NFS - and i have to fix the encoder tests because i modified the mask adding LastAccessTime and LastStatusChangeTime - i'll have to do these tomorrow night. sorry for overlooking these. i'll make sure i dont bug you until i get the tests working locally - i'll remove the WIP from the PR when its done. apologies again. |
visualphoenix
changed the title
Contributor
Author
|
Ready for review |
EdSchouten
approved these changes
Jun 1, 2026
EdSchouten
left a comment
EdSchouten
left a comment
Member
There was a problem hiding this comment.
One small remark. Be sure to fix, and I'll merge assuming CI is happy.
Contributor
Author
|
apologies for not fixing the lint fail at the same time. at this point i expect all of the ci checks to pass. thanks again for the detailed and excellent code review! <3 |
EdSchouten
added a commit
that referenced
this pull request
Jun 1, 2026
In PR #226 we changed VirtualMknod() to accept a virtual.Attributes instead of just the file type. With that change merged, we can argue that VirtualSymlink() has become superfluous. The signature of VirtualMknod() is complete enough to also support the creation of symlinks. Change InMemoryPrepopulatedDirectory.VirtualMknod() to either create special files or symlinks depending on the provided file type. With his change made, we can patch up all of the callers of VirtualSymlink() to call VirtualMknod() instead.
EdSchouten
added a commit
that referenced
this pull request
Jun 1, 2026
In PR #226 we changed VirtualMknod() to accept a virtual.Attributes instead of just the file type. With that change merged, we can argue that VirtualSymlink() has become superfluous. The signature of VirtualMknod() is complete enough to also support the creation of symlinks. Change InMemoryPrepopulatedDirectory.VirtualMknod() to either create special files or symlinks depending on the provided file type. With his change made, we can patch up all of the callers of VirtualSymlink() to call VirtualMknod() instead.
EdSchouten
added a commit
that referenced
this pull request
Jun 1, 2026
In PR #226 we changed VirtualMknod() to accept a virtual.Attributes instead of just the file type. With that change merged, we can argue that VirtualSymlink() has become superfluous. The signature of VirtualMknod() is complete enough to also support the creation of symlinks. Change InMemoryPrepopulatedDirectory.VirtualMknod() to either create special files or symlinks depending on the provided file type. With his change made, we can patch up all of the callers of VirtualSymlink() to call VirtualMknod() instead.
EdSchouten
added a commit
that referenced
this pull request
Jun 1, 2026
In PR #226 we changed VirtualMknod() to accept a virtual.Attributes instead of just the file type. With that change merged, we can argue that VirtualSymlink() has become superfluous. The signature of VirtualMknod() is complete enough to also support the creation of symlinks. Change InMemoryPrepopulatedDirectory.VirtualMknod() to either create special files or symlinks depending on the provided file type. With his change made, we can patch up all of the callers of VirtualSymlink() to call VirtualMknod() instead.
EdSchouten
added a commit
that referenced
this pull request
Jun 1, 2026
In PR #226 we changed VirtualMknod() to accept a virtual.Attributes instead of just the file type. With that change merged, we can argue that VirtualSymlink() has become superfluous. The signature of VirtualMknod() is complete enough to also support the creation of symlinks. Change InMemoryPrepopulatedDirectory.VirtualMknod() to either create special files or symlinks depending on the provided file type. With his change made, we can patch up all of the callers of VirtualSymlink() to call VirtualMknod() instead.
EdSchouten
added a commit
that referenced
this pull request
Jun 1, 2026
In PR #226 we changed VirtualMknod() to accept a virtual.Attributes instead of just the file type. With that change merged, we can argue that VirtualSymlink() has become superfluous. The signature of VirtualMknod() is complete enough to also support the creation of symlinks. Change InMemoryPrepopulatedDirectory.VirtualMknod() to either create special files or symlinks depending on the provided file type. With his change made, we can patch up all of the callers of VirtualSymlink() to call VirtualMknod() instead.
EdSchouten
added a commit
that referenced
this pull request
Jun 1, 2026
In PR #226 we changed VirtualMknod() to accept a virtual.Attributes instead of just the file type. With that change merged, we can argue that VirtualSymlink() has become superfluous. The signature of VirtualMknod() is complete enough to also support the creation of symlinks. Change InMemoryPrepopulatedDirectory.VirtualMknod() to either create special files or symlinks depending on the provided file type. With his change made, we can patch up all of the callers of VirtualSymlink() to call VirtualMknod() instead.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Hi @EdSchouten ❤️
Not sure what you'll think about these changes, but fwiw I successfully used these patches to pass 100% of pjdfstest on a pure go nfs v4/v4.1 server I've been hacking on.
I understand if they aren't what you want upstream, but I figured I'd put them up for your consideration.
Thank you again for maintaining this wonderful code base.