9p file sharing #4866
9p file sharing #4866
Conversation
WalkthroughThis change integrates 9P file sharing support by adding the Changes
Sequence Diagram(s)sequenceDiagram
participant Host as Host Daemon
participant 9PServer as 9P Server (fs9p)
participant VM as VM Guest OS
Host->>9PServer: Start 9P server on gateway IP:Plan9Port, sharing host home directory
VM->>9PServer: Mount 9P filesystem from Host (192.168.127.1:564)
9PServer-->>VM: Serve file requests via 9P protocol
VM-->>9PServer: Read/write files as needed
Estimated code review effort🎯 5 (Critical) | ⏱️ ~75+ minutes Possibly related issues
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 24
🧹 Nitpick comments (14)
vendor/github.com/DeedleFake/p9/internal/util/util.go (1)
44-52: Consider edge cases in error handlingThe
Errorffunction only checks forio.EOFin the special handling. Consider if other common error types (likenil) should also be handled directly.Apply this diff to handle
nilerrors as well:func Errorf(str string, args ...interface{}) error { for _, arg := range args { + if arg == nil { + return nil + } if arg == io.EOF { return arg.(error) } }vendor/github.com/DeedleFake/p9/internal/debug/nodebug.go (1)
1-5: Consider updating to modern build constraint syntaxThe implementation is correct and follows the standard pattern for conditional compilation. However, consider updating to the modern build constraint syntax for consistency with Go 1.17+.
Apply this diff to use modern build constraint syntax:
-// +build !p9debug +//go:build !p9debugThe functionality remains identical, but the new syntax is preferred in modern Go projects.
vendor/github.com/DeedleFake/p9/addr_unix.go (1)
17-29: Consider consistency with other platform implementation.The implementation is functionally correct, but there's an inconsistency with
addr_other.gowhich usesos.TempDir()instead of hardcoded "/tmp".For consistency, consider using
os.TempDir():- return filepath.Join("/", "tmp", "ns."+u.Username+"."+display) + return filepath.Join(os.TempDir(), "ns."+u.Username+"."+display)However, if the hardcoded "/tmp" is intentional for Unix systems (following 9p conventions), this should be documented in a comment.
vendor/github.com/DeedleFake/p9/README.md (1)
12-51: Address markdown formatting consistency.The documentation is well-written and provides clear examples, but there are some formatting inconsistencies flagged by static analysis.
Consider fixing the markdown formatting issues:
- Replace hard tabs with spaces in code blocks (lines 17, 26, 32, 37, 43, 49)
- Use consistent heading style (either ATX
#or Setext underline style)These are minor formatting issues that don't affect functionality but improve consistency.
vendor/github.com/DeedleFake/p9/doc.go (1)
40-40: Update deprecatedioutil.ReadAllin documentation example.The example uses
ioutil.ReadAllwhich has been deprecated since Go 1.16. Consider updating toio.ReadAllfor better practices in documentation examples.-// buf, _ := ioutil.ReadAll(file) +// buf, _ := io.ReadAll(file)vendor/github.com/DeedleFake/p9/dir_linux.go (1)
57-60: Consider dynamic QID version handling.The QID version is hardcoded to 0, which may not reflect actual file changes. In 9P, the version field typically changes when file content is modified to invalidate client caches.
vendor/github.com/DeedleFake/p9/dir_darwin.go (1)
57-60: Consider dynamic QID version handling.Same as the Linux implementation, the QID version is hardcoded to 0. Consider implementing proper version tracking for file change detection.
vendor/github.com/DeedleFake/p9/encoding.go (1)
16-30: Consider memory efficiency for large directories.The function reads all directory entries into memory at once, which could be problematic for very large directories. The recommendation about buffering is good, but consider also documenting memory implications.
Add a comment about memory usage:
// ReadDir decodes a series of directory entries from a reader. It // reads until EOF, so it doesn't return io.EOF as a possible error. // +// Note: This function loads all directory entries into memory. For very +// large directories, consider implementing streaming or batched reading. +// // It is recommended that the reader passed to ReadDir have some formpkg/fileserver/fs9p/shares.go (1)
46-53: Add context support for graceful shutdown.The error monitoring goroutine runs indefinitely. Consider adding context support for graceful shutdown when the application terminates.
- go func() { + go func(server *Server, serverDir string, addr string) { if err := server.WaitForError(); err != nil { - logrus.Errorf("Error from 9p server on %s for %s: %v", m.Listener.Addr().String(), serverDir, err) + logrus.Errorf("Error from 9p server on %s for %s: %v", addr, serverDir, err) } else { // We do not expect server exits - this should run until the program exits. - logrus.Warnf("9p server on %s for %s exited without error", m.Listener.Addr().String(), serverDir) + logrus.Warnf("9p server on %s for %s exited without error", addr, serverDir) } - }() + }(server, serverDir, m.Listener.Addr().String())This fixes variable capture issues and makes the goroutine parameters explicit.
vendor/github.com/DeedleFake/p9/proto/client.go (1)
138-145: Potential infinite loop with tag exhaustionIf all possible tag values (0-65535) are in use, the tag allocation loop will run forever. While unlikely, this could cause a deadlock in high-concurrency scenarios.
Consider adding a maximum iteration limit:
case c.nextTag <- nextTag: + iterations := 0 for { nextTag++ + iterations++ + if iterations > 65536 { + // All tags exhausted + break + } if _, ok := tags[nextTag]; !ok { break } }vendor/github.com/DeedleFake/p9/remote.go (1)
273-275: Unusual use of io.EOF for partial writesReturning
io.EOFwhen the write count is less than the data length is non-standard. Short writes typically don't indicate EOF in Go's io conventions.Consider using
io.ErrShortWriteor a custom error:if write.Count < uint32(len(data)) { - return int(write.Count), io.EOF + return int(write.Count), io.ErrShortWrite }vendor/github.com/DeedleFake/p9/proto/server.go (1)
19-22: Consider handling temporary network errors gracefullyThe server terminates on any accept error, but some errors might be temporary (e.g., EMFILE, ENFILE). Consider checking if the error is temporary and implementing a backoff strategy to keep the server running.
for { c, err := lis.Accept() if err != nil { + // Check if this is a temporary error + if ne, ok := err.(net.Error); ok && ne.Temporary() { + log.Printf("Temporary accept error: %v", err) + time.Sleep(time.Millisecond * 10) + continue + } return err }vendor/github.com/DeedleFake/p9/dir.go (1)
95-103: Hardcoded file permissions in Open methodThe
Openmethod uses hardcoded0644permissions when opening files (line 99). While this parameter is typically ignored for existing files, it would be better to use0or get the actual file permissions.func (d Dir) Open(p string, mode uint8) (File, error) { flag := toOSFlags(mode) - file, err := os.OpenFile(d.path(p), flag, 0644) + file, err := os.OpenFile(d.path(p), flag, 0) return &dirFile{ File: file, }, err }vendor/github.com/DeedleFake/p9/msg.go (1)
145-145: Consider implementing a proper Mode type as indicated by TODOsThe TODO comments indicate that
Modefields should have a proper type instead of using rawuint8. This would improve type safety and make the code more maintainable.Would you like me to create an issue to track the implementation of a proper Mode type for these fields?
Also applies to: 157-157
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (34)
cmd/crc/cmd/daemon.go(4 hunks)go.mod(2 hunks)pkg/crc/constants/constants.go(1 hunks)pkg/crc/machine/libhvee/driver_windows.go(1 hunks)pkg/crc/machine/start.go(2 hunks)pkg/fileserver/fs9p/server.go(1 hunks)pkg/fileserver/fs9p/shares.go(1 hunks)vendor/github.com/DeedleFake/p9/LICENSE(1 hunks)vendor/github.com/DeedleFake/p9/README.md(1 hunks)vendor/github.com/DeedleFake/p9/addr.go(1 hunks)vendor/github.com/DeedleFake/p9/addr_other.go(1 hunks)vendor/github.com/DeedleFake/p9/addr_unix.go(1 hunks)vendor/github.com/DeedleFake/p9/client.go(1 hunks)vendor/github.com/DeedleFake/p9/dir.go(1 hunks)vendor/github.com/DeedleFake/p9/dir_darwin.go(1 hunks)vendor/github.com/DeedleFake/p9/dir_linux.go(1 hunks)vendor/github.com/DeedleFake/p9/dir_other.go(1 hunks)vendor/github.com/DeedleFake/p9/dir_plan9.go(1 hunks)vendor/github.com/DeedleFake/p9/dir_windows.go(1 hunks)vendor/github.com/DeedleFake/p9/doc.go(1 hunks)vendor/github.com/DeedleFake/p9/encoding.go(1 hunks)vendor/github.com/DeedleFake/p9/fs.go(1 hunks)vendor/github.com/DeedleFake/p9/internal/debug/debug.go(1 hunks)vendor/github.com/DeedleFake/p9/internal/debug/nodebug.go(1 hunks)vendor/github.com/DeedleFake/p9/internal/util/util.go(1 hunks)vendor/github.com/DeedleFake/p9/msg.go(1 hunks)vendor/github.com/DeedleFake/p9/p9.go(1 hunks)vendor/github.com/DeedleFake/p9/proto/client.go(1 hunks)vendor/github.com/DeedleFake/p9/proto/encoding.go(1 hunks)vendor/github.com/DeedleFake/p9/proto/proto.go(1 hunks)vendor/github.com/DeedleFake/p9/proto/server.go(1 hunks)vendor/github.com/DeedleFake/p9/remote.go(1 hunks)vendor/github.com/DeedleFake/p9/stat.go(1 hunks)vendor/modules.txt(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (19)
vendor/github.com/DeedleFake/p9/internal/debug/debug.go (1)
vendor/github.com/DeedleFake/p9/internal/debug/nodebug.go (1)
Log(5-5)
vendor/github.com/DeedleFake/p9/internal/debug/nodebug.go (1)
vendor/github.com/DeedleFake/p9/internal/debug/debug.go (1)
Log(10-12)
vendor/github.com/DeedleFake/p9/dir_other.go (1)
vendor/github.com/DeedleFake/p9/stat.go (3)
DirEntry(255-267)FileMode(23-23)ModeFromOS(43-78)
cmd/crc/cmd/daemon.go (2)
pkg/crc/constants/constants.go (2)
Plan9Port(59-59)GetHomeDir(164-170)pkg/fileserver/fs9p/shares.go (2)
StartShares(26-57)Mount9p(13-16)
vendor/github.com/DeedleFake/p9/addr.go (2)
vendor/github.com/DeedleFake/p9/addr_other.go (1)
NamespaceDir(17-29)vendor/github.com/DeedleFake/p9/addr_unix.go (1)
NamespaceDir(17-29)
vendor/github.com/DeedleFake/p9/encoding.go (2)
vendor/github.com/DeedleFake/p9/stat.go (2)
DirEntry(255-267)Stat(156-168)vendor/github.com/DeedleFake/p9/proto/encoding.go (2)
Read(34-38)Write(25-31)
vendor/github.com/DeedleFake/p9/client.go (4)
vendor/github.com/DeedleFake/p9/proto/client.go (3)
Client(18-31)NewClient(35-56)Dial(60-67)vendor/github.com/DeedleFake/p9/msg.go (8)
Proto(75-77)Tversion(79-82)Tversion(84-84)Rversion(86-89)Tauth(95-99)Rauth(101-103)Tattach(105-110)Rattach(112-114)vendor/github.com/DeedleFake/p9/p9.go (3)
Version(10-10)NoFID(18-18)QID(42-46)vendor/github.com/DeedleFake/p9/remote.go (1)
Remote(19-27)
vendor/github.com/DeedleFake/p9/proto/encoding.go (1)
vendor/github.com/DeedleFake/p9/internal/util/util.go (1)
Errorf(44-52)
vendor/github.com/DeedleFake/p9/addr_other.go (1)
vendor/github.com/DeedleFake/p9/addr_unix.go (1)
NamespaceDir(17-29)
pkg/crc/machine/start.go (1)
pkg/os/exec.go (1)
RunPrivileged(48-59)
pkg/crc/machine/libhvee/driver_windows.go (1)
vendor/github.com/crc-org/machine/libmachine/drivers/base.go (1)
SharedDir(27-35)
vendor/github.com/DeedleFake/p9/p9.go (1)
vendor/github.com/DeedleFake/p9/stat.go (1)
FileMode(23-23)
vendor/github.com/DeedleFake/p9/addr_unix.go (1)
vendor/github.com/DeedleFake/p9/addr_other.go (1)
NamespaceDir(17-29)
vendor/github.com/DeedleFake/p9/dir_windows.go (1)
vendor/github.com/DeedleFake/p9/stat.go (3)
DirEntry(255-267)FileMode(23-23)ModeFromOS(43-78)
vendor/github.com/DeedleFake/p9/remote.go (6)
vendor/github.com/DeedleFake/p9/client.go (1)
Client(22-26)vendor/github.com/DeedleFake/p9/p9.go (4)
QID(42-46)QIDType(49-49)Version(10-10)IOHeaderSize(70-70)vendor/github.com/DeedleFake/p9/msg.go (14)
Twalk(133-137)Rwalk(139-141)Topen(143-146)Ropen(148-151)Tcreate(153-158)Rcreate(160-163)Tremove(192-194)Tread(165-169)Rread(171-173)Twrite(175-179)Rwrite(181-183)Tclunk(185-187)Tstat(199-201)Rstat(203-205)vendor/github.com/DeedleFake/p9/stat.go (3)
FileMode(23-23)Stat(156-168)DirEntry(255-267)vendor/github.com/DeedleFake/p9/internal/util/util.go (1)
Errorf(44-52)vendor/github.com/DeedleFake/p9/encoding.go (1)
ReadDir(16-30)
vendor/github.com/DeedleFake/p9/dir_plan9.go (3)
vendor/github.com/DeedleFake/p9/stat.go (3)
DirEntry(255-267)FileMode(23-23)ModeFromOS(43-78)vendor/github.com/DeedleFake/p9/dir.go (1)
Dir(16-16)vendor/github.com/DeedleFake/p9/p9.go (2)
QID(42-46)Version(10-10)
vendor/github.com/DeedleFake/p9/msg.go (6)
vendor/github.com/DeedleFake/p9/proto/proto.go (2)
NewProto(44-60)Proto(38-41)vendor/github.com/DeedleFake/p9/p9.go (2)
Version(10-10)QID(42-46)vendor/github.com/DeedleFake/p9/proto/client.go (1)
P9NoTag(221-223)vendor/github.com/DeedleFake/p9/stat.go (3)
FileMode(23-23)Stat(156-168)ErrLargeStat(18-18)vendor/github.com/DeedleFake/p9/proto/encoding.go (2)
Write(25-31)Read(34-38)vendor/github.com/DeedleFake/p9/internal/util/util.go (1)
LimitedReader(13-17)
vendor/github.com/DeedleFake/p9/proto/server.go (2)
vendor/github.com/DeedleFake/p9/msg.go (1)
Proto(75-77)vendor/github.com/DeedleFake/p9/proto/proto.go (1)
Proto(38-41)
vendor/github.com/DeedleFake/p9/dir.go (3)
vendor/github.com/DeedleFake/p9/stat.go (4)
Stat(156-168)DirEntry(255-267)StatChanges(316-318)FileMode(23-23)vendor/github.com/DeedleFake/p9/fs.go (3)
File(100-119)Attachment(44-74)FileSystem(21-34)vendor/github.com/DeedleFake/p9/p9.go (7)
OWRITE(27-27)ORDWR(28-28)OEXEC(29-29)OTRUNC(31-31)OCEXEC(32-32)ORCLOSE(33-33)OREAD(26-26)
🪛 markdownlint-cli2 (0.17.2)
vendor/github.com/DeedleFake/p9/README.md
12-12: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
17-17: Hard tabs
Column: 1
(MD010, no-hard-tabs)
21-21: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
26-26: Hard tabs
Column: 1
(MD010, no-hard-tabs)
32-32: Hard tabs
Column: 1
(MD010, no-hard-tabs)
37-37: Hard tabs
Column: 1
(MD010, no-hard-tabs)
43-43: Hard tabs
Column: 1
(MD010, no-hard-tabs)
49-49: Hard tabs
Column: 1
(MD010, no-hard-tabs)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (18)
- GitHub Check: build (ubuntu-latest, 1.23)
- GitHub Check: build (macOS-13, 1.23)
- GitHub Check: build (macOS-14, 1.23)
- GitHub Check: build (windows-2022, 1.23)
- GitHub Check: build (windows-2022, 1.23)
- GitHub Check: build-qe (windows, amd64)
- GitHub Check: verify-devcontainer
- GitHub Check: build (macOS-14, 1.23)
- GitHub Check: build (macOS-13, 1.23)
- GitHub Check: build-qe (linux, arm64)
- GitHub Check: build (ubuntu-22.04, 1.23)
- GitHub Check: build (ubuntu-latest, 1.23)
- GitHub Check: build-qe (darwin, amd64)
- GitHub Check: Run OKD bundle with crc (1.23)
- GitHub Check: build-qe (linux, amd64)
- GitHub Check: build-qe (darwin, arm64)
- GitHub Check: build-installer (windows-2022, 1.23)
- GitHub Check: Konflux kflux-prd-rh02 / crc-binary-on-pull-request
🔇 Additional comments (34)
vendor/github.com/DeedleFake/p9/internal/util/util.go (2)
8-17: LGTM - Well-documented LimitedReader implementationThe type definition and documentation clearly explain the differences from the standard
io.LimitedReader. The use ofuint32forNis beneficial for larger sizes on 32-bit systems, and the custom error fieldEprovides better error handling flexibility.
27-39: Ignore boundary‐condition tests for vendored codeThis file lives under
vendor/…and is an upstream dependency. We don’t add or maintain tests for vendored packages in our repo—any fixes or additional coverage should be contributed back to the original DeedleFake/p9 project. Please disregard this request here.Likely an incorrect or invalid review comment.
vendor/modules.txt (1)
13-18: Dependency update verified:p9v0.6.12 is up-to-date and secureI’ve confirmed that v0.6.12 is the latest release of
github.com/DeedleFake/p9(as of August 2025) and no publicly reported security vulnerabilities affect this version. No changes are needed.pkg/crc/constants/constants.go (1)
59-60: LGTM - Appropriate 9P protocol constantsThe constants are well-chosen:
- Port 564 is the standard IANA-assigned port for the 9P protocol
- 1MB message size (
Plan9Msize) provides a good balance between performance and memory usagevendor/github.com/DeedleFake/p9/LICENSE (1)
1-22: LGTM - Standard MIT LicenseThe MIT License is permissive and compatible with most projects. The license text is standard and complete.
vendor/github.com/DeedleFake/p9/internal/debug/debug.go (1)
10-12: LGTM! Clean debug logging implementation.The conditional compilation approach with build tags is well-implemented, providing debug logging only when needed while maintaining a no-op version for production builds.
go.mod (1)
7-7: LGTM! Dependencies appropriately added for 9p support.The addition of
github.com/DeedleFake/p9v0.6.12 and promotion ofgithub.hdec.top/linuxkit/virtsockto a direct dependency aligns with the 9p file sharing functionality being introduced.Also applies to: 34-34
pkg/crc/machine/start.go (1)
262-269: 9p mount configuration verifiedThe hardcoded gateway IP (
192.168.127.1) matchesconstants.VSockGatewayin pkg/crc/constants/constants.go and the9pfscommand is the correct Plan 9 filesystem client (port 564). Approving these changes as-is.vendor/github.com/DeedleFake/p9/addr_other.go (1)
17-29: LGTM! Proper cross-platform namespace directory implementation.The use of
os.TempDir()instead of hardcoded/tmpis appropriate for non-Unix platforms, while maintaining consistent behavior with the Unix version for DISPLAY handling and error conditions.vendor/github.com/DeedleFake/p9/dir_other.go (1)
7-14: LGTM! Appropriate minimal fallback implementation.This provides essential file metadata conversion for platforms without specific implementations. The missing fields (UID, GID, ATime, etc.) are reasonable omissions for a fallback version.
cmd/crc/cmd/daemon.go (2)
27-27: LGTM - Clean integration of fs9p package.The import is appropriately placed and follows the existing import structure.
181-181: LGTM - Improved address construction.Using
net.JoinHostPortprovides better IPv6 compatibility and cleaner code compared to string concatenation.Also applies to: 197-197
vendor/github.com/DeedleFake/p9/dir_windows.go (1)
9-27: LGTM - Well-implemented platform-specific file metadata handling.The implementation correctly handles Windows-specific file attributes while providing a graceful fallback. The type assertion is properly checked, and the timestamp conversion from Windows FILETIME to Go time.Time is accurate.
vendor/github.com/DeedleFake/p9/dir_linux.go (1)
12-44: LGTM! Robust Linux-specific file metadata handling.The
infoToEntryfunction properly handles both cases where syscall information is available and when it's not, with graceful fallback for user/group name resolution.vendor/github.com/DeedleFake/p9/dir_darwin.go (1)
12-44: LGTM! Platform-appropriate Darwin file metadata handling.The implementation correctly uses Darwin-specific
syscall.Stat_tfields, particularlysys.Atimespec.Unix()for access time, which differs appropriately from the Linux implementation.vendor/github.com/DeedleFake/p9/dir_plan9.go (2)
10-31: Excellent native Plan 9 implementation.This implementation leverages Plan 9's native understanding of 9P concepts, directly accessing UID/GID as strings and using the native
syscall.Dirstructure. This is cleaner than the user lookup required on other platforms.
44-48: Perfect QID handling for Plan 9.Unlike other platforms, this correctly uses the actual QID information from the filesystem (
sys.Qid.Type,sys.Qid.Vers,sys.Qid.Path) rather than constructing it. This provides proper version tracking and is the reference implementation other platforms should aspire to.vendor/github.com/DeedleFake/p9/addr.go (2)
32-66: Comprehensive address parsing with good edge case handling.The
ParseAddrfunction handles multiple 9P address formats correctly, including namespace addresses, Unix sockets, TCP with special port mappings, and bang-separated network specifications. The logic flow is well-structured with appropriate fallbacks.
70-72: Confirm panic behavior in NamespaceDirWe verified that both implementations of NamespaceDir (addr_other.go and addr_unix.go) will panic if user.Current() returns an error, and that GetNamespace simply calls it without recovery. As a result, any failure in user lookup will crash the process when ParseAddr is given a “$name” address.
Points to address:
- vendor/github.com/DeedleFake/p9/addr_other.go, addr_unix.go: NamespaceDir() panics on
user.Current()error.- vendor/github.com/DeedleFake/p9/addr.go: GetNamespace(name) joins NamespaceDir() without error handling.
Action items:
- Decide whether a hard panic here is acceptable in your environment (e.g., user.Current should never fail on your target platforms).
- If not, refactor NamespaceDir to return
(string, error), update GetNamespace (and ParseAddr) to propagate errors, and handle them where ParseAddr is invoked.- At minimum, document this panic behavior clearly so callers know to avoid or catch it.
pkg/fileserver/fs9p/server.go (1)
26-37: LGTM - Proper input validation for security.The function correctly validates that the exposed directory path is absolute, exists, and is actually a directory. This prevents common security issues like relative path traversal attacks.
vendor/github.com/DeedleFake/p9/encoding.go (1)
32-42: LGTM - Clean error handling.The WriteDir function has appropriate error handling and follows Go conventions for early return on error.
vendor/github.com/DeedleFake/p9/p9.go (3)
7-11: LGTM - Standard 9P protocol version.Using "9P2000" as the version string is correct according to the 9P protocol specification.
63-66: LGTM - FileMode conversion follows specification.The left shift by 24 bits to convert QIDType to FileMode follows the standard 9P convention where QID type bits are stored in the upper byte of the mode field.
52-61: QIDType constants correctly match the 9P2000 spec
The current bit-shift pattern produces exactly the defined values (0x00, 0x80, 0x40, 0x20, 0x10, 0x08, 0x04, 0x02), so no changes are needed.vendor/github.com/DeedleFake/p9/client.go (1)
49-69: LGTM - Proper handshake implementation.The handshake method correctly implements the 9P version negotiation protocol, including version validation and msize configuration.
pkg/fileserver/fs9p/shares.go (1)
59-82: LGTM - Clean VSOCK integration.The VSOCK share functionality properly converts GUIDs to listeners and delegates to the standard share management. The error handling is appropriate.
vendor/github.com/DeedleFake/p9/proto/encoding.go (2)
13-20: LGTM!The
Sizefunction correctly calculates message size using the encoder in size mode.
22-31: LGTM!The
Writefunction properly encodes and writes messages without buffering, as documented.vendor/github.com/DeedleFake/p9/fs.go (1)
297-346: Well-implemented walk handlerThe walk implementation correctly handles partial walks according to the 9P specification, properly distinguishing between first-element failures and subsequent failures.
vendor/github.com/DeedleFake/p9/remote.go (2)
34-71: Well-implemented walk functionThe walk implementation correctly handles path traversal and properly marks incomplete walks with an invalid QID.
229-249: Correct implementation of chunked readingThe
ReadAtimplementation properly handles message size limits by splitting large reads into multiple requests, with appropriate error handling.vendor/github.com/DeedleFake/p9/proto/proto.go (2)
44-60: Well-designed protocol mapping initializationThe
NewProtofunction correctly creates bidirectional mappings and properly handles both pointer and non-pointer types.
125-171: Efficient buffered sending with proper resource managementThe
Sendimplementation correctly uses buffer pooling and ensures atomic message writes with proper cleanup in all code paths.vendor/github.com/DeedleFake/p9/proto/server.go (1)
81-92: Incorrect msize duplicate check logicThe warning for setting msize twice (line 83) is inside the
sync.Once.Do()callback, which only executes once. The check should be outside theDo()call.if rmsg, ok := rmsg.(Msizer); ok { - if msize > 0 { - log.Println("Warning: Attempted to set msize twice.") - } - setter.Do(func() { msize = rmsg.P9Msize() mode = func(f func()) { go f() } }) + if msize > 0 && msize != rmsg.P9Msize() { + log.Printf("Warning: Attempted to set msize twice. Current: %d, Attempted: %d", msize, rmsg.P9Msize()) + } }Likely an incorrect or invalid review comment.
| // 9p home directory sharing | ||
| listener9p, err := vn.Listen("tcp", net.JoinHostPort(configuration.GatewayIP, fmt.Sprintf("%d", constants.Plan9Port))) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| if _, err := fs9p.StartShares([]fs9p.Mount9p{{Listener: listener9p, Path: constants.GetHomeDir()}}); err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Consider security implications of serving the entire home directory.
The 9p server exposes the user's complete home directory without apparent access restrictions. This could potentially expose sensitive files like SSH keys, configuration files, or personal documents.
Consider implementing:
- A dedicated shared directory instead of the entire home directory
- Access control mechanisms
- Read-only access by default
🏁 Script executed:
#!/bin/bash
# Check if there are any access control mechanisms in the fs9p package
ast-grep --pattern 'type $_ struct {
$$$
ReadOnly $_
$$$
}'
# Look for authentication or authorization code
rg -A 5 -B 5 "auth|Auth|permission|Permission|access|Access" pkg/fileserver/fs9p/Length of output: 148475
Limit 9p Shares to a Scoped, Read-Only Directory
The current code in cmd/crc/cmd/daemon.go (lines 252–259) publishes the user’s entire home via 9p without any access controls or read-only enforcement. Our scan of pkg/fileserver/fs9p found no built-in auth or permission checks.
Please update to:
- Share a dedicated subdirectory (e.g.
$CRC_HOME/share) rather thanconstants.GetHomeDir(). - Enforce read-only access by default (e.g. add a
ReadOnlyflag on the Mount9p struct or wrap the listener). - If stronger security is needed, implement path filters or authentication in fs9p before serving.
Affected location:
• cmd/crc/cmd/daemon.go:252–259
🤖 Prompt for AI Agents
In cmd/crc/cmd/daemon.go lines 252 to 259, the code shares the entire user home
directory via 9p without access controls or read-only enforcement. To fix this,
change the shared path from constants.GetHomeDir() to a dedicated subdirectory
like $CRC_HOME/share. Then, enforce read-only access by setting a ReadOnly flag
on the Mount9p struct or by wrapping the listener to restrict write operations.
Optionally, consider adding path filters or authentication in fs9p for stronger
security before serving.
| sharedDir9p := drivers.SharedDir{ | ||
| Source: dir, | ||
| Target: convertToUnixPath(dir) + "9p", // temporary solution until smb sharing is removed | ||
| Tag: "crc-dir0", // same as above | ||
| //Tag: fmt.Sprintf("dir%d", i), | ||
| Type: "9p", | ||
| } | ||
| sharedDirs = append(sharedDirs, sharedDir, sharedDir9p) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Address the duplicate tag usage and temporary implementation.
Using the same tag "crc-dir0" for both CIFS and 9p shares could lead to conflicts. The "9p" suffix on the target path and the comment indicate this is a temporary solution.
Consider implementing unique tags per share type:
sharedDir9p := drivers.SharedDir{
Source: dir,
- Target: convertToUnixPath(dir) + "9p", // temporary solution until smb sharing is removed
- Tag: "crc-dir0", // same as above
- //Tag: fmt.Sprintf("dir%d", i),
+ Target: convertToUnixPath(dir),
+ Tag: "crc-dir0-9p",
Type: "9p",
}This approach provides cleaner separation between protocols and prepares for eventual CIFS removal.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| sharedDir9p := drivers.SharedDir{ | |
| Source: dir, | |
| Target: convertToUnixPath(dir) + "9p", // temporary solution until smb sharing is removed | |
| Tag: "crc-dir0", // same as above | |
| //Tag: fmt.Sprintf("dir%d", i), | |
| Type: "9p", | |
| } | |
| sharedDirs = append(sharedDirs, sharedDir, sharedDir9p) | |
| sharedDir9p := drivers.SharedDir{ | |
| Source: dir, | |
| Target: convertToUnixPath(dir), | |
| Tag: "crc-dir0-9p", | |
| Type: "9p", | |
| } | |
| sharedDirs = append(sharedDirs, sharedDir, sharedDir9p) |
🤖 Prompt for AI Agents
In pkg/crc/machine/libhvee/driver_windows.go around lines 46 to 53, the code
uses the same tag "crc-dir0" for both CIFS and 9p shares, which can cause
conflicts. Update the tags to be unique for each share type, for example, use
distinct tags like "crc-dir0-cifs" and "crc-dir0-9p" or similar naming
conventions. This will ensure clear separation between protocols and support the
planned removal of CIFS sharing.
| errChan := make(chan error) | ||
|
|
||
| fs := p9.FileSystem(p9.Dir(exposeDir)) | ||
|
|
||
| go func() { | ||
| errChan <- proto.Serve(listener, p9.Proto(), p9.FSConnHandler(fs, constants.Plan9Msize)) | ||
| close(errChan) | ||
| }() |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Potential race condition in server startup detection.
The non-blocking select statement may miss early startup errors that occur after the goroutine starts but before the select executes. Consider adding a small timeout or using a synchronization mechanism to ensure reliable error detection.
- // Just before returning, check to see if we got an error off server startup.
- select {
- case err := <-errChan:
- return nil, fmt.Errorf("starting 9p server: %w", err)
- default:
- logrus.Infof("Successfully started 9p server on %s for directory %s", listener.Addr().String(), exposeDir)
- }
+ // Wait briefly for potential startup errors
+ select {
+ case err := <-errChan:
+ return nil, fmt.Errorf("starting 9p server: %w", err)
+ case <-time.After(100 * time.Millisecond):
+ logrus.Infof("Successfully started 9p server on %s for directory %s", listener.Addr().String(), exposeDir)
+ }You'll need to import the time package.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In pkg/fileserver/fs9p/server.go around lines 39 to 46, the current non-blocking
select used to detect server startup errors can miss errors occurring
immediately after the goroutine starts. To fix this, replace the non-blocking
select with a blocking receive that includes a timeout using the time package.
This ensures the main goroutine waits for either an error from errChan or a
timeout before proceeding, providing reliable error detection during server
startup.
| // Stop a running server. | ||
| // Please note that this does *BAD THINGS* to clients if they are still running | ||
| // when the server stops. Processes get stuck in I/O deep sleep and zombify, and | ||
| // nothing I do other than restarting the VM can remove the zombies. | ||
| func (s *Server) Stop() error { | ||
| if err := s.Listener.Close(); err != nil { | ||
| return err | ||
| } | ||
| logrus.Infof("Successfully stopped 9p server for directory %s", s.ExposedDir) | ||
| return nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Document the zombie process issue for operators.
The comment about "BAD THINGS" and zombie processes is concerning for production use. Consider adding structured logging or returning a specific error type to help operators understand the implications.
// Stop a running server.
-// Please note that this does *BAD THINGS* to clients if they are still running
-// when the server stops. Processes get stuck in I/O deep sleep and zombify, and
-// nothing I do other than restarting the VM can remove the zombies.
+// Stop a running server.
+// WARNING: Stopping the server while clients are actively using it may cause
+// client processes to become unresponsive and require VM restart to recover.
+// Ensure all clients have disconnected before calling Stop().
func (s *Server) Stop() error {
if err := s.Listener.Close(); err != nil {
- return err
+ return fmt.Errorf("failed to close 9p server listener: %w", err)
}
logrus.Infof("Successfully stopped 9p server for directory %s", s.ExposedDir)
return nil
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Stop a running server. | |
| // Please note that this does *BAD THINGS* to clients if they are still running | |
| // when the server stops. Processes get stuck in I/O deep sleep and zombify, and | |
| // nothing I do other than restarting the VM can remove the zombies. | |
| func (s *Server) Stop() error { | |
| if err := s.Listener.Close(); err != nil { | |
| return err | |
| } | |
| logrus.Infof("Successfully stopped 9p server for directory %s", s.ExposedDir) | |
| return nil | |
| } | |
| // Stop a running server. | |
| // WARNING: Stopping the server while clients are actively using it may cause | |
| // client processes to become unresponsive and require VM restart to recover. | |
| // Ensure all clients have disconnected before calling Stop(). | |
| func (s *Server) Stop() error { | |
| if err := s.Listener.Close(); err != nil { | |
| return fmt.Errorf("failed to close 9p server listener: %w", err) | |
| } | |
| logrus.Infof("Successfully stopped 9p server for directory %s", s.ExposedDir) | |
| return nil | |
| } |
🤖 Prompt for AI Agents
In pkg/fileserver/fs9p/server.go around lines 64 to 74, enhance the Stop method
by adding structured logging or returning a specific error type that clearly
communicates the risk of zombie processes when stopping the server while clients
are still connected. Update the comment to be more informative for operators,
and ensure the log or error provides actionable information about the potential
impact and recommended operator actions.
| // WaitForError from a running server. | ||
| func (s *Server) WaitForError() error { | ||
| err := <-s.ErrChan | ||
| return err | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add timeout to prevent indefinite blocking.
The WaitForError method blocks indefinitely waiting for an error. Consider adding a context parameter or timeout to prevent hanging callers.
-// WaitForError from a running server.
-func (s *Server) WaitForError() error {
- err := <-s.ErrChan
- return err
-}
+// WaitForError from a running server with context support.
+func (s *Server) WaitForError(ctx context.Context) error {
+ select {
+ case err := <-s.ErrChan:
+ return err
+ case <-ctx.Done():
+ return ctx.Err()
+ }
+}You'll need to import the context package and update callers accordingly.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // WaitForError from a running server. | |
| func (s *Server) WaitForError() error { | |
| err := <-s.ErrChan | |
| return err | |
| } | |
| // WaitForError from a running server with context support. | |
| func (s *Server) WaitForError(ctx context.Context) error { | |
| select { | |
| case err := <-s.ErrChan: | |
| return err | |
| case <-ctx.Done(): | |
| return ctx.Err() | |
| } | |
| } |
🤖 Prompt for AI Agents
In pkg/fileserver/fs9p/server.go around lines 76 to 80, the WaitForError method
blocks indefinitely on receiving from s.ErrChan, which can cause callers to
hang. Modify the method to accept a context.Context parameter and use a select
statement to wait on either s.ErrChan or ctx.Done(), returning an appropriate
error if the context times out or is canceled. Update all callers of
WaitForError to pass a context and handle the new timeout behavior. Also, import
the context package at the top of the file.
return created 9p servers for StartShares
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
pkg/fileserver/fs9p/shares.go (1)
25-57: The error cleanup issue identified in the previous review still needs to be addressed.The defer function inside the loop still captures
defErrby closure and creates multiple defers that reference the sameservervariable, leading to unreliable cleanup on partial failure.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
cmd/crc/cmd/daemon.go(4 hunks)pkg/crc/constants/constants.go(1 hunks)pkg/crc/machine/libhvee/driver_windows.go(1 hunks)pkg/crc/machine/start.go(2 hunks)pkg/fileserver/fs9p/server.go(1 hunks)pkg/fileserver/fs9p/shares.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- pkg/crc/constants/constants.go
🚧 Files skipped from review as they are similar to previous changes (4)
- pkg/crc/machine/libhvee/driver_windows.go
- cmd/crc/cmd/daemon.go
- pkg/crc/machine/start.go
- pkg/fileserver/fs9p/server.go
🧰 Additional context used
🪛 GitHub Check: build (macOS-14, 1.23)
pkg/fileserver/fs9p/shares.go
[failure] 69-69:
undefined: hvsock.Listen
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (17)
- GitHub Check: build (windows-2022, 1.23)
- GitHub Check: build (ubuntu-latest, 1.23)
- GitHub Check: build (ubuntu-22.04, 1.23)
- GitHub Check: build (macOS-13, 1.23)
- GitHub Check: build-installer (windows-2022, 1.23)
- GitHub Check: verify-devcontainer
- GitHub Check: build (windows-2022, 1.23)
- GitHub Check: build-qe (darwin, amd64)
- GitHub Check: build (ubuntu-latest, 1.23)
- GitHub Check: Run OKD bundle with crc (1.23)
- GitHub Check: build-qe (linux, arm64)
- GitHub Check: build-qe (darwin, arm64)
- GitHub Check: build-qe (windows, amd64)
- GitHub Check: build-qe (linux, amd64)
- GitHub Check: build (macOS-13, 1.23)
- GitHub Check: build (macOS-14, 1.23)
- GitHub Check: Konflux kflux-prd-rh02 / crc-binary-on-pull-request
🔇 Additional comments (1)
pkg/fileserver/fs9p/shares.go (1)
11-23: LGTM!The struct definitions are well-documented and appropriately designed for their purpose of representing 9P mounts with different connection types.
… into 9pfs client
|
@redbeam: The following tests failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
| defer func() { | ||
| if defErr != nil { | ||
| if err := server.Stop(); err != nil { | ||
| logrus.Errorf("Error stopping 9p server: %v", err) | ||
| } | ||
| } | ||
| }() |
There was a problem hiding this comment.
do we need this defer block, it seems defErr value is not being set in the function?
| go func() { | ||
| errChan <- proto.Serve(listener, p9.Proto(), p9.FSConnHandler(fs, constants.Plan9Msize)) | ||
| close(errChan) | ||
| }() | ||
|
|
||
| toReturn := new(Server) | ||
| toReturn.Listener = listener | ||
| toReturn.ExposedDir = exposeDir | ||
| toReturn.ErrChan = errChan | ||
|
|
||
| // Just before returning, check to see if we got an error off server startup. | ||
| select { | ||
| case err := <-errChan: | ||
| return nil, fmt.Errorf("starting 9p server: %w", err) | ||
| default: | ||
| logrus.Infof("Successfully started 9p server on %s for directory %s", listener.Addr().String(), exposeDir) | ||
| } | ||
|
|
||
| return toReturn, nil |
There was a problem hiding this comment.
we could also take out the server start code to a separate func (s *Server) Start() error method
Note
Together with this PR, we also need to merge
crc-org/snc#1123 to add the 9pfs client to the bundle.
This adds 9p file sharing support as discussed in #4858.
Summary by CodeRabbit
New Features
Chores