9p file sharing #4866
9p file sharing #4866
Conversation
WalkthroughAdds Plan 9 (9P) file sharing: new constants, vendored 9P library and proto, a new fs9p server, Windows daemon logic to listen and serve the host home over TCP 9P, driver updates to advertise 9P shares, and guest-side mount support for 9P during VM startup. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant Daemon
participant FS as fs9p Server
Note over Daemon: daemon startup
User->>Daemon: start daemon
Daemon->>Daemon: bind HTTP listeners (net.JoinHostPort)
alt Windows
Daemon->>FS: listen on GatewayIP:Plan9TcpPort
FS-->>Daemon: Start() serving host home over 9P
end
Daemon-->>User: signal startupDone
sequenceDiagram
autonumber
participant Starter as VM Start
participant VM
participant HostFS as Host fs9p Server
Starter->>Starter: advertise shares (CIFS + 9P)
Starter->>VM: prepare mount target (chown core:core)
VM->>HostFS: TCP connect 192.168.127.1:564
VM->>HostFS: 9P handshake & attach
HostFS-->>VM: serve filesystem ops (read/write/readdir)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
|
[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.com/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.
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.
redbeam
force-pushed
the
9p-deedlefake
branch
from
4eafa75 to
31f4ba7
Compare
There was a problem hiding this comment.
Actionable comments posted: 10
♻️ Duplicate comments (13)
cmd/crc/cmd/daemon.go (1)
253-266: Do not share the entire home by default; gate and scope the share.Expose a scoped directory (e.g., ~/.crc/share), make the feature opt‑in, and prefer read‑only. Fall back to TCP only when enabled.
Apply something like:
@@ - // 9p home directory sharing - if runtime.GOOS == "windows" { - if _, err := fs9p.StartHvsockShares([]fs9p.HvsockMount9p{{Path: constants.GetHomeDir(), HvsockGUID: constants.Plan9HvsockGUID}}); err != nil { + // 9p file sharing (Windows only, opt-in) + if runtime.GOOS == "windows" && os.Getenv("CRC_ENABLE_9P_HOME") == "1" { + shareDir := filepath.Join(constants.CrcBaseDir, "share") + if err := os.MkdirAll(shareDir, 0700); err != nil { + return fmt.Errorf("preparing 9p share dir: %w", err) + } + if _, err := fs9p.StartHvsockShares([]fs9p.HvsockMount9p{{Path: shareDir, HvsockGUID: constants.Plan9HvsockGUID}}); err != nil { logging.Warnf("Failed to start 9p file server on hvsock: %v", err) logging.Warnf("Falling back to 9p over TCP") - listener9p, err := vn.Listen("tcp", net.JoinHostPort(configuration.GatewayIP, fmt.Sprintf("%d", constants.Plan9TcpPort))) + listener9p, err := vn.Listen("tcp", net.JoinHostPort(configuration.GatewayIP, fmt.Sprintf("%d", constants.Plan9TcpPort))) if err != nil { return err } - if _, err := fs9p.StartShares([]fs9p.Mount9p{{Listener: listener9p, Path: constants.GetHomeDir()}}); err != nil { + if _, err := fs9p.StartShares([]fs9p.Mount9p{{Listener: listener9p, Path: shareDir}}); err != nil { return err } } }Optional next step: add a read‑only layer in fs9p that rejects create/write/chmod ops.
pkg/fileserver/fs9p/shares.go (1)
24-47: Remove defer-in-loop cleanup; do explicit cleanup on failure.The closure relies on a named return and captures loop vars; replace with clear cleanup to avoid surprises.
-// StartShares starts a new 9p server for each of the supplied mounts. -func StartShares(mounts []Mount9p) (servers []*Server, defErr error) { - servers9p := []*Server{} +// StartShares starts a new 9p server for each of the supplied mounts. +func StartShares(mounts []Mount9p) ([]*Server, error) { + servers9p := make([]*Server, 0, len(mounts)) + cleanup := func() { + for _, s := range servers9p { + if err := s.Stop(); err != nil { + logrus.Errorf("Error stopping 9p server during cleanup: %v", err) + } + } + } for _, m := range mounts { server, err := New9pServer(m.Listener, m.Path) if err != nil { - return nil, fmt.Errorf("serving directory %s on %s: %w", m.Path, m.Listener.Addr().String(), err) + cleanup() + return nil, fmt.Errorf("serving directory %s on %s: %w", m.Path, m.Listener.Addr().String(), err) } err = server.Start() if err != nil { - return nil, fmt.Errorf("starting 9p server for directory %s: %w", m.Path, err) + cleanup() + return nil, fmt.Errorf("starting 9p server for directory %s: %w", m.Path, err) } servers9p = append(servers9p, server) - - defer func() { - if defErr != nil { - if err := server.Stop(); err != nil { - logrus.Errorf("Error stopping 9p server: %v", err) - } - } - }()vendor/github.com/DeedleFake/p9/proto/proto.go (1)
101-108: Remove dead code and drain invalid message payload.err is nil here; also drain remaining bytes to keep stream aligned.
t := p.TypeFromID(msgType) if t == nil { - if err != nil { - return nil, NoTag, err - } - - return nil, NoTag, util.Errorf("receive: invalid message type: %v", msgType) + // Drain rest of payload so the connection can recover. + _, _ = io.Copy(io.Discard, lr) + return nil, NoTag, util.Errorf("receive: invalid message type: %v", msgType) }vendor/github.com/DeedleFake/p9/proto/server.go (1)
70-78: Treat receive errors as fatal for the connection.After a framing/deserialize error the stream is misaligned; bail out.
tmsg, tag, err := p.Receive(c, msize) if err != nil { if err == io.EOF { return } log.Printf("Error reading message: %v", err) - continue + return }vendor/github.com/DeedleFake/p9/fs.go (4)
12-12: Remove unsafe string-to-bytes; drop unused unsafe importReplace the unsafe conversion and remove the unused import.
@@ -import ( +import ( "bytes" "crypto/sha256" "encoding/binary" "fmt" "io" "path" "sync" "time" - "unsafe" @@ - sum := sha256.Sum256(*(*[]byte)(unsafe.Pointer(&p))) + sum := sha256.Sum256([]byte(p))Also applies to: 179-181
121-130: Make directory reads spec-compliant by validating offsetsTrack and enforce dir read offsets per 9P: offset must be 0 or previous offset + previous read length.
@@ type fsFile struct { sync.RWMutex path string a Attachment file File - dir bytes.Buffer + dir bytes.Buffer + // last directory read offset to validate per-9P sequential reads + dirOff int64 } @@ switch { case qid.Type&QTDir != 0: - if msg.Offset == 0 { + if msg.Offset == 0 { dir, err := file.file.Readdir() if err != nil { return &Rerror{ Ename: err.Error(), } } @@ file.dir.Reset() err = WriteDir(&file.dir, dir) if err != nil { return &Rerror{ Ename: err.Error(), } } - } + file.dirOff = 0 + } else if int64(msg.Offset) != file.dirOff { + return &Rerror{Ename: "invalid directory read offset"} + } - // This technically isn't quite accurate to the 9P specification. - // The specification states that all reads of a directory must - // either be at offset 0 or at the previous offset plus the length - // of the previous read. Instead, this implemenation just ignores - // the offset if it's not zero. This is backwards compatible with - // the specification, however, so it's probably not really an - // issue. tmp, err := file.dir.Read(buf) if (err != nil) && (err != io.EOF) { return &Rerror{ Ename: err.Error(), } } n = tmp + file.dirOff += int64(n)Also applies to: 468-513
536-538: Use exclusive lock for writesWrites mutate state; RLock allows concurrent writes -> data races.
- file.RLock() // Somewhat ironic that this doesn't require a - defer file.RUnlock() // full lock like read() does. + file.Lock() + defer file.Unlock()
593-598: Fix incorrect error detection after clunkclunk returns protocol responses; check for *Rerror, not error.
rsp := h.clunk(&Tclunk{ FID: msg.FID, }) - if _, ok := rsp.(error); ok { + if _, ok := rsp.(*Rerror); ok { return rsp }vendor/github.com/DeedleFake/p9/stat.go (1)
9-13: Avoid unsafe conversion in FileMode.String; remove unsafe importUse safe string(buf) and drop unsafe import.
@@ import ( "bytes" "errors" "io" "os" "time" - "unsafe" @@ - return *(*string)(unsafe.Pointer(&buf)) + return string(buf)Also applies to: 152-153
vendor/github.com/DeedleFake/p9/dir.go (2)
105-122: Directory create: don’t open with file flags; return a directory handleOpening a directory with os.OpenFile + flags is invalid. Create dir, then open it read-only.
func (d Dir) Create(p string, perm FileMode, mode uint8) (File, error) { p = d.path(p) if perm&ModeDir != 0 { err := os.Mkdir(p, os.FileMode(perm.Perm())) if err != nil { return nil, err } + f, err := os.Open(p) + return &dirFile{File: f}, err } flag := toOSFlags(mode) file, err := os.OpenFile(p, flag|os.O_CREATE, os.FileMode(perm.Perm())) return &dirFile{ File: file, }, err }
239-260: Mutually exclusive access flagsORDWR, OWRITE, OREAD should be exclusive; current logic can combine them.
func toOSFlags(mode uint8) (flag int) { - if mode&OREAD != 0 { - flag |= os.O_RDONLY - } - if mode&OWRITE != 0 { - flag |= os.O_WRONLY - } - if mode&ORDWR != 0 { - flag |= os.O_RDWR - } + if mode&ORDWR != 0 { + flag |= os.O_RDWR + } else if mode&OWRITE != 0 { + flag |= os.O_WRONLY + } else { + // O_RDONLY is 0; nothing to set + } if mode&OTRUNC != 0 { flag |= os.O_TRUNC } //if mode&OEXCL != 0 { // flag |= os.O_EXCL //} //if mode&OAPPEND != 0 { // flag |= os.O_APPEND //} return flag }vendor/github.com/DeedleFake/p9/msg.go (2)
207-217: Return early on encode error to avoid partial writes (Rstat.P9Encode)Stop after the first failed write.
func (stat *Rstat) P9Encode() ([]byte, error) { var buf bytes.Buffer err := proto.Write(&buf, stat.Stat.size()+2) if err != nil { return nil, err } err = proto.Write(&buf, stat.Stat) - return buf.Bytes(), err + if err != nil { + return nil, err + } + return buf.Bytes(), nil }
240-255: Return early on encode error to avoid partial writes (Twstat.P9Encode)Same pattern as above.
func (stat *Twstat) P9Encode() ([]byte, error) { var buf bytes.Buffer err := proto.Write(&buf, stat.FID) if err != nil { return nil, err } err = proto.Write(&buf, stat.Stat.size()+2) if err != nil { return nil, err } err = proto.Write(&buf, stat.Stat) - return buf.Bytes(), err + if err != nil { + return nil, err + } + return buf.Bytes(), nil }
🧹 Nitpick comments (9)
vendor/github.com/DeedleFake/p9/addr_other.go (3)
5-9: Add strings import required by sanitization.import ( "os" "os/user" "path/filepath" + "strings" )
11-16: Doc tweak: clarify behavior for this non-Linux/Darwin variant.Current comment references Unix-like
/tmp/...which doesn’t apply here.-// NamespaceDir returns the path of the directory that is used for the -// -// current namespace. On Unix-like systems, this is -// -// /tmp/ns.$USER.$DISPLAY. +// NamespaceDir returns the path of the directory used for the +// current namespace on non-Linux/Darwin systems. Here it is: +// os.TempDir()/ns.<user>.<display> +// On platforms with restrictive filename characters (e.g., Windows), +// components are sanitized; DISPLAY defaults to "0" if unset.
18-21: Optional: avoid panic on user lookup failure.If you want to harden the vendored lib against environment quirks, fall back to a generic username instead of panicking.
u, err := user.Current() uname := "unknown" if err == nil && u.Username != "" { uname = u.Username }Note: if keeping behavior parity with upstream is a goal, skip this change and keep the panic.
vendor/github.com/DeedleFake/p9/dir_windows.go (1)
9-27: Minor: use LastWriteTime for MTime for consistency with ATime source.Keeps both timestamps from the same source; avoids subtle rounding differences.
- return DirEntry{ - FileMode: ModeFromOS(fi.Mode()), - ATime: time.Unix(0, sys.LastAccessTime.Nanoseconds()), - MTime: fi.ModTime(), - Length: uint64(fi.Size()), - EntryName: fi.Name(), - } + return DirEntry{ + FileMode: ModeFromOS(fi.Mode()), + ATime: time.Unix(0, sys.LastAccessTime.Nanoseconds()), + MTime: time.Unix(0, sys.LastWriteTime.Nanoseconds()), + Length: uint64(fi.Size()), + EntryName: fi.Name(), + }vendor/github.com/DeedleFake/p9/dir_plan9.go (2)
21-30: Populate QID fields in DirEntry when available.Include Path and Version from sys.Qid to avoid losing QID info in directory listings.
return DirEntry{ FileMode: ModeFromOS(fi.Mode()), ATime: time.Unix(int64(sys.Atime), 0), MTime: fi.ModTime(), Length: uint64(fi.Size()), EntryName: fi.Name(), UID: sys.Uid, GID: sys.Gid, MUID: sys.Muid, + Path: sys.Qid.Path, + Version: sys.Qid.Vers, }
39-42: Improve error to include path context.Helps debugging when QID extraction fails.
-import "errors" +import ( + "errors" + "fmt" +) - return QID{}, errors.New("failed to get QID: FileInfo was not Dir") + return QID{}, fmt.Errorf("failed to get QID for %q: FileInfo.Sys() was not *syscall.Dir", d.path(p))vendor/github.com/DeedleFake/p9/remote.go (2)
273-275: Use io.ErrShortWrite for partial writes instead of io.EOF.EOF on write is misleading; ErrShortWrite is the standard signal.
- if write.Count < uint32(len(data)) { - return int(write.Count), io.EOF - } + if write.Count < uint32(len(data)) { + return int(write.Count), io.ErrShortWrite + }
323-331: Avoid receiver shadowing in Stat.Local var named file shadows the receiver and harms readability.
- if p != "" { - file, err := file.walk(p) + if p != "" { + rf, err := file.walk(p) if err != nil { return DirEntry{}, err } - defer file.Close() + defer rf.Close() - return file.Stat("") + return rf.Stat("") }vendor/github.com/DeedleFake/p9/proto/server.go (1)
94-97: Close on send errors to avoid half-open sessions.If writes fail, the connection is likely broken.
err := p.Send(c, tag, rmsg) if err != nil { log.Printf("Error writing message: %v", err) + return }
📜 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 (36)
cmd/crc/cmd/daemon.go(5 hunks)go.mod(2 hunks)packaging/windows/product.wxs.template(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)pkg/fileserver/fs9p/shares_fallback.go(1 hunks)pkg/fileserver/fs9p/shares_windows.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)
✅ Files skipped from review due to trivial changes (2)
- vendor/github.com/DeedleFake/p9/LICENSE
- vendor/github.com/DeedleFake/p9/doc.go
🚧 Files skipped from review as they are similar to previous changes (16)
- vendor/github.com/DeedleFake/p9/internal/debug/nodebug.go
- vendor/github.com/DeedleFake/p9/addr_unix.go
- pkg/crc/machine/libhvee/driver_windows.go
- vendor/github.com/DeedleFake/p9/encoding.go
- vendor/github.com/DeedleFake/p9/dir_other.go
- vendor/github.com/DeedleFake/p9/proto/client.go
- vendor/github.com/DeedleFake/p9/internal/util/util.go
- vendor/github.com/DeedleFake/p9/client.go
- vendor/github.com/DeedleFake/p9/internal/debug/debug.go
- pkg/fileserver/fs9p/server.go
- vendor/github.com/DeedleFake/p9/addr.go
- vendor/github.com/DeedleFake/p9/p9.go
- pkg/crc/machine/start.go
- vendor/github.com/DeedleFake/p9/dir_linux.go
- vendor/github.com/DeedleFake/p9/dir_darwin.go
- vendor/github.com/DeedleFake/p9/proto/encoding.go
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: redbeam
PR: crc-org/crc#4866
File: pkg/fileserver/fs9p/shares.go:0-0
Timestamp: 2025-08-06T09:48:10.441Z
Learning: The vsock implementation in pkg/fileserver/fs9p/shares.go was temporarily removed because 9pfs doesn't fully support it yet. It will be added back when 9pfs provides proper vsock support.
📚 Learning: 2025-08-06T09:48:10.441Z
Learnt from: redbeam
PR: crc-org/crc#4866
File: pkg/fileserver/fs9p/shares.go:0-0
Timestamp: 2025-08-06T09:48:10.441Z
Learning: The vsock implementation in pkg/fileserver/fs9p/shares.go was temporarily removed because 9pfs doesn't fully support it yet. It will be added back when 9pfs provides proper vsock support.
Applied to files:
pkg/fileserver/fs9p/shares_windows.gopkg/fileserver/fs9p/shares_fallback.gocmd/crc/cmd/daemon.gopkg/fileserver/fs9p/shares.govendor/github.com/DeedleFake/p9/fs.govendor/github.com/DeedleFake/p9/README.md
🧬 Code graph analysis (14)
pkg/fileserver/fs9p/shares_windows.go (3)
pkg/fileserver/fs9p/shares_fallback.go (1)
StartHvsockShares(11-13)pkg/fileserver/fs9p/shares.go (3)
HvsockMount9p(19-22)Mount9p(12-15)StartShares(25-61)pkg/fileserver/fs9p/server.go (1)
Server(15-24)
pkg/fileserver/fs9p/shares_fallback.go (3)
pkg/fileserver/fs9p/shares_windows.go (1)
StartHvsockShares(12-33)pkg/fileserver/fs9p/shares.go (1)
HvsockMount9p(19-22)pkg/fileserver/fs9p/server.go (1)
Server(15-24)
cmd/crc/cmd/daemon.go (5)
pkg/fileserver/fs9p/shares_fallback.go (1)
StartHvsockShares(11-13)pkg/fileserver/fs9p/shares_windows.go (1)
StartHvsockShares(12-33)pkg/fileserver/fs9p/shares.go (3)
HvsockMount9p(19-22)StartShares(25-61)Mount9p(12-15)pkg/crc/constants/constants.go (3)
GetHomeDir(166-172)Plan9HvsockGUID(61-61)Plan9TcpPort(60-60)pkg/crc/logging/logging.go (1)
Warnf(100-102)
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/addr_other.go (1)
vendor/github.com/DeedleFake/p9/addr_unix.go (1)
NamespaceDir(17-29)
pkg/fileserver/fs9p/shares.go (1)
pkg/fileserver/fs9p/server.go (2)
Server(15-24)New9pServer(29-52)
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/stat.go (3)
vendor/github.com/DeedleFake/p9/p9.go (3)
QIDType(49-49)QID(42-46)Version(10-10)vendor/github.com/DeedleFake/p9/proto/encoding.go (3)
Write(25-31)Read(34-38)Size(14-20)vendor/github.com/DeedleFake/p9/internal/util/util.go (1)
LimitedReader(13-17)
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/proto/proto.go (2)
vendor/github.com/DeedleFake/p9/proto/encoding.go (3)
Read(34-38)Write(25-31)Size(14-20)vendor/github.com/DeedleFake/p9/internal/util/util.go (2)
Errorf(44-52)LimitedReader(13-17)
vendor/github.com/DeedleFake/p9/remote.go (7)
vendor/github.com/DeedleFake/p9/client.go (1)
Client(22-26)vendor/github.com/DeedleFake/p9/proto/client.go (1)
Client(18-31)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/fs.go (5)
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/p9.go (7)
QID(42-46)QIDType(49-49)IOHeaderSize(70-70)Version(10-10)QTAuth(58-58)NoFID(18-18)QTDir(54-54)vendor/github.com/DeedleFake/p9/proto/server.go (1)
MessageHandler(135-139)vendor/github.com/DeedleFake/p9/msg.go (27)
Tversion(79-82)Tversion(84-84)Rerror(118-120)Rversion(86-89)Tauth(95-99)Rauth(101-103)Tflush(126-128)Tattach(105-110)Rattach(112-114)Twalk(133-137)Rwalk(139-141)Topen(143-146)Ropen(148-151)Tcreate(153-158)Rcreate(160-163)Tread(165-169)Rread(171-173)Twrite(175-179)Rwrite(181-183)Tclunk(185-187)Rclunk(189-190)Tremove(192-194)Rremove(196-197)Tstat(199-201)Rstat(203-205)Twstat(235-238)Rwstat(278-279)vendor/github.com/DeedleFake/p9/encoding.go (1)
WriteDir(33-42)
vendor/github.com/DeedleFake/p9/msg.go (5)
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/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/dir.go (3)
vendor/github.com/DeedleFake/p9/stat.go (5)
Stat(156-168)DirEntry(255-267)StatChanges(316-318)FileMode(23-23)ModeDir(27-27)vendor/github.com/DeedleFake/p9/fs.go (4)
File(100-119)Attachment(44-74)FileSystem(21-34)QIDFS(89-91)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 (macOS-14, 1.24)
- GitHub Check: build (ubuntu-latest, 1.24)
- GitHub Check: build (ubuntu-22.04, 1.24)
- GitHub Check: build (macOS-13, 1.24)
- GitHub Check: build (macOS-13, 1.24)
- GitHub Check: Run OKD bundle with crc (1.24)
- GitHub Check: build (windows-2022, 1.24)
- GitHub Check: build (macOS-14, 1.24)
- GitHub Check: build (ubuntu-latest, 1.24)
- GitHub Check: build (windows-2022, 1.24)
- GitHub Check: verify-devcontainer
- GitHub Check: build-installer (windows-2022, 1.24)
- GitHub Check: build-qe (darwin, arm64)
- GitHub Check: build-qe (linux, amd64)
- GitHub Check: build-qe (windows, amd64)
- GitHub Check: build-qe (darwin, amd64)
- GitHub Check: build-qe (linux, arm64)
- GitHub Check: Konflux kflux-prd-rh02 / crc-binary-on-pull-request
🔇 Additional comments (7)
packaging/windows/product.wxs.template (1)
75-79: Registering the 9P hvsock GCS channel looks correct.Component has KeyPath and is referenced by the feature; aligns with the GUID in constants. Uninstall will cleanly remove the value.
Also applies to: 138-138
vendor/github.com/DeedleFake/p9/README.md (1)
1-52: Vendor docs — leave as-is.Upstream formatting nits from linters can be ignored for vendored README.
pkg/fileserver/fs9p/shares_fallback.go (1)
1-13: Non-Windows stub is fine.Clear error on unsupported platforms keeps builds green.
cmd/crc/cmd/daemon.go (2)
182-186: Good: use net.JoinHostPort for HTTP listeners.Also applies to: 198-201
14-14: New imports are appropriate for platform gating and 9P wiring.Also applies to: 28-28
go.mod (1)
7-7: Deps verified — p9 vendored with LICENSE; linuxkit/virtsock direct; no other 9P libs found.vendor/github.com/DeedleFake/p9 contains LICENSE; go.mod pins github.com/DeedleFake/p9 v0.6.12 (line 7) and github.com/linuxkit/virtsock is direct (line 34); repository imports 9P only via github.com/DeedleFake/p9 (pkg/fileserver/fs9p/server.go) and uses hvsock via github.com/linuxkit/virtsock/pkg/hvsock (pkg/fileserver/fs9p/shares_windows.go).
pkg/crc/constants/constants.go (1)
59-62: Document new constants; confirm unused ones or remove.Add brief comments for each constant in pkg/crc/constants/constants.go (lines 59–62). Plan9HvsockPort appears unused in this PR; either wire it or drop it to avoid confusion. Ripgrep usage check returned "No files were searched" — manual verification of usages recommended.
- Plan9Msize = 1024 * 1024 + // Plan9Msize is the negotiated maximum message size for 9P I/O (bytes). + Plan9Msize = 1024 * 1024 - Plan9TcpPort = 564 + // Plan9TcpPort is the default TCP port for 9P service inside the VM network. + Plan9TcpPort = 564 - Plan9HvsockGUID = "00009000-FACB-11E6-BD58-64006A7986D3" + // Plan9HvsockGUID identifies the Hyper-V GCS channel used for 9P. + Plan9HvsockGUID = "00009000-FACB-11E6-BD58-64006A7986D3" - Plan9HvsockPort = 36864 + // Plan9HvsockPort (unused): remove if not required. + Plan9HvsockPort = 36864
| display, ok := os.LookupEnv("DISPLAY") | ||
| if !ok { | ||
| display = ":0" | ||
| } | ||
|
|
||
| return filepath.Join(os.TempDir(), "ns."+u.Username+"."+display) | ||
| } |
There was a problem hiding this comment.
Windows breakage: invalid path chars from DISPLAY default (":0") and possible backslash in Username.
On Windows, ":" and "" are illegal in filename components. ns.<username>.:0 will fail; user.Current().Username may include a domain prefix (DOMAIN\user), introducing "" as a path separator. This will break 9P namespace resolution on Windows (hvsock/TCP fallback paths in this PR).
Fix by sanitizing both Username and DISPLAY, stripping the domain component, and defaulting DISPLAY to "0" on non-Unix builds.
Apply this diff to the function body (imports and helper provided in separate comments):
func NamespaceDir() string {
- u, err := user.Current()
+ u, err := user.Current()
if err != nil {
panic(err)
}
- display, ok := os.LookupEnv("DISPLAY")
- if !ok {
- display = ":0"
- }
+ uname := u.Username
+ // Strip potential Windows domain prefix: DOMAIN\user -> user
+ if i := strings.LastIndex(uname, "\\"); i >= 0 {
+ uname = uname[i+1:]
+ }
+ uname = sanitizeComponent(uname)
+
+ display, ok := os.LookupEnv("DISPLAY")
+ if !ok || display == "" {
+ display = "0"
+ }
+ display = sanitizeComponent(display)
- return filepath.Join(os.TempDir(), "ns."+u.Username+"."+display)
+ return filepath.Join(os.TempDir(), "ns."+uname+"."+display)
}📝 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.
| display, ok := os.LookupEnv("DISPLAY") | |
| if !ok { | |
| display = ":0" | |
| } | |
| return filepath.Join(os.TempDir(), "ns."+u.Username+"."+display) | |
| } | |
| func NamespaceDir() string { | |
| u, err := user.Current() | |
| if err != nil { | |
| panic(err) | |
| } | |
| uname := u.Username | |
| // Strip potential Windows domain prefix: DOMAIN\user -> user | |
| if i := strings.LastIndex(uname, "\\"); i >= 0 { | |
| uname = uname[i+1:] | |
| } | |
| uname = sanitizeComponent(uname) | |
| display, ok := os.LookupEnv("DISPLAY") | |
| if !ok || display == "" { | |
| display = "0" | |
| } | |
| display = sanitizeComponent(display) | |
| return filepath.Join(os.TempDir(), "ns."+uname+"."+display) | |
| } |
🤖 Prompt for AI Agents
In vendor/github.com/DeedleFake/p9/addr_other.go around lines 23–29, sanitize
the username and DISPLAY before composing the temp path: strip any domain prefix
from u.Username (remove everything up to and including a backslash), replace or
remove illegal filename characters (e.g., '\' and ':' on Windows) from both the
username and DISPLAY, and on non-Unix builds default DISPLAY to "0" instead of
":0"; then join using filepath.Join as before. Implement these changes by
normalizing the two strings (trim spaces, remove or replace ':' and path
separators) and using the provided helper functions/imports so the returned
filename is valid on Windows and Unix.
| func (d Dir) path(p string) string { | ||
| return filepath.Join(string(d), filepath.FromSlash(p)) | ||
| } |
There was a problem hiding this comment.
Windows path escape: absolute p overrides base
On Windows, Join(base, FromSlash(p)) with leading slash makes p absolute, escaping the share root. Sanitize to a relative path first.
@@
func (d Dir) path(p string) string {
- return filepath.Join(string(d), filepath.FromSlash(p))
+ ps := filepath.FromSlash(p)
+ // Trim any leading separators to force relative join
+ for len(ps) > 0 && (ps[0] == os.PathSeparator) {
+ ps = ps[1:]
+ }
+ return filepath.Join(string(d), ps)
}📝 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.
| func (d Dir) path(p string) string { | |
| return filepath.Join(string(d), filepath.FromSlash(p)) | |
| } | |
| func (d Dir) path(p string) string { | |
| ps := filepath.FromSlash(p) | |
| // Trim any leading separators to force relative join | |
| for len(ps) > 0 && (ps[0] == os.PathSeparator) { | |
| ps = ps[1:] | |
| } | |
| return filepath.Join(string(d), ps) | |
| } |
| func (h *fsHandler) flush(msg *Tflush) interface{} { | ||
| // TODO: Implement this. | ||
| return &Rerror{ | ||
| Ename: "flush is not supported", | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Tflush unsupported — confirm client compatibility or return Rflush
Returning an error for flush can break clients that rely on it. Consider returning Rflush and documenting that cancellation isn’t guaranteed, or gate this behind a capability.
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "PWD: $(pwd)"
echo "---- Searching for Tflush/Rflush/flush occurrences ----"
rg -n --hidden --no-ignore -S '\bTflush\b|\bRflush\b|\bflush\s*\(' || true
echo
echo "---- Showing vendor/github.com/DeedleFake/p9/fs.go (first 400 lines) ----"
if [ -f vendor/github.com/DeedleFake/p9/fs.go ]; then
sed -n '1,400p' vendor/github.com/DeedleFake/p9/fs.go || true
else
echo "vendor/github.com/DeedleFake/p9/fs.go not found"
fi
echo
echo "---- Find fsHandler type and methods ----"
rg -n --hidden --no-ignore -S 'type\s+fsHandler\b' || true
rg -n --hidden --no-ignore -S 'func\s+\(h\s+\*\s*fsHandler\)\s+\w+' || true
echo
echo "---- Search for 'not supported' strings in p9 package ----"
rg -n --hidden --no-ignore -S 'is not supported' vendor/github.com/DeedleFake/p9 || true
echo
echo "---- Search for BUG/Tflush notes in package docs ----"
rg -n --hidden --no-ignore -S 'Tflush' || trueLength of output: 21611
Tflush unsupported — return Rflush or implement cancellation (vendor/.../p9/fs.go:244–249; BUG noted at vendor/.../p9/fs.go:147)
Returning an Rerror breaks clients that expect flush to cancel pending calls. Options:
- Return &Rflush{} (and document that cancellation may not be guaranteed).
- Implement cancellable ReadAt/WriteAt so Tflush can abort in-flight ops.
- Gate flush behavior behind a capability and document the limitation.
| func (h *fsHandler) Close() error { | ||
| h.fids.Range(func(k, v interface{}) bool { | ||
| file := v.(*fsFile) | ||
| if file.file != nil { | ||
| file.file.Close() | ||
| } | ||
| return true | ||
| }) | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
Close should lock per-file before closing
Avoid races with in-flight ops by locking file objects during Close().
func (h *fsHandler) Close() error {
h.fids.Range(func(k, v interface{}) bool {
file := v.(*fsFile)
- if file.file != nil {
- file.file.Close()
- }
+ file.Lock()
+ if file.file != nil {
+ _ = file.file.Close()
+ }
+ file.Unlock()
return true
})
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.
| func (h *fsHandler) Close() error { | |
| h.fids.Range(func(k, v interface{}) bool { | |
| file := v.(*fsFile) | |
| if file.file != nil { | |
| file.file.Close() | |
| } | |
| return true | |
| }) | |
| return nil | |
| } | |
| func (h *fsHandler) Close() error { | |
| h.fids.Range(func(k, v interface{}) bool { | |
| file := v.(*fsFile) | |
| file.Lock() | |
| if file.file != nil { | |
| _ = file.file.Close() | |
| } | |
| file.Unlock() | |
| return true | |
| }) | |
| return nil | |
| } |
🤖 Prompt for AI Agents
In vendor/github.com/DeedleFake/p9/fs.go around lines 719 to 729, the Close
implementation closes file.file without acquiring the file-level lock which can
race with in-flight ops; modify the Range loop to for each v.(*fsFile) acquire
that file's mutex (e.g. file.mu.Lock() or file.Lock()), check file.file != nil,
call file.file.Close(), set file.file = nil, then release the mutex
(file.mu.Unlock() or file.Unlock()); ensure you use the actual lock field/type
on fsFile and handle nil safely.
| if (msize > 0) && (size > msize) { | ||
| return nil, NoTag, util.Errorf("receive: %w", ErrLargeMessage) | ||
| } | ||
|
|
||
| lr := &util.LimitedReader{ | ||
| R: r, | ||
| N: size, | ||
| E: ErrLargeMessage, | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Limit reader to payload (size-4) and guard short sizes.
Prevents over-reads and enforces framing strictly.
- if (msize > 0) && (size > msize) {
+ if (msize > 0) && (size > msize) {
return nil, NoTag, util.Errorf("receive: %w", ErrLargeMessage)
}
- lr := &util.LimitedReader{
+ if size < 4 {
+ return nil, NoTag, util.Errorf("receive: short message size: %d", size)
+ }
+ lr := &util.LimitedReader{
R: r,
- N: size,
+ N: size - 4,
E: ErrLargeMessage,
}📝 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.
| if (msize > 0) && (size > msize) { | |
| return nil, NoTag, util.Errorf("receive: %w", ErrLargeMessage) | |
| } | |
| lr := &util.LimitedReader{ | |
| R: r, | |
| N: size, | |
| E: ErrLargeMessage, | |
| } | |
| if (msize > 0) && (size > msize) { | |
| return nil, NoTag, util.Errorf("receive: %w", ErrLargeMessage) | |
| } | |
| if size < 4 { | |
| return nil, NoTag, util.Errorf("receive: short message size: %d", size) | |
| } | |
| lr := &util.LimitedReader{ | |
| R: r, | |
| N: size - 4, | |
| E: ErrLargeMessage, | |
| } |
🤖 Prompt for AI Agents
In vendor/github.com/DeedleFake/p9/proto/proto.go around lines 85 to 94, the
LimitedReader is currently set to read the entire declared size which can allow
over-reads; change logic to first guard against too-small frames (if size < 4
return ErrShortRead or ErrLargeMessage as appropriate) and set
util.LimitedReader.N to size-4 so it only reads the payload (excluding the
4-byte tag/length field). Ensure the short-size check happens before creating
the LimitedReader and preserve existing msize/size comparison behavior and error
returns.
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (21)
pkg/fileserver/fs9p/server.go (3)
54-69: Acknowledge previous race condition concern.A previous reviewer noted that the non-blocking select (lines 62-68) may miss early startup errors. The reviewer suggested adding a timeout (e.g., 100ms) to wait briefly for potential startup errors before returning success.
Based on learnings, you mentioned moving the server start code to a separate method (as agreed with anjannath). Ensure this refactoring is tracked if not yet completed.
71-81: Acknowledge previous zombie process concern.A previous reviewer flagged the "BAD THINGS" comment about zombie processes and suggested improving the documentation and error wrapping to help operators understand the implications of stopping the server while clients are active.
83-87: Acknowledge previous indefinite blocking concern.A previous reviewer noted that WaitForError blocks indefinitely and suggested adding context support to allow timeouts or cancellation.
cmd/crc/cmd/daemon.go (1)
253-266: Acknowledge previous security concern about sharing entire home directory.A previous reviewer flagged that exposing
constants.GetHomeDir()(line 259) shares the user's complete home directory without access controls or read-only enforcement. They recommended:
- Share a dedicated subdirectory (e.g.,
$CRC_HOME/share) instead- Enforce read-only access by default
- Implement path filters or authentication in fs9p
Additionally, consider whether 9P sharing should be conditional (opt-in via configuration) rather than automatically enabled on Windows.
#!/bin/bash # Check if there are configuration flags or user preferences for enabling 9P sharing rg -n -C3 "Enable.*9p|9p.*Enable|shared.*dir.*config" --type=govendor/github.com/DeedleFake/p9/client.go (3)
45-47: FID generator can wrap into NoFID (MaxUint32). Handle overflow or use uint64.func (c *Client) nextFID() uint32 { - return atomic.AddUint32(&c.fid, 1) - 1 + fid := atomic.AddUint32(&c.fid, 1) - 1 + if fid == NoFID { + // Skip reserved NoFID, wrap to 0. + atomic.StoreUint32(&c.fid, 0) + return 0 + } + return fid }Longer term: recycle freed FIDs to avoid unbounded growth and eventual wrap.
84-90: Add safe type assertion for Rauth response.- rauth := rsp.(*Rauth) + rauth, ok := rsp.(*Rauth) + if !ok { + return nil, fmt.Errorf("unexpected response type for auth: %T", rsp) + }
113-119: Add safe type assertion for Rattach response.- attach := rsp.(*Rattach) + attach, ok := rsp.(*Rattach) + if !ok { + return nil, fmt.Errorf("unexpected response type for attach: %T", rsp) + }vendor/github.com/DeedleFake/p9/proto/encoding.go (3)
65-71: Size-mode for custom Encoder: count bytes explicitlyIn size mode, explicitly add len(buf) to avoid any ambiguity with binary.Size on slices. Safer and clearer.
case Encoder: buf, err := v.P9Encode() e.err = err - e.mode(buf) + if e.mode == e.size { + e.n += uint32(len(buf)) + } else { + e.mode(buf) + } return
150-153: Avoid unsafe cast when reading 16-bit lengthUnsafe pointer cast is endianness-unsafe. Read into a uint16 and widen.
- default: - d.read((*uint16)(unsafe.Pointer(&length))) + default: + var len16 uint16 + d.read(&len16) + length = uint32(len16)
168-171: Remove unsafe []byte→string conversionUse safe conversion to preserve Go’s string immutability guarantees.
buf := make([]byte, int(length)) d.read(buf) - - v.SetString(*(*string)(unsafe.Pointer(&buf))) + v.SetString(string(buf))vendor/github.com/DeedleFake/p9/proto/client.go (1)
90-97: Don’t loop forever on read errorsContinuing on non-EOF errors can cause an endless loop. Log and return (or implement backoff). Minimal fix below.
- if err != nil { - if (ctx.Err() != nil) || (err == io.EOF) { - return - } - - continue - } + if err != nil { + if (ctx.Err() != nil) || (err == io.EOF) { + return + } + log.Printf("Read error: %v", err) + return + }vendor/github.com/DeedleFake/p9/proto/server.go (1)
70-79: Skip processing after read errorContinue the loop after logging; don’t process a potentially invalid tmsg.
tmsg, tag, err := p.Receive(c, msize) if err != nil { if err == io.EOF { return } log.Printf("Error reading message: %v", err) + continue }vendor/github.com/DeedleFake/p9/stat.go (1)
152-152: Unsafe pointer conversion in vendored code.The unsafe.Pointer conversion was flagged in previous reviews. While
string(buf)would be safer, this is vendored third-party code from DeedleFake/p9. Consider reporting this upstream if it's a concern, but avoid modifying vendored dependencies directly.vendor/github.com/DeedleFake/p9/fs.go (6)
179-180: Unsafe string-to-byte conversion in vendored code.The unsafe.Pointer conversion to pass a string as
[]byteto SHA256 was previously flagged. While the function only reads the data, this violates Go's memory safety rules. This is vendored code from DeedleFake/p9 and should be addressed upstream.
499-512: Non-standard directory offset handling (documented in vendored code).The implementation's directory read behavior deviates from strict 9P specification requirements, as noted in the inline comment. The code is backward-compatible but may not work with all 9P clients. This is vendored code with an acknowledged limitation.
536-537: Potential write race condition in vendored code.Using
RLockfor write operations was previously flagged as a potential race condition. The comment acknowledges this is "ironic" but concurrent writes tofile.filecould still cause issues depending on the underlying File implementation. This is vendored code from DeedleFake/p9.
593-598: Error handling pattern in vendored code is actually correct.The previous review comment suggested changing the error type assertion. However, since
*Rerrorimplements theerrorinterface (via itsError() stringmethod at line 122-124 in msg.go), the current patternif _, ok := rsp.(error); okcorrectly detects error responses. This is vendored code.
244-249: Tflush not supported (documented limitation in vendored code).The lack of Tflush support is documented in the BUG comment at line 147 and acknowledged with a TODO. This is a known limitation of the vendored DeedleFake/p9 library that may affect clients expecting full 9P cancellation support.
719-729: Close lacks per-file locking in vendored code.The
Closemethod iterates all files and closes them without acquiring per-file locks, which could race with in-flight operations. This was previously flagged. This is vendored code from DeedleFake/p9 and should be addressed upstream if it causes issues in practice.vendor/github.com/DeedleFake/p9/msg.go (1)
207-217: Error handling in P9Encode is correct (vendored code).The previous review comment suggested improving error handling, but the current implementation correctly returns early on all intermediate errors and only returns the buffer with the final write's error. Since only the last write could fail without early return, there's no partial write issue. This is vendored code.
vendor/github.com/DeedleFake/p9/remote.go (1)
37-43: Path splitting produces empty component for absolute paths (vendored code).The previous review correctly identified that
strings.Split("/a/b", "/")produces["", "a", "b"], which includes an invalid empty component for Twalk. This is vendored code from DeedleFake/p9 and should be addressed upstream as it could cause walk failures for absolute paths.
🧹 Nitpick comments (10)
cmd/crc/cmd/daemon.go (1)
182-182: Prefer net.JoinHostPort for clarity.Good refactor using
net.JoinHostPortinstead offmt.Sprintffor constructing listener addresses. This is more idiomatic and handles IPv6 addresses correctly.Also applies to: 198-198
vendor/github.com/DeedleFake/p9/internal/util/util.go (3)
19-25: Consider pointer receiver for consistency.The
err()method uses a value receiver (line 19), whileRead()uses a pointer receiver (line 27). For consistency and to avoid copying the struct, consider changing line 19 to:-func (lr LimitedReader) err() error { +func (lr *LimitedReader) err() error {This is a minor issue since the method only reads fields, but pointer receivers are more conventional for struct methods.
27-39: Simplify condition and verify underflow handling.Two observations:
Line 28: The condition
lr.N <= 0can be simplified tolr.N == 0since N isuint32and cannot be negative.Line 37: The decrement
lr.N -= uint32(n)could underflow if the underlying reader returns more bytes than requested. However, since Go's io.Reader contract guaranteesn <= len(buf)and we've already limitedbuftolr.N(lines 32-34), this should be safe.Apply this diff to simplify the condition:
- if lr.N <= 0 { + if lr.N == 0 { return 0, lr.err() }
41-52: Verify type assertion safety in Errorf.Line 47 performs an unchecked type assertion
arg.(error). While this should be safe because we've confirmedarg == io.EOF(which is an error type), a safer approach would be:if err, ok := arg.(error); ok && err == io.EOF { return err }However, since
io.EOFis a known constant of typeerror, the current code is acceptable but slightly fragile if the logic changes.Consider adding a comment explaining why the type assertion is safe:
for _, arg := range args { - if arg == io.EOF { - return arg.(error) + // io.EOF is always of type error, so this assertion is safe + if err, ok := arg.(error); ok && err == io.EOF { + return err } }vendor/github.com/DeedleFake/p9/dir_darwin.go (1)
23-33: Optional: fallback UID/GID to numeric strings when lookups fail.Empty UID/GID can hinder clients. Consider populating with the numeric IDs if lookup fails.
- var uname string - uid, err := user.LookupId(strconv.FormatUint(uint64(sys.Uid), 10)) - if err == nil { - uname = uid.Username - } + var uname = strconv.FormatUint(uint64(sys.Uid), 10) + if u, err := user.LookupId(uname); err == nil { + uname = u.Username + } - var gname string - gid, err := user.LookupGroupId(strconv.FormatUint(uint64(sys.Gid), 10)) - if err == nil { - gname = gid.Name - } + var gname = strconv.FormatUint(uint64(sys.Gid), 10) + if g, err := user.LookupGroupId(gname); err == nil { + gname = g.Name + }vendor/github.com/DeedleFake/p9/dir_linux.go (1)
23-33: Optional: fallback UID/GID to numeric strings when lookups fail.Populate UID/GID with numeric IDs if lookups fail to avoid empty fields.
- var uname string - uid, err := user.LookupId(strconv.FormatUint(uint64(sys.Uid), 10)) - if err == nil { - uname = uid.Username - } + var uname = strconv.FormatUint(uint64(sys.Uid), 10) + if u, err := user.LookupId(uname); err == nil { + uname = u.Username + } - var gname string - gid, err := user.LookupGroupId(strconv.FormatUint(uint64(sys.Gid), 10)) - if err == nil { - gname = gid.Name - } + var gname = strconv.FormatUint(uint64(sys.Gid), 10) + if g, err := user.LookupGroupId(gname); err == nil { + gname = g.Name + }vendor/github.com/DeedleFake/p9/proto/encoding.go (1)
3-11: Drop unused unsafe import (after applying above changes)Once unsafe sites are removed, the import becomes unused.
import ( "encoding/binary" "io" "reflect" "time" - "unsafe" "github.com/DeedleFake/p9/internal/util" )vendor/github.com/DeedleFake/p9/proto/client.go (2)
69-75: Graceful shutdown: wait for goroutines to exitCancel, wait for coord to close done, then close the conn to avoid races.
func (c *Client) Close() error { - c.cancel() - return c.c.Close() + c.cancel() + <-c.done + return c.c.Close() }
80-88: Make read timeout configurable (nice-to-have)Hard-coded 10s may not fit all environments. Consider a field or option to tune it.
vendor/github.com/DeedleFake/p9/proto/server.go (1)
61-68: Concurrent writes to net.Conn may interleaveIf proto.Send does multiple writes, responses can interleave across goroutines. Either guarantee atomic writes in proto.Send or guard with a per-conn write mutex. Minimal guard shown.
func handleMessages(c net.Conn, p Proto, handler MessageHandler) { var setter sync.Once var msize uint32 + var wmu sync.Mutex @@ - err := p.Send(c, ctag, rmsg) + wmu.Lock() + err := p.Send(c, ctag, rmsg) + wmu.Unlock() if err != nil { log.Printf("Error writing message: %v", err) }To verify if the guard is needed, please check whether proto.Send writes the entire message in a single conn.Write call.
Also applies to: 94-97
📜 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 (33)
cmd/crc/cmd/daemon.go(5 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)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)
✅ Files skipped from review due to trivial changes (1)
- vendor/github.com/DeedleFake/p9/internal/debug/nodebug.go
🚧 Files skipped from review as they are similar to previous changes (13)
- vendor/github.com/DeedleFake/p9/LICENSE
- pkg/crc/machine/libhvee/driver_windows.go
- vendor/github.com/DeedleFake/p9/encoding.go
- vendor/github.com/DeedleFake/p9/addr_other.go
- vendor/github.com/DeedleFake/p9/dir_other.go
- vendor/github.com/DeedleFake/p9/proto/proto.go
- pkg/crc/constants/constants.go
- vendor/github.com/DeedleFake/p9/dir.go
- vendor/github.com/DeedleFake/p9/addr_unix.go
- go.mod
- vendor/github.com/DeedleFake/p9/p9.go
- vendor/github.com/DeedleFake/p9/dir_plan9.go
- vendor/github.com/DeedleFake/p9/doc.go
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: redbeam
PR: crc-org/crc#4866
File: pkg/fileserver/fs9p/shares.go:0-0
Timestamp: 2025-08-06T09:48:10.441Z
Learning: The vsock implementation in pkg/fileserver/fs9p/shares.go was temporarily removed because 9pfs doesn't fully support it yet. It will be added back when 9pfs provides proper vsock support.
🧬 Code graph analysis (17)
vendor/github.com/DeedleFake/p9/proto/client.go (6)
vendor/github.com/DeedleFake/p9/msg.go (1)
Proto(75-77)vendor/github.com/DeedleFake/p9/proto/proto.go (3)
Proto(38-41)NoTag(27-27)ErrClientClosed(22-22)vendor/github.com/DeedleFake/p9/client.go (3)
NewClient(30-32)Client(22-26)Dial(36-43)vendor/github.com/DeedleFake/p9/internal/debug/debug.go (1)
Log(10-12)vendor/github.com/DeedleFake/p9/internal/debug/nodebug.go (1)
Log(5-5)vendor/github.com/DeedleFake/p9/p9.go (1)
NoTag(15-15)
pkg/crc/machine/start.go (1)
pkg/os/exec.go (1)
RunPrivileged(48-59)
vendor/github.com/DeedleFake/p9/dir_linux.go (3)
vendor/github.com/DeedleFake/p9/stat.go (4)
DirEntry(255-267)FileMode(23-23)ModeFromOS(43-78)Stat(156-168)vendor/github.com/DeedleFake/p9/dir.go (1)
Dir(16-16)vendor/github.com/DeedleFake/p9/p9.go (3)
QID(42-46)QIDType(49-49)Version(10-10)
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/dir_darwin.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 (3)
QID(42-46)QIDType(49-49)Version(10-10)
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)
cmd/crc/cmd/daemon.go (2)
pkg/crc/constants/constants.go (2)
Plan9TcpPort(60-60)GetHomeDir(164-170)pkg/fileserver/fs9p/server.go (1)
New9pServer(29-52)
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/remote.go (7)
vendor/github.com/DeedleFake/p9/client.go (1)
Client(22-26)vendor/github.com/DeedleFake/p9/proto/client.go (1)
Client(18-31)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/proto/encoding.go (1)
vendor/github.com/DeedleFake/p9/internal/util/util.go (1)
Errorf(44-52)
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_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/fs.go (8)
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/p9.go (7)
QID(42-46)QIDType(49-49)IOHeaderSize(70-70)Version(10-10)QTAuth(58-58)NoFID(18-18)QTDir(54-54)vendor/github.com/DeedleFake/p9/proto/server.go (3)
MessageHandler(135-139)ConnHandler(112-114)ConnHandlerFunc(125-125)vendor/github.com/DeedleFake/p9/internal/debug/debug.go (1)
Log(10-12)vendor/github.com/DeedleFake/p9/internal/debug/nodebug.go (1)
Log(5-5)vendor/github.com/DeedleFake/p9/msg.go (27)
Tversion(79-82)Tversion(84-84)Rerror(118-120)Rversion(86-89)Tauth(95-99)Rauth(101-103)Tflush(126-128)Tattach(105-110)Rattach(112-114)Twalk(133-137)Rwalk(139-141)Topen(143-146)Ropen(148-151)Tcreate(153-158)Rcreate(160-163)Tread(165-169)Rread(171-173)Twrite(175-179)Rwrite(181-183)Tclunk(185-187)Rclunk(189-190)Tremove(192-194)Rremove(196-197)Tstat(199-201)Rstat(203-205)Twstat(235-238)Rwstat(278-279)vendor/github.com/DeedleFake/p9/client.go (1)
ErrUnsupportedVersion(14-14)vendor/github.com/DeedleFake/p9/encoding.go (1)
WriteDir(33-42)
pkg/fileserver/fs9p/server.go (3)
vendor/github.com/DeedleFake/p9/fs.go (2)
FileSystem(21-34)FSConnHandler(162-167)vendor/github.com/DeedleFake/p9/proto/server.go (1)
Serve(17-42)pkg/crc/constants/constants.go (1)
Plan9Msize(59-59)
vendor/github.com/DeedleFake/p9/client.go (5)
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/proto/proto.go (1)
Proto(38-41)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/internal/util/util.go (1)
vendor/github.com/DeedleFake/p9/proto/encoding.go (1)
Read(34-38)
vendor/github.com/DeedleFake/p9/stat.go (3)
vendor/github.com/DeedleFake/p9/p9.go (3)
QIDType(49-49)QID(42-46)Version(10-10)vendor/github.com/DeedleFake/p9/proto/encoding.go (3)
Write(25-31)Read(34-38)Size(14-20)vendor/github.com/DeedleFake/p9/internal/util/util.go (1)
LimitedReader(13-17)
🪛 markdownlint-cli2 (0.18.1)
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). (17)
- GitHub Check: build (windows-2022, 1.24)
- GitHub Check: build (ubuntu-latest, 1.24)
- GitHub Check: verify-devcontainer
- GitHub Check: Run OKD bundle with crc (1.24)
- GitHub Check: build-qe (darwin, arm64)
- GitHub Check: build (macOS-13, 1.24)
- GitHub Check: build (macOS-13, 1.24)
- GitHub Check: build (ubuntu-22.04, 1.24)
- GitHub Check: build (ubuntu-latest, 1.24)
- GitHub Check: build (macOS-14, 1.24)
- GitHub Check: build-installer (windows-2022, 1.24)
- GitHub Check: build-qe (linux, arm64)
- GitHub Check: build-qe (windows, amd64)
- GitHub Check: build-qe (darwin, amd64)
- GitHub Check: build (windows-2022, 1.24)
- GitHub Check: build-qe (linux, amd64)
- GitHub Check: Konflux kflux-prd-rh02 / crc-binary-on-pull-request
🔇 Additional comments (11)
vendor/github.com/DeedleFake/p9/internal/debug/debug.go (1)
10-12: LGTM: Simple and appropriate debug logger for vendored library.The Log function provides conditional debug output to stderr when the
p9debugbuild tag is set, complementing the no-op variant in nodebug.go. The implementation is minimal and appropriate for debug logging in a vendored 9P library.vendor/github.com/DeedleFake/p9/README.md (1)
1-51: Vendored documentation looks appropriate.This is documentation from the upstream DeedleFake/p9 library. The examples are clear and demonstrate basic server/client usage. The static analysis warnings (hard tabs, heading style) are from the upstream library and should not be modified here.
vendor/github.com/DeedleFake/p9/addr.go (1)
1-78: Vendored address parsing utilities look appropriate.This is part of the upstream DeedleFake/p9 library. The address parsing logic correctly handles multiple 9P address formats (namespace, Unix socket, TCP with various delimiters). The code integrates with platform-specific namespace directory utilities (
NamespaceDir()fromaddr_unix.goandaddr_other.go).As vendored code, this should remain unchanged from upstream. Any improvements or concerns should be addressed in the upstream repository.
vendor/modules.txt (1)
13-18: Verify p9 fork declaration and documentation
- Check go.mod contains
require github.com/DeedleFake/p9 v0.6.12(within a require block) and noreplacefrom hugelgupf/p9.- Ensure no remaining imports of
github.com/hugelgupf/p9outside vendor.- Confirm vendor/github.com/DeedleFake/p9/LICENSE is included.
- Add a brief note in go.mod comments or a deps.md explaining the fork’s intent, license (MIT), and update process for future maintainers.
pkg/fileserver/fs9p/server.go (2)
15-24: LGTM! Server structure is well-defined.The Server struct encapsulates all necessary components for a 9P file server: the network listener, filesystem abstraction, exposed directory path, and error channel.
26-52: LGTM! Constructor validates inputs properly.The New9pServer function correctly validates that the exposed directory is absolute, exists, and is a directory before creating the filesystem wrapper.
cmd/crc/cmd/daemon.go (2)
14-14: LGTM! Runtime import added for OS detection.The runtime package is appropriately imported to support Windows-specific 9P sharing logic.
28-28: LGTM! fs9p package imported for 9P server.The fs9p import is correctly added to support the new 9P file sharing functionality.
vendor/github.com/DeedleFake/p9/dir_windows.go (1)
9-27: LGTM! Windows metadata extraction is appropriate.The
infoToEntryfunction correctly:
- Attempts to extract Windows-specific attributes via type assertion (line 10)
- Provides a sensible fallback when Windows attributes are unavailable (lines 12-18)
- Populates ATime from
LastAccessTimewhen available (line 22)- Uses standard FileInfo methods for cross-platform fields
The conversion from
Filetime.Nanoseconds()totime.Unix(0, ...)(line 22) is correct for translating Windows time to Go time.vendor/github.com/DeedleFake/p9/internal/util/util.go (1)
8-17: LGTM! LimitedReader structure is clear.The LimitedReader type provides a bounded reader with custom error support. Using
uint32for N allows larger sizes on 32-bit systems, as documented.pkg/crc/machine/start.go (1)
262-269: Use VSockGateway constant and validate 9pfs usage
- Replace the literal
"192.168.127.1"withconstants.VSockGatewayin pkg/crc/machine/start.go.- Verify that the
9pfsbinary is bundled and that the mount command syntax (9pfs <gateway-IP> <target>) is correct.- Annotate the privileged
chown core:core(line 264) to document its security implications.
| version := rsp.(*Rversion) | ||
| if version.Version != Version { | ||
| return 0, ErrUnsupportedVersion | ||
| } | ||
|
|
||
| c.SetMsize(version.Msize) | ||
|
|
||
| return version.Msize, nil | ||
| } |
There was a problem hiding this comment.
Guard the Rversion type assertion to avoid panic.
rsp.(*Rversion) can panic if server replies with error or unexpected type. Use two-value assertion and return a clear error.
- version := rsp.(*Rversion)
+ version, ok := rsp.(*Rversion)
+ if !ok {
+ return 0, fmt.Errorf("unexpected response type for version: %T", rsp)
+ }Also consider accepting compatible variants like "9P2000.u" if the server negotiates an extension; strict equality may reduce interoperability.
📝 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.
| version := rsp.(*Rversion) | |
| if version.Version != Version { | |
| return 0, ErrUnsupportedVersion | |
| } | |
| c.SetMsize(version.Msize) | |
| return version.Msize, nil | |
| } | |
| version, ok := rsp.(*Rversion) | |
| if !ok { | |
| return 0, fmt.Errorf("unexpected response type for version: %T", rsp) | |
| } | |
| if version.Version != Version { | |
| return 0, ErrUnsupportedVersion | |
| } | |
| c.SetMsize(version.Msize) | |
| return version.Msize, nil | |
| } |
🤖 Prompt for AI Agents
In vendor/github.com/DeedleFake/p9/client.go around lines 61 to 69, the code
uses a single-value type assertion rsp.(*Rversion) which can panic if rsp is an
error or another unexpected type; replace it with the two-value assertion (v,
ok) and if ok is false inspect whether rsp is an *Rerror and return that error
or otherwise return a clear ErrUnexpectedResponse; after safely asserting, use
v.Msize; also relax the version equality check to accept compatible variants
(e.g., use a prefix or explicit allowed list such as
strings.HasPrefix(version.Version, "9P2000") or allow "9P2000.u") so
server-negotiated extensions don’t cause ErrUnsupportedVersion.
| return DirEntry{ | ||
| FileMode: ModeFromOS(fi.Mode()), | ||
| ATime: time.Unix(sys.Atimespec.Unix()), | ||
| MTime: fi.ModTime(), | ||
| Length: uint64(fi.Size()), | ||
| EntryName: fi.Name(), | ||
| UID: uname, | ||
| GID: gname, | ||
| } |
There was a problem hiding this comment.
ATime conversion uses non-existent Timespec.Unix(); fix to Sec/Nsec fields.
syscall.Timespec on Darwin has Sec/Nsec; there’s no Unix() method. This won’t compile. Use time.Unix(sec, nsec).
- ATime: time.Unix(sys.Atimespec.Unix()),
+ ATime: time.Unix(sys.Atimespec.Sec, sys.Atimespec.Nsec),📝 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.
| return DirEntry{ | |
| FileMode: ModeFromOS(fi.Mode()), | |
| ATime: time.Unix(sys.Atimespec.Unix()), | |
| MTime: fi.ModTime(), | |
| Length: uint64(fi.Size()), | |
| EntryName: fi.Name(), | |
| UID: uname, | |
| GID: gname, | |
| } | |
| return DirEntry{ | |
| FileMode: ModeFromOS(fi.Mode()), | |
| ATime: time.Unix(sys.Atimespec.Sec, sys.Atimespec.Nsec), | |
| MTime: fi.ModTime(), | |
| Length: uint64(fi.Size()), | |
| EntryName: fi.Name(), | |
| UID: uname, | |
| GID: gname, | |
| } |
🤖 Prompt for AI Agents
In vendor/github.com/DeedleFake/p9/dir_darwin.go around lines 35 to 43, the code
calls sys.Atimespec.Unix() which doesn't exist on Darwin's syscall.Timespec;
replace the ATime construction to use time.Unix(sys.Atimespec.Sec,
sys.Atimespec.Nsec) (i.e. pass Sec and Nsec fields to time.Unix) so the access
time is built from the Timespec Sec/Nsec fields.
| return DirEntry{ | ||
| FileMode: ModeFromOS(fi.Mode()), | ||
| ATime: time.Unix(sys.Atim.Unix()), | ||
| MTime: fi.ModTime(), | ||
| Length: uint64(fi.Size()), | ||
| EntryName: fi.Name(), | ||
| UID: uname, | ||
| GID: gname, | ||
| } |
There was a problem hiding this comment.
ATime conversion uses non-existent Timespec.Unix(); fix to Sec/Nsec fields.
syscall.Timespec on Linux has Sec/Nsec; there’s no Unix() method. Use time.Unix(sec, nsec).
- ATime: time.Unix(sys.Atim.Unix()),
+ ATime: time.Unix(sys.Atim.Sec, sys.Atim.Nsec),📝 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.
| return DirEntry{ | |
| FileMode: ModeFromOS(fi.Mode()), | |
| ATime: time.Unix(sys.Atim.Unix()), | |
| MTime: fi.ModTime(), | |
| Length: uint64(fi.Size()), | |
| EntryName: fi.Name(), | |
| UID: uname, | |
| GID: gname, | |
| } | |
| return DirEntry{ | |
| FileMode: ModeFromOS(fi.Mode()), | |
| ATime: time.Unix(sys.Atim.Sec, sys.Atim.Nsec), | |
| MTime: fi.ModTime(), | |
| Length: uint64(fi.Size()), | |
| EntryName: fi.Name(), | |
| UID: uname, | |
| GID: gname, | |
| } |
🤖 Prompt for AI Agents
In vendor/github.com/DeedleFake/p9/dir_linux.go around lines 35 to 43, the ATime
conversion calls a non-existent Timespec.Unix() method; replace that call with
time.Unix using the Sec and Nsec fields (e.g., time.Unix(sys.Atim.Sec,
sys.Atim.Nsec)) so the access time is constructed from the correct Timespec
fields.
| case c.nextTag <- nextTag: | ||
| for { | ||
| nextTag++ | ||
| if _, ok := tags[nextTag]; !ok { | ||
| break | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Never allocate the reserved NoTag value
Ensure the allocator never returns NoTag, even on wrap-around.
- case c.nextTag <- nextTag:
- for {
- nextTag++
- if _, ok := tags[nextTag]; !ok {
- break
- }
- }
+ case c.nextTag <- func() uint16 {
+ // Produce a tag to return now (skip NoTag).
+ if nextTag == NoTag {
+ nextTag++
+ }
+ t := nextTag
+ // Prepare the next available tag, skipping NoTag and in-use tags.
+ for {
+ nextTag++
+ if nextTag == NoTag {
+ nextTag++
+ }
+ if _, ok := tags[nextTag]; !ok {
+ break
+ }
+ }
+ return t
+ }():📝 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.
| case c.nextTag <- nextTag: | |
| for { | |
| nextTag++ | |
| if _, ok := tags[nextTag]; !ok { | |
| break | |
| } | |
| } | |
| } | |
| case c.nextTag <- func() uint16 { | |
| // Produce a tag to return now (skip NoTag). | |
| if nextTag == NoTag { | |
| nextTag++ | |
| } | |
| t := nextTag | |
| // Prepare the next available tag, skipping NoTag and in-use tags. | |
| for { | |
| nextTag++ | |
| if nextTag == NoTag { | |
| nextTag++ | |
| } | |
| if _, ok := tags[nextTag]; !ok { | |
| break | |
| } | |
| } | |
| return t | |
| }(): |
🤖 Prompt for AI Agents
In vendor/github.com/DeedleFake/p9/proto/client.go around lines 138 to 145, the
tag allocator can return the reserved NoTag when wrapping; change the selection
loop so it never yields NoTag by advancing nextTag again if it equals NoTag (and
continue skipping any tags present in the map). Concretely, after incrementing
nextTag check both that tags[nextTag] is false and nextTag != NoTag; if nextTag
== NoTag, increment/wrap again and keep looping until you find a tag that is not
in the map and not NoTag, then send that value on c.nextTag.
| mode(func() { | ||
| rmsg := handler.HandleMessage(tmsg) | ||
| 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() | ||
| } | ||
| }) | ||
| } | ||
|
|
||
| err := p.Send(c, tag, rmsg) | ||
| if err != nil { | ||
| log.Printf("Error writing message: %v", err) | ||
| } | ||
| }) | ||
| } |
There was a problem hiding this comment.
Data race/logic bug: goroutine closes over loop vars tag/tmsg
The closure may observe mutated tag/tmsg. Copy them into locals before launching.
- mode(func() {
- rmsg := handler.HandleMessage(tmsg)
+ ctag := tag
+ ctmsg := tmsg
+ mode(func() {
+ rmsg := handler.HandleMessage(ctmsg)
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()
}
})
}
- err := p.Send(c, tag, rmsg)
+ err := p.Send(c, ctag, rmsg)
if err != nil {
log.Printf("Error writing message: %v", err)
}
})📝 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.
| mode(func() { | |
| rmsg := handler.HandleMessage(tmsg) | |
| 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() | |
| } | |
| }) | |
| } | |
| err := p.Send(c, tag, rmsg) | |
| if err != nil { | |
| log.Printf("Error writing message: %v", err) | |
| } | |
| }) | |
| } | |
| ctag := tag | |
| ctmsg := tmsg | |
| mode(func() { | |
| rmsg := handler.HandleMessage(ctmsg) | |
| 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() | |
| } | |
| }) | |
| } | |
| err := p.Send(c, ctag, rmsg) | |
| if err != nil { | |
| log.Printf("Error writing message: %v", err) | |
| } | |
| }) |
🤖 Prompt for AI Agents
In vendor/github.com/DeedleFake/p9/proto/server.go around lines 79 to 99, the
anonymous function passed to mode closes over the loop variables tag and tmsg
which can be mutated by the loop and cause a data race; fix it by copying tag
and tmsg into new local variables immediately before calling mode (e.g.,
localTag, localTmsg) and use those locals inside the closure so each goroutine
sees the intended values, leaving the rest of the logic unchanged.
9d23c41 to
f176d25
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (27)
pkg/fileserver/fs9p/server.go (3)
56-68: Wait briefly for startup errors to avoid “false success” log.
Non‑blocking select can miss immediate Accept errors; add a short timeout.func (s *Server) Start() error { go func() { s.ErrChan <- proto.Serve(s.Listener, p9.Proto(), p9.FSConnHandler(s.Filesystem, constants.Plan9Msize)) close(s.ErrChan) }() - // Just before returning, check to see if we got an error off server startup. - select { - case err := <-s.ErrChan: - return fmt.Errorf("starting 9p server: %w", err) - default: - logrus.Infof("Successfully started 9p server on %s for directory %s", s.Listener.Addr().String(), s.ExposedDir) - return nil - } + // Wait briefly for potential startup errors. + select { + case err := <-s.ErrChan: + return fmt.Errorf("starting 9p server: %w", err) + case <-time.After(100 * time.Millisecond): + logrus.WithFields(logrus.Fields{ + "addr": s.Listener.Addr().String(), + "dir": s.ExposedDir, + }).Info("Successfully started 9p server") + return nil + }
71-81: Clarify shutdown semantics and wrap errors for operators.
Warn about client impact and wrap Close error.-// 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 while clients are active may leave client processes +// unresponsive; a VM restart might be required. Ensure clients disconnect first. 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) + logrus.WithField("dir", s.ExposedDir).Info("Successfully stopped 9p server") return nil }
83-87: Provide context‑aware wait and handle net.ErrClosed as normal shutdown.
Avoid indefinite block; treat graceful Close as non‑error; keep back‑compat by delegating the old method.-// WaitForError from a running server. -func (s *Server) WaitForError() error { - err := <-s.ErrChan - return err -} +// WaitForError from a running server (back‑compat). +func (s *Server) WaitForError() error { + return s.WaitForErrorCtx(context.Background()) +} + +// WaitForErrorCtx waits for a server error or context cancellation. +func (s *Server) WaitForErrorCtx(ctx context.Context) error { + select { + case err, ok := <-s.ErrChan: + if !ok { + return nil + } + // Treat listener closure as graceful shutdown. + if errors.Is(err, net.ErrClosed) || errors.Is(err, os.ErrClosed) { + return nil + } + return err + case <-ctx.Done(): + return ctx.Err() + } +}To ensure this change won’t break callers, please verify usages:
#!/bin/bash # Find uses of WaitForError and ErrChan across the repo rg -nP --glob '!**/vendor/**' '\bWaitForError\s*\(' -C2 rg -nP --glob '!**/vendor/**' '\.ErrChan\b' -C2vendor/github.com/DeedleFake/p9/dir_darwin.go (1)
35-43: Critical: ATime conversion uses non-existent method.Line 37 calls
sys.Atimespec.Unix(), which doesn't exist on Darwin'ssyscall.Timespec. The struct hasSecandNsecfields instead. This will cause a compilation error.Apply this diff:
- ATime: time.Unix(sys.Atimespec.Unix()), + ATime: time.Unix(sys.Atimespec.Sec, sys.Atimespec.Nsec),vendor/github.com/DeedleFake/p9/p9.go (1)
25-39: Duplicate: Open-mode flag values still don't match 9P2000 spec.The past review comment on lines 25-39 correctly identified that the flag bit patterns are offset by one position. This is a critical protocol compatibility issue that remains unresolved. Since this is vendored code, consider:
- Opening an issue with the upstream library maintainer.
- Patching the vendored code locally with the correct values.
- Verifying whether the servers and clients in use are tolerant of these non-standard values.
vendor/github.com/DeedleFake/p9/client.go (4)
45-47: Duplicate: FID generation vulnerable to overflow.The past review comment correctly identified that the atomic
uint32counter will overflow after 2^32 operations, potentially causing FID conflicts in long-running clients. Since this is vendored code:
- Consider reporting to upstream maintainer.
- Evaluate whether the expected usage pattern (client lifespan and FID allocation rate) makes overflow likely in practice.
- If overflow is a realistic concern, patch locally to handle wraparound or use a larger counter type.
49-69: Duplicate: Unsafe type assertion in Handshake can panic.The past review comments correctly identified that the type assertion
rsp.(*Rversion)at line 61 lacks safety checks and will panic if the server returns an error response or unexpected type. This is a critical reliability issue. Since this is vendored code, consider patching locally or reporting upstream.
71-91: Duplicate: Unsafe type assertion in Auth can panic.The past review comment correctly identified that the type assertion
rsp.(*Rauth)at line 84 will panic if the response is not of the expected type. This is a critical issue that should be addressed either by patching the vendored code locally or reporting to the upstream maintainer.
93-120: Duplicate: Unsafe type assertion in Attach can panic.The past review comment correctly identified that the type assertion
rsp.(*Rattach)at line 113 will panic if the server returns an unexpected response. This is a critical reliability issue that affects the robustness of the client. Consider patching the vendored code to use safe type assertions with error handling.vendor/github.com/DeedleFake/p9/proto/client.go (2)
90-97: Break on non-EOF read errors to avoid tight loopContinuing on non-EOF/non-cancel errors can loop indefinitely on a broken conn. Return (or backoff) instead.
- if (ctx.Err() != nil) || (err == io.EOF) { - return - } - - continue + if (ctx.Err() != nil) || (err == io.EOF) { + return + } + log.Printf("client reader: receive error: %v", err) + return
138-145: Never allocate reserved NoTag (0xFFFF)Allocator can yield NoTag on wrap-around. Skip it both when returning and when advancing.
- case c.nextTag <- nextTag: - for { - nextTag++ - if _, ok := tags[nextTag]; !ok { - break - } - } + case c.nextTag <- func() uint16 { + if nextTag == NoTag { + nextTag++ + } + t := nextTag + for { + nextTag++ + if nextTag == NoTag { + nextTag++ + } + if _, ok := tags[nextTag]; !ok { + break + } + } + return t + }():vendor/github.com/DeedleFake/p9/proto/encoding.go (2)
150-153: Unsafe uint16 read into uint32 is endianness-unsafeAvoid unsafe.Pointer; read into a uint16 and widen.
- default: - d.read((*uint16)(unsafe.Pointer(&length))) + default: + var len16 uint16 + d.read(&len16) + length = uint32(len16)
168-171: Avoid unsafe []byte→string conversionUse a safe conversion; preserves Go’s string immutability.
- v.SetString(*(*string)(unsafe.Pointer(&buf))) + v.SetString(string(buf))vendor/github.com/DeedleFake/p9/proto/proto.go (2)
85-94: Frame bound should exclude the 4‑byte size header; add short-size guardSet limit to size-4 and reject frames smaller than header to prevent over/under‑reads.
- if (msize > 0) && (size > msize) { + if (msize > 0) && (size > msize) { return nil, NoTag, util.Errorf("receive: %w", ErrLargeMessage) } - - lr := &util.LimitedReader{ + if size < 4 { + return nil, NoTag, util.Errorf("receive: short message size: %d", size) + } + lr := &util.LimitedReader{ R: r, - N: size, + N: size - 4, E: ErrLargeMessage, }
101-108: Remove unreachable error check in unknown-type pathThe inner err check is dead code here.
- t := p.TypeFromID(msgType) - if t == nil { - if err != nil { - return nil, NoTag, err - } - - return nil, NoTag, util.Errorf("receive: invalid message type: %v", msgType) - } + t := p.TypeFromID(msgType) + if t == nil { + return nil, NoTag, util.Errorf("receive: invalid message type: %v", msgType) + }vendor/github.com/DeedleFake/p9/fs.go (6)
499-512: Directory read offset handling deviates from 9P; validate offsets or document.Spec requires offset 0 or previous offset+len. Current code ignores offset. Track and validate per-FID dir read offsets.
I can draft a minimal per-file
lastOff/lastLenin fsFile and enforce the rule if desired.
179-180: Remove unsafe string→[]byte; use safe conversion.
unsafe.Pointerhere violates memory safety for strings.- sum := sha256.Sum256(*(*[]byte)(unsafe.Pointer(&p))) + sum := sha256.Sum256([]byte(p))Also drop the unused
unsafeimport.
244-249: Tflush: don’t return Rerror; minimally return Rflush (document limitation).Returning an error breaks clients that send flush. If true cancellation isn’t implemented, reply with
&Rflush{}and document semantics.func (h *fsHandler) flush(msg *Tflush) interface{} { - // TODO: Implement this. - return &Rerror{ - Ename: "flush is not supported", - } + // Note: does not guarantee cancellation of in-flight ops. + return &Rflush{} }
536-537: Use exclusive lock for writes; RLock risks races.Writes mutate file state;
RLockallows concurrent writers.- file.RLock() // Somewhat ironic that this doesn't require a - defer file.RUnlock() // full lock like read() does. + file.Lock() + defer file.Unlock()
593-598: Fix error check after clunk in Remove.
rspis a protocol message, noterror. Check*Rerror.- if _, ok := rsp.(error); ok { + if _, ok := rsp.(*Rerror); ok { return rsp }
719-729: Close(): lock per file during close to avoid races with in‑flight ops.Synchronize close with other operations.
func (h *fsHandler) Close() error { h.fids.Range(func(k, v interface{}) bool { file := v.(*fsFile) - if file.file != nil { - file.file.Close() - } + file.Lock() + if file.file != nil { + _ = file.file.Close() + file.file = nil + } + file.Unlock() return true }) return nil }vendor/github.com/DeedleFake/p9/stat.go (1)
135-153: Avoid unsafe conversion in FileMode.String().Use
string(buf); removeunsafeimport if now unused.func (m FileMode) String() string { buf := []byte("----------") ... - return *(*string)(unsafe.Pointer(&buf)) + return string(buf) }vendor/github.com/DeedleFake/p9/dir.go (3)
18-20: Fix path join to prevent share escape on Windows and absolute paths.Leading separators make
pabsolute, causingJoin(base, p)to ignore the base. Strip leading separators before join.Apply:
func (d Dir) path(p string) string { - return filepath.Join(string(d), filepath.FromSlash(p)) + ps := filepath.FromSlash(p) + // Force relative join by trimming leading separators + for len(ps) > 0 && (ps[0] == os.PathSeparator) { + ps = ps[1:] + } + return filepath.Join(string(d), ps) }
105-122: Create(dir): don’t open directories with file flags; open after Mkdir using os.Open and return.Opening a directory via
os.OpenFile(..., O_CREATE, ...)is invalid and will fail on most platforms. Return a handle afterMkdir.func (d Dir) Create(p string, perm FileMode, mode uint8) (File, error) { p = d.path(p) if perm&ModeDir != 0 { err := os.Mkdir(p, os.FileMode(perm.Perm())) if err != nil { return nil, err } + f, err := os.Open(p) + return &dirFile{File: f}, err } flag := toOSFlags(mode) file, err := os.OpenFile(p, flag|os.O_CREATE, os.FileMode(perm.Perm())) return &dirFile{ File: file, }, err }
239-260: Make OREAD/OWRITE/ORDWR/OEXEC mutually exclusive; treat them as mode values, not bitflags.Current bitwise checks can yield wrong flags (e.g., OEXEC matching OWRITE). Use low 2-bit mask and a switch.
-func toOSFlags(mode uint8) (flag int) { - if mode&OREAD != 0 { - flag |= os.O_RDONLY - } - if mode&OWRITE != 0 { - flag |= os.O_WRONLY - } - if mode&ORDWR != 0 { - flag |= os.O_RDWR - } +func toOSFlags(mode uint8) (flag int) { + switch mode & 0x3 { // OREAD=0, OWRITE=1, ORDWR=2, OEXEC=3 + case OWRITE: + flag |= os.O_WRONLY + case ORDWR: + flag |= os.O_RDWR + default: + // OREAD or OEXEC -> read-only (O_RDONLY is 0) + } if mode&OTRUNC != 0 { flag |= os.O_TRUNC } //if mode&OEXCL != 0 { // flag |= os.O_EXCL //} //if mode&OAPPEND != 0 { // flag |= os.O_APPEND //} return flag }vendor/github.com/DeedleFake/p9/msg.go (1)
207-217: Error handling issue in vendored code.The past review comment correctly identified that
P9Encodemethods don't check errors after eachproto.Writecall, which could lead to returning corrupted data if an intermediate write fails. The same issue exists inTwstat.P9Encode(lines 240-255).Since this is vendored third-party code from
github.com/DeedleFake/p9, consider:
- Checking if a newer version of the library addresses this issue
- Reporting the issue upstream to the library maintainer
- Documenting this known limitation if the risk is acceptable
Check if a newer version of the p9 library exists:
#!/bin/bash # Check the vendored version and compare with upstream echo "=== Vendored version information ===" if [ -f "go.mod" ]; then grep "github.com/DeedleFake/p9" go.mod fi echo "" echo "=== Latest releases from GitHub ===" gh api repos/DeedleFake/p9/releases --jq '.[0:3] | .[] | {tag_name, published_at, name}'vendor/github.com/DeedleFake/p9/remote.go (1)
37-43: Path splitting issue in vendored code.The past review comment correctly identified that
strings.Spliton an absolute path like"/a/b"produces["", "a", "b"], where the leading empty string is invalid forTwalk. The current logic doesn't filter out this empty component.Since this is vendored third-party code from
github.com/DeedleFake/p9:
- Verify if a newer version addresses this issue
- Consider reporting upstream if not fixed
- Test that the 9P use cases in this PR don't trigger this path with leading slashes
Search the codebase for calls to
Open,Create,Remove, andStatmethods that might pass absolute paths:#!/bin/bash # Search for Remote method calls that might pass absolute paths echo "=== Searching for Remote.Open/Create/Remove/Stat calls ===" rg -n -C2 -tgo '\.(Open|Create|Remove|Stat)\s*\(\s*"/' --glob '!vendor/**' echo "" echo "=== Check if paths are always relative in fs9p usage ===" rg -n -C3 -tgo 'Remote\.(Open|Create|Remove|Stat)' pkg/fileserver/fs9p/
🧹 Nitpick comments (9)
pkg/fileserver/fs9p/server.go (3)
3-13: Add missing imports for timeout/context/error handling.
Required by the startup timeout and ctx-aware wait refactor below.import ( + "context" + "errors" "fmt" "net" "os" "path/filepath" + "time" "github.com/DeedleFake/p9" "github.com/DeedleFake/p9/proto" "github.com/crc-org/crc/v2/pkg/crc/constants" "github.com/sirupsen/logrus" )
45-51: Minor: Initialize Server with a struct literal.
Clearer and less error‑prone.-toReturn := new(Server) -toReturn.Listener = listener -toReturn.Filesystem = fs -toReturn.ExposedDir = exposeDir -toReturn.ErrChan = errChan - -return toReturn, nil +return &Server{ + Listener: listener, + Filesystem: fs, + ExposedDir: exposeDir, + ErrChan: errChan, +}, nil
15-24: Consider making fields unexported to tighten API surface.
Expose methods, not internals (e.g., errChan), to prevent external sends/close.type Server struct { // Listener this server is bound to Listener net.Listener // Plan9 Filesystem type that holds the exposed directory Filesystem p9.FileSystem // Directory this server exposes ExposedDir string // Errors from the server being started will come out here - ErrChan chan error + ErrChan chan error // consider: make this unexported (errChan) and use WaitForError/WaitForErrorCtx }vendor/github.com/DeedleFake/p9/README.md (1)
12-49: Fix markdown formatting issues.Static analysis flags hard tabs and heading style inconsistencies. Replace hard tabs with spaces and use consistent heading styles (e.g., setext throughout or atx throughout).
vendor/github.com/DeedleFake/p9/proto/client.go (1)
69-74: Close should wait for goroutines to exitEnsure reader/coord exit to avoid races/leaks; wait briefly for c.done.
func (c *Client) Close() error { c.cancel() - return c.c.Close() + _ = c.c.Close() + select { + case <-c.done: + case <-time.After(1 * time.Second): + } + return nil }vendor/github.com/DeedleFake/p9/proto/encoding.go (1)
85-96: Speed up []byte encoding pathFor slices of bytes, write the payload in one shot instead of byte-by-byte.
- case reflect.Array, reflect.Slice: - switch v.Type().Elem().Kind() { - case reflect.Uint8: - e.mode(uint32(v.Len())) - default: - e.mode(uint16(v.Len())) - } - - for i := 0; i < v.Len(); i++ { - e.encode(v.Index(i)) - } + case reflect.Array, reflect.Slice: + switch v.Type().Elem().Kind() { + case reflect.Uint8: + e.mode(uint32(v.Len())) + // Fast path for []byte + e.mode(v.Bytes()) + default: + e.mode(uint16(v.Len())) + for i := 0; i < v.Len(); i++ { + e.encode(v.Index(i)) + } + }vendor/github.com/DeedleFake/p9/dir.go (1)
190-196: Consider allowing OEXEC on read‑only FS.Denying
OEXECblocks execute-only opens that don’t mutate state. If safe for your use case, removeOEXECfrom the rejection mask.- if mode&(OWRITE|ORDWR|OEXEC|OTRUNC|OCEXEC|ORCLOSE) != 0 { + if mode&(OWRITE|ORDWR|OTRUNC|OCEXEC|ORCLOSE) != 0 {vendor/github.com/DeedleFake/p9/fs.go (1)
205-220: Be more lenient in version negotiation.Many clients send “9P2000.u/L”. Consider replying
RversionwithVersion(9P2000) and adjusted msize instead of hard‑erroring.- if msg.Version != Version { - return &Rerror{ Ename: ErrUnsupportedVersion.Error() } - } + // Accept unknown variants by responding with our Version. + // If strict negotiation is desired, gate behind a flag.pkg/crc/machine/start.go (1)
267-269: Make the 9pfs invocation explicit (use -p or use mount -t 9p).Current code calls sshRunner.Run("9pfs 192.168.127.1", mount.Target) which yields
9pfs 192.168.127.1 <mountpoint>— that can work (9pfs accepts server then mountpoint and will default to TCP port 564), but typical/robust usage is to specify the port/options explicitly (e.g.9pfs 192.168.127.1 -p 564 <mountpoint>) or usemount -t 9p -o trans=tcp,port=564 .... Confirm whether you want to rely on the default port here or change the invocation to include-p constants.Plan9TcpPort(or switch to a mount(9p) form) for clarity and future-proofing. (pmikkelsen.com)Location: pkg/crc/machine/start.go:267-269.
📜 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 (33)
cmd/crc/cmd/daemon.go(5 hunks)go.mod(1 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)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)
🚧 Files skipped from review as they are similar to previous changes (12)
- vendor/github.com/DeedleFake/p9/LICENSE
- go.mod
- vendor/github.com/DeedleFake/p9/addr.go
- vendor/github.com/DeedleFake/p9/dir_linux.go
- pkg/crc/constants/constants.go
- vendor/github.com/DeedleFake/p9/addr_unix.go
- vendor/github.com/DeedleFake/p9/proto/server.go
- vendor/modules.txt
- vendor/github.com/DeedleFake/p9/addr_other.go
- vendor/github.com/DeedleFake/p9/internal/debug/nodebug.go
- cmd/crc/cmd/daemon.go
- pkg/crc/machine/libhvee/driver_windows.go
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: redbeam
PR: crc-org/crc#4866
File: pkg/fileserver/fs9p/shares.go:0-0
Timestamp: 2025-08-06T09:48:10.441Z
Learning: The vsock implementation in pkg/fileserver/fs9p/shares.go was temporarily removed because 9pfs doesn't fully support it yet. It will be added back when 9pfs provides proper vsock support.
🧬 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/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/dir_other.go (1)
vendor/github.com/DeedleFake/p9/stat.go (3)
DirEntry(255-267)FileMode(23-23)ModeFromOS(43-78)
pkg/fileserver/fs9p/server.go (3)
vendor/github.com/DeedleFake/p9/fs.go (2)
FileSystem(21-34)FSConnHandler(162-167)vendor/github.com/DeedleFake/p9/proto/server.go (1)
Serve(17-42)pkg/crc/constants/constants.go (1)
Plan9Msize(59-59)
vendor/github.com/DeedleFake/p9/proto/encoding.go (1)
vendor/github.com/DeedleFake/p9/internal/util/util.go (1)
Errorf(44-52)
pkg/crc/machine/start.go (1)
pkg/os/exec.go (1)
RunPrivileged(48-59)
vendor/github.com/DeedleFake/p9/internal/util/util.go (1)
vendor/github.com/DeedleFake/p9/proto/encoding.go (1)
Read(34-38)
vendor/github.com/DeedleFake/p9/fs.go (7)
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/p9.go (7)
QID(42-46)QIDType(49-49)IOHeaderSize(70-70)Version(10-10)QTAuth(58-58)NoFID(18-18)QTDir(54-54)vendor/github.com/DeedleFake/p9/proto/server.go (3)
MessageHandler(135-139)ConnHandler(112-114)ConnHandlerFunc(125-125)vendor/github.com/DeedleFake/p9/internal/debug/debug.go (1)
Log(10-12)vendor/github.com/DeedleFake/p9/internal/debug/nodebug.go (1)
Log(5-5)vendor/github.com/DeedleFake/p9/msg.go (27)
Tversion(79-82)Tversion(84-84)Rerror(118-120)Rversion(86-89)Tauth(95-99)Rauth(101-103)Tflush(126-128)Tattach(105-110)Rattach(112-114)Twalk(133-137)Rwalk(139-141)Topen(143-146)Ropen(148-151)Tcreate(153-158)Rcreate(160-163)Tread(165-169)Rread(171-173)Twrite(175-179)Rwrite(181-183)Tclunk(185-187)Rclunk(189-190)Tremove(192-194)Rremove(196-197)Tstat(199-201)Rstat(203-205)Twstat(235-238)Rwstat(278-279)vendor/github.com/DeedleFake/p9/encoding.go (1)
WriteDir(33-42)
vendor/github.com/DeedleFake/p9/dir_plan9.go (3)
vendor/github.com/DeedleFake/p9/stat.go (4)
DirEntry(255-267)FileMode(23-23)ModeFromOS(43-78)Stat(156-168)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/proto/client.go (5)
vendor/github.com/DeedleFake/p9/msg.go (1)
Proto(75-77)vendor/github.com/DeedleFake/p9/proto/proto.go (3)
Proto(38-41)NoTag(27-27)ErrClientClosed(22-22)vendor/github.com/DeedleFake/p9/client.go (3)
NewClient(30-32)Client(22-26)Dial(36-43)vendor/github.com/DeedleFake/p9/internal/debug/debug.go (1)
Log(10-12)vendor/github.com/DeedleFake/p9/internal/debug/nodebug.go (1)
Log(5-5)
vendor/github.com/DeedleFake/p9/dir_darwin.go (3)
vendor/github.com/DeedleFake/p9/stat.go (4)
DirEntry(255-267)FileMode(23-23)ModeFromOS(43-78)Stat(156-168)vendor/github.com/DeedleFake/p9/dir.go (1)
Dir(16-16)vendor/github.com/DeedleFake/p9/p9.go (3)
QID(42-46)QIDType(49-49)Version(10-10)
vendor/github.com/DeedleFake/p9/stat.go (3)
vendor/github.com/DeedleFake/p9/p9.go (3)
QIDType(49-49)QID(42-46)Version(10-10)vendor/github.com/DeedleFake/p9/proto/encoding.go (3)
Write(25-31)Read(34-38)Size(14-20)vendor/github.com/DeedleFake/p9/internal/util/util.go (1)
LimitedReader(13-17)
vendor/github.com/DeedleFake/p9/proto/proto.go (4)
vendor/github.com/DeedleFake/p9/msg.go (1)
Proto(75-77)vendor/github.com/DeedleFake/p9/proto/encoding.go (3)
Read(34-38)Write(25-31)Size(14-20)vendor/github.com/DeedleFake/p9/p9.go (1)
NoTag(15-15)vendor/github.com/DeedleFake/p9/internal/util/util.go (2)
Errorf(44-52)LimitedReader(13-17)
vendor/github.com/DeedleFake/p9/dir.go (3)
vendor/github.com/DeedleFake/p9/stat.go (5)
Stat(156-168)DirEntry(255-267)StatChanges(316-318)FileMode(23-23)ModeDir(27-27)vendor/github.com/DeedleFake/p9/fs.go (4)
File(100-119)Attachment(44-74)FileSystem(21-34)QIDFS(89-91)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)
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/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/p9.go (1)
vendor/github.com/DeedleFake/p9/stat.go (1)
FileMode(23-23)
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/remote.go (7)
vendor/github.com/DeedleFake/p9/client.go (1)
Client(22-26)vendor/github.com/DeedleFake/p9/proto/client.go (1)
Client(18-31)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)
🪛 markdownlint-cli2 (0.18.1)
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). (14)
- GitHub Check: build (ubuntu-22.04, 1.24)
- GitHub Check: build (ubuntu-latest, 1.24)
- GitHub Check: verify-devcontainer
- GitHub Check: build (macOS-14, 1.24)
- GitHub Check: build (macOS-13, 1.24)
- GitHub Check: build (ubuntu-latest, 1.24)
- GitHub Check: build-qe (linux, arm64)
- GitHub Check: build-qe (darwin, arm64)
- GitHub Check: Run OKD bundle with crc (1.24)
- GitHub Check: build-qe (darwin, amd64)
- GitHub Check: build-qe (linux, amd64)
- GitHub Check: build-qe (windows, amd64)
- GitHub Check: build-installer (windows-2022, 1.24)
- GitHub Check: Konflux kflux-prd-rh02 / crc-binary-on-pull-request
🔇 Additional comments (16)
vendor/github.com/DeedleFake/p9/internal/debug/debug.go (1)
1-12: Vendored debug implementation looks correct.This is vendored code from the DeedleFake/p9 library. The conditional compilation approach (p9debug build tag enables logging, !p9debug provides no-op) is standard practice for optional debug instrumentation. The implementation is straightforward and appropriate for debug logging.
pkg/fileserver/fs9p/server.go (1)
31-40: Path validation looks solid.
Absolute path + existence + directory check is correct and cross‑platform.vendor/github.com/DeedleFake/p9/dir_other.go (1)
1-14: LGTM: Fallback implementation is correct.This provides a reasonable fallback for OSes without platform-specific syscall data. The minimal DirEntry population is appropriate.
vendor/github.com/DeedleFake/p9/dir_windows.go (1)
1-27: LGTM: Windows-specific implementation is correct.The use of
LastAccessTime.Nanoseconds()is valid for Windows syscall.Filetime. Fallback handling is appropriate.vendor/github.com/DeedleFake/p9/doc.go (1)
1-63: LGTM: Package documentation is comprehensive and clear.The documentation provides a solid overview of the 9P protocol handling, client/server abstractions, and usage patterns.
vendor/github.com/DeedleFake/p9/dir_darwin.go (1)
46-62: LGTM: GetQID implementation is correct.The QID retrieval using
os.Statand inode extraction fromsyscall.Stat_tis correct for Darwin. Error handling is appropriate.vendor/github.com/DeedleFake/p9/encoding.go (2)
16-30: LGTM – ReadDir implementation is sound.The function correctly handles EOF as normal termination and accumulates directory entries. The warning about buffering is a helpful guard against protocol issues.
33-42: LGTM – WriteDir implementation is correct.The function correctly serializes directory entries and fails fast on the first write error, which is appropriate for this protocol layer.
vendor/github.com/DeedleFake/p9/internal/util/util.go (2)
27-39: LimitedReader.Read logic is correct.The implementation correctly enforces the byte limit and returns the custom error when exhausted. The check
lr.N <= 0at line 28 is technically redundant (sinceNisuint32and cannot be negative), but it doesn't affect correctness and may be defensive coding for clarity.
44-52: Errorf correctly preserves sentinel errors.The function correctly returns
io.EOFdirectly when found in the arguments, avoiding wrapping it. The type assertion at line 47 is safe sinceio.EOFis of typeerror.vendor/github.com/DeedleFake/p9/dir_plan9.go (2)
10-31: LGTM – infoToEntry safely extracts Plan 9 metadata.The function correctly uses a two-value type assertion to safely extract Plan 9-specific metadata when available, falling back to basic metadata otherwise.
33-49: LGTM – GetQID safely extracts QID on Plan 9.The method correctly uses a safe type assertion and returns a clear error if the platform-specific metadata is unavailable. The QID construction from
syscall.Diris appropriate for Plan 9.vendor/github.com/DeedleFake/p9/p9.go (2)
7-19: LGTM – Core 9P constants are correct.The version string and sentinel values are properly defined and match the 9P2000 protocol specification.
41-71: LGTM – QID and related constants are correct.The QID structure, QIDType constants, and FileMode conversion are correctly implemented according to the 9P2000 specification. The bit patterns for QIDType properly use descending bit positions.
vendor/github.com/DeedleFake/p9/client.go (1)
11-43: LGTM – Client structure and constructors are well-designed.The Client type appropriately embeds the proto.Client and maintains per-client FID state. The constructor and Dial function are correctly implemented.
vendor/github.com/DeedleFake/p9/proto/client.go (1)
50-51: Default msize seems low; confirm Tversion negotiation adjusts it1024B limits throughput. Verify the caller negotiates a larger msize (e.g., 8–64 KiB) via Tversion before regular traffic.
| case "9p": | ||
| // change owner to core user to allow mounting to it as a non-root user | ||
| if _, _, err := sshRunner.RunPrivileged("Changing owner of mount directory", "chown core:core", mount.Target); err != nil { | ||
| return err | ||
| } | ||
| if _, _, err := sshRunner.Run("9pfs 192.168.127.1", mount.Target); err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
Fix the chown command syntax.
Line 264 has incorrect argument passing. The command "chown core:core" and mount.Target are passed as separate arguments, but RunPrivileged expects the command name followed by its arguments as individual parameters.
Apply this diff to fix the syntax:
case "9p":
// change owner to core user to allow mounting to it as a non-root user
- if _, _, err := sshRunner.RunPrivileged("Changing owner of mount directory", "chown core:core", mount.Target); err != nil {
+ if _, _, err := sshRunner.RunPrivileged("Changing owner of mount directory", "chown", "core:core", mount.Target); err != nil {
return err
}
if _, _, err := sshRunner.Run("9pfs 192.168.127.1", mount.Target); err != nil {
return err
}📝 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.
| case "9p": | |
| // change owner to core user to allow mounting to it as a non-root user | |
| if _, _, err := sshRunner.RunPrivileged("Changing owner of mount directory", "chown core:core", mount.Target); err != nil { | |
| return err | |
| } | |
| if _, _, err := sshRunner.Run("9pfs 192.168.127.1", mount.Target); err != nil { | |
| return err | |
| } | |
| case "9p": | |
| // change owner to core user to allow mounting to it as a non-root user | |
| if _, _, err := sshRunner.RunPrivileged("Changing owner of mount directory", "chown", "core:core", mount.Target); err != nil { | |
| return err | |
| } | |
| if _, _, err := sshRunner.Run("9pfs 192.168.127.1", mount.Target); err != nil { | |
| return err | |
| } |
🤖 Prompt for AI Agents
In pkg/crc/machine/start.go around lines 262 to 269, the chown invocation passes
the entire command string and target as separate parameters causing incorrect
argument splitting; update the RunPrivileged call to pass the command and its
arguments as separate parameters (e.g., use "chown", "core:core", mount.Target)
so RunPrivileged receives the command name followed by its args.
| atime, ok1 := changes.ATime() | ||
| mtime, ok2 := changes.MTime() | ||
| if ok1 || ok2 { | ||
| err := os.Chtimes(p, atime, mtime) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Avoid clobbering unspecified times in WriteStat; handle partial ATime/MTime updates safely.
Calling os.Chtimes(p, atime, mtime) when only one is set will overwrite the other with zero time or error. Preserve the unspecified value.
- atime, ok1 := changes.ATime()
- mtime, ok2 := changes.MTime()
- if ok1 || ok2 {
- err := os.Chtimes(p, atime, mtime)
+ atime, ok1 := changes.ATime()
+ mtime, ok2 := changes.MTime()
+ if ok1 || ok2 {
+ // Preserve unspecified field by probing current times (best-effort).
+ fi, err := os.Stat(p)
+ if err != nil {
+ return err
+ }
+ if !ok2 {
+ mtime = fi.ModTime()
+ }
+ if !ok1 {
+ // Best-effort: fall back to current mtime if atime not available.
+ // Consider adding OS-specific atime retrieval for accuracy.
+ atime = fi.ModTime()
+ }
+ err = os.Chtimes(p, atime, mtime)
if err != nil {
return err
}
}📝 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.
| atime, ok1 := changes.ATime() | |
| mtime, ok2 := changes.MTime() | |
| if ok1 || ok2 { | |
| err := os.Chtimes(p, atime, mtime) | |
| if err != nil { | |
| return err | |
| } | |
| } | |
| atime, ok1 := changes.ATime() | |
| mtime, ok2 := changes.MTime() | |
| if ok1 || ok2 { | |
| // Preserve unspecified field by probing current times (best-effort). | |
| fi, err := os.Stat(p) | |
| if err != nil { | |
| return err | |
| } | |
| if !ok2 { | |
| mtime = fi.ModTime() | |
| } | |
| if !ok1 { | |
| // Best-effort: fall back to current mtime if atime not available. | |
| // Consider adding OS-specific atime retrieval for accuracy. | |
| atime = fi.ModTime() | |
| } | |
| err = os.Chtimes(p, atime, mtime) | |
| if err != nil { | |
| return err | |
| } | |
| } |
🤖 Prompt for AI Agents
In vendor/github.com/DeedleFake/p9/dir.go around lines 52 to 60, the current
logic calls os.Chtimes(p, atime, mtime) even when only one of ATime/MTime is
provided which can clobber the unspecified time; change it to first stat the
file (os.Stat) to read the existing atime/mtime, then if ok1 is false reuse the
existing atime, and if ok2 is false reuse the existing mtime, and finally call
os.Chtimes with the preserved values (keeping the existing modification/access
times when not supplied) and return any error from that call.
| length, ok := changes.Length() | ||
| if ok { | ||
| err := os.Truncate(p, int64(length)) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| } |
There was a problem hiding this comment.
Guard against uint64→int64 overflow in Truncate.
Large length can wrap negative when cast, corrupting data or erroring.
- length, ok := changes.Length()
+ length, ok := changes.Length()
if ok {
- err := os.Truncate(p, int64(length))
+ if length > math.MaxInt64 {
+ return fmt.Errorf("truncate length too large: %d", length)
+ }
+ err := os.Truncate(p, int64(length))
if err != nil {
return err
}
}Add imports:
+ "fmt"
+ "math"📝 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.
| length, ok := changes.Length() | |
| if ok { | |
| err := os.Truncate(p, int64(length)) | |
| if err != nil { | |
| return err | |
| } | |
| } | |
| // At the top of the file, in the import block, add: | |
| import ( | |
| // ... other imports ... | |
| "fmt" | |
| "math" | |
| ) | |
| // Then, in the function around lines 61–67, replace the existing snippet with: | |
| length, ok := changes.Length() | |
| if ok { | |
| if length > math.MaxInt64 { | |
| return fmt.Errorf("truncate length too large: %d", length) | |
| } | |
| err := os.Truncate(p, int64(length)) | |
| if err != nil { | |
| return err | |
| } | |
| } |
🤖 Prompt for AI Agents
In vendor/github.com/DeedleFake/p9/dir.go around lines 61–67, guard against
uint64→int64 overflow before calling os.Truncate: if changes.Length() returns a
length greater than int64 max, return a descriptive error instead of casting and
calling Truncate; otherwise safely cast to int64 and call os.Truncate. Add the
necessary imports (math and fmt or errors) to construct the error.
| name, ok := changes.Name() | ||
| if ok { | ||
| err := os.Rename(p, filepath.Join(base, filepath.FromSlash(name))) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| } |
There was a problem hiding this comment.
Sanitize rename target to a base name to prevent path traversal.
filepath.Join(base, FromSlash(name)) allows ../.. to escape the directory. Restrict to basename.
- name, ok := changes.Name()
+ name, ok := changes.Name()
if ok {
- err := os.Rename(p, filepath.Join(base, filepath.FromSlash(name)))
+ newName := filepath.Base(filepath.FromSlash(name))
+ if newName == "." || newName == "" {
+ return errors.New("invalid new name")
+ }
+ err := os.Rename(p, filepath.Join(base, newName))
if err != nil {
return err
}
}📝 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.
| name, ok := changes.Name() | |
| if ok { | |
| err := os.Rename(p, filepath.Join(base, filepath.FromSlash(name))) | |
| if err != nil { | |
| return err | |
| } | |
| } | |
| name, ok := changes.Name() | |
| if ok { | |
| newName := filepath.Base(filepath.FromSlash(name)) | |
| if newName == "." || newName == "" { | |
| return errors.New("invalid new name") | |
| } | |
| err := os.Rename(p, filepath.Join(base, newName)) | |
| if err != nil { | |
| return err | |
| } | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (11)
vendor/github.com/DeedleFake/p9/dir_darwin.go (1)
37-37: ATime conversion uses non-existent Timespec.Unix(); fix to Sec/Nsec fields.
syscall.Timespecon Darwin hasSecandNsecfields; there's noUnix()method. This will cause a compilation error.Apply this diff to fix the issue:
- ATime: time.Unix(sys.Atimespec.Unix()), + ATime: time.Unix(sys.Atimespec.Sec, sys.Atimespec.Nsec),vendor/github.com/DeedleFake/p9/dir_linux.go (1)
37-37: ATime conversion uses non-existent Timespec.Unix(); fix to Sec/Nsec fields.
syscall.Timespecon Linux hasSecandNsecfields; there's noUnix()method. This will cause a compilation error.Apply this diff to fix the issue:
- ATime: time.Unix(sys.Atim.Unix()), + ATime: time.Unix(sys.Atim.Sec, sys.Atim.Nsec),vendor/github.com/DeedleFake/p9/msg.go (2)
207-217: Acknowledge prior error handling concern in vendored code.A previous review identified that this method returns buffer bytes even when the final
proto.Writefails (line 215), which could produce corrupted output. Since this is vendored code fromgithub.com/DeedleFake/p9 v0.6.12, modifications should be submitted upstream rather than made locally.Verify whether this concern has been reported to the upstream repository. If not, consider opening an issue at https://github.com/DeedleFake/p9.
240-255: Same error handling concern as Rstat.P9Encode.This method exhibits the same pattern as
Rstat.P9Encode: it returns buffer bytes even when the finalproto.Writefails (line 253). As vendored code fromgithub.com/DeedleFake/p9 v0.6.12, this should be addressed upstream.cmd/crc/cmd/daemon.go (1)
259-259: Security: Entire home directory exposed via 9p.This concern was already raised in previous reviews. The current implementation exposes
constants.GetHomeDir()(the entire user home directory) via 9p without access controls or read-only enforcement. This creates security risks as noted in the existing review comments.pkg/fileserver/fs9p/server.go (1)
56-70: Race condition: startup error detection is still unreliable.The non-blocking
select(lines 63-69) checks for immediate errors butproto.Servemay not encounter errors until after it starts accepting connections. For example, if the first client triggers a protocol error, that error arrives inErrChanmilliseconds later, well afterStart()has returned success.Apply this diff to wait briefly for startup errors:
// Start a server created by New9pServer. func (s *Server) Start() error { go func() { s.ErrChan <- proto.Serve(s.Listener, p9.Proto(), p9.FSConnHandler(s.Filesystem, constants.Plan9Msize)) close(s.ErrChan) }() - // Just before returning, check to see if we got an error off server startup. + // Wait briefly for potential startup errors select { case err := <-s.ErrChan: return fmt.Errorf("starting 9p server: %w", err) - default: + case <-time.After(100 * time.Millisecond): logrus.Infof("Successfully started 9p server on %s for directory %s", s.Listener.Addr().String(), s.ExposedDir) return nil } }You'll need to import the
timepackage.vendor/github.com/DeedleFake/p9/proto/client.go (2)
90-99: Stop spinning forever on persistent read errorsThis re-raises the unresolved point: when
Receivereturns a non-EOF error, the loop just continues, so a dead TCP connection causes a tight spin. Please propagate the error (and cancel waiting senders) instead of looping forever.
138-145: Never hand out the reserved NoTagReiterating prior feedback: when
nextTagreaches0xFFFF, this allocator returns the reservedNoTag, breaking the protocol on the very next request. Ensure you skipNoTagboth when handing out a tag and while searching for the next free value.vendor/github.com/DeedleFake/p9/proto/encoding.go (1)
146-154: Big-endian decode interprets 16-bit lengths as 32-bit gigabytesLine [152] writes a 16-bit value into a
uint32viaunsafe.Pointer, which only works on little-end machines. On big-endian targets the bytes land in the high half, so a length of 5 turns into 0x00050000, allocating hundreds of KB (worst case >4 GB). Decode into a realuint16first and widen safely.- default: - d.read((*uint16)(unsafe.Pointer(&length))) + default: + var len16 uint16 + d.read(&len16) + length = uint32(len16)vendor/github.com/DeedleFake/p9/client.go (2)
74-120: Auth/Attach can panic on server errorsLines [84] and [113] also use unchecked assertions (
rsp.(*Rauth),rsp.(*Rattach)). When the server replies withRerror—which is normal for auth/attach failures—the client panics instead of returning the error. Guard these assertions and surface a clear error.- rauth := rsp.(*Rauth) + rauth, ok := rsp.(*Rauth) + if !ok { + return nil, fmt.Errorf("unexpected response type for auth: %T", rsp) + } @@ - attach := rsp.(*Rattach) + attach, ok := rsp.(*Rattach) + if !ok { + return nil, fmt.Errorf("unexpected response type for attach: %T", rsp) + }
53-68: Handshake rejects standard server replies and can panicLine [61] blindly asserts
rsp.(*Rversion), so a legitimateRerrorresponse (e.g., auth failure) would panic. Even if it succeeds, the strict equality on Line [62] rejects common replies like"9P2000.L"from the Linux kernel, so the handshake always fails against standard servers. Please guard the type assertion and acceptVersionprefixes so"9P2000"family replies work.-import ( - "errors" - "net" - "sync/atomic" - - "github.com/DeedleFake/p9/proto" -) +import ( + "errors" + "fmt" + "net" + "strings" + "sync/atomic" + + "github.com/DeedleFake/p9/proto" +) @@ - version := rsp.(*Rversion) - if version.Version != Version { - return 0, ErrUnsupportedVersion - } + version, ok := rsp.(*Rversion) + if !ok { + return 0, fmt.Errorf("unexpected response type for version: %T", rsp) + } + if !strings.HasPrefix(version.Version, Version) { + return 0, fmt.Errorf("%w: server replied %q", ErrUnsupportedVersion, version.Version) + }
🧹 Nitpick comments (4)
vendor/github.com/DeedleFake/p9/addr_unix.go (1)
28-28: Consider username sanitization for edge cases.The path construction concatenates
u.Usernamewithout sanitization (line 28). While Unix usernames are typically restricted (alphanumeric + underscore/hyphen), edge cases with special characters (spaces, backslashes) could lead to unexpected paths.If you want to add extra robustness, consider sanitizing the username:
- return filepath.Join("/", "tmp", "ns."+u.Username+"."+display) + sanitizedUsername := strings.Map(func(r rune) rune { + if (r >= 'a' && r <= 'z') || (r >= 'A' && r <= 'Z') || (r >= '0' && r <= '9') || r == '_' || r == '-' { + return r + } + return '_' + }, u.Username) + return filepath.Join("/", "tmp", "ns."+sanitizedUsername+"."+display)Note: This is an optional refinement since vendored library code typically assumes standard Unix username constraints.
pkg/fileserver/fs9p/server.go (3)
72-82: Document zombie process risk more clearly.The comment warns about "BAD THINGS" and zombie processes but doesn't provide actionable guidance for operators. Consider clarifying when it's safe to call
Stop()and what the consequences are.Apply this diff to improve the warning:
// 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. +// WARNING: Stopping the server while clients are actively mounted will cause +// client processes to hang in uninterruptible I/O sleep, requiring VM restart +// to recover. Ensure all 9p mounts are unmounted 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 }
84-88: WaitForError blocks indefinitely without timeout.
WaitForError()blocks until an error arrives onErrChan. Ifproto.Serveruns without errors (normal operation), this method never returns. Callers cannot implement timeouts or graceful shutdown.Apply this diff to add context support:
+import "context" + // WaitForError from a running server. -func (s *Server) WaitForError() error { - err := <-s.ErrChan - return err +func (s *Server) WaitForError(ctx context.Context) error { + select { + case err := <-s.ErrChan: + return err + case <-ctx.Done(): + return ctx.Err() + } }Update callers (e.g., in
daemon.go) to pass a context:go func() { if err := server9p.WaitForError(context.Background()); err != nil { errCh <- errors.Wrap(err, "9p server failed") } }()
59-59: Channel closed after first error; subsequent errors lost.Line 59 closes
ErrChanafterproto.Servereturns. IfStop()is called and then another error occurs (unlikely but possible in edge cases), the second send panics. More importantly, closing immediately after the first error meansWaitForError()will return a zero value on subsequent calls rather than blocking.Consider removing the
close(s.ErrChan)line if you wantWaitForError()to block indefinitely when no error has occurred, or document thatWaitForError()should only be called once. Alternatively, keep the close but document that the channel is single-use.
📜 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 (33)
cmd/crc/cmd/daemon.go(5 hunks)go.mod(1 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)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)
🚧 Files skipped from review as they are similar to previous changes (21)
- vendor/github.com/DeedleFake/p9/internal/debug/debug.go
- pkg/crc/machine/start.go
- go.mod
- vendor/github.com/DeedleFake/p9/addr.go
- vendor/github.com/DeedleFake/p9/LICENSE
- vendor/github.com/DeedleFake/p9/internal/util/util.go
- pkg/crc/constants/constants.go
- vendor/github.com/DeedleFake/p9/proto/server.go
- vendor/modules.txt
- pkg/crc/machine/libhvee/driver_windows.go
- vendor/github.com/DeedleFake/p9/dir_plan9.go
- vendor/github.com/DeedleFake/p9/doc.go
- vendor/github.com/DeedleFake/p9/internal/debug/nodebug.go
- vendor/github.com/DeedleFake/p9/stat.go
- vendor/github.com/DeedleFake/p9/dir_windows.go
- vendor/github.com/DeedleFake/p9/p9.go
- vendor/github.com/DeedleFake/p9/dir_other.go
- vendor/github.com/DeedleFake/p9/proto/proto.go
- vendor/github.com/DeedleFake/p9/dir.go
- vendor/github.com/DeedleFake/p9/fs.go
- vendor/github.com/DeedleFake/p9/addr_other.go
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: redbeam
PR: crc-org/crc#4866
File: pkg/fileserver/fs9p/shares.go:0-0
Timestamp: 2025-08-06T09:48:10.441Z
Learning: The vsock implementation in pkg/fileserver/fs9p/shares.go was temporarily removed because 9pfs doesn't fully support it yet. It will be added back when 9pfs provides proper vsock support.
🧬 Code graph analysis (11)
vendor/github.com/DeedleFake/p9/dir_linux.go (3)
vendor/github.com/DeedleFake/p9/stat.go (4)
DirEntry(255-267)FileMode(23-23)ModeFromOS(43-78)Stat(156-168)vendor/github.com/DeedleFake/p9/dir.go (1)
Dir(16-16)vendor/github.com/DeedleFake/p9/p9.go (3)
QID(42-46)QIDType(49-49)Version(10-10)
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/proto/encoding.go (1)
vendor/github.com/DeedleFake/p9/internal/util/util.go (1)
Errorf(44-52)
vendor/github.com/DeedleFake/p9/proto/client.go (4)
vendor/github.com/DeedleFake/p9/proto/proto.go (3)
Proto(38-41)NoTag(27-27)ErrClientClosed(22-22)vendor/github.com/DeedleFake/p9/client.go (3)
NewClient(30-32)Client(22-26)Dial(36-43)vendor/github.com/DeedleFake/p9/internal/debug/debug.go (1)
Log(10-12)vendor/github.com/DeedleFake/p9/internal/debug/nodebug.go (1)
Log(5-5)
vendor/github.com/DeedleFake/p9/dir_darwin.go (3)
vendor/github.com/DeedleFake/p9/stat.go (4)
DirEntry(255-267)FileMode(23-23)ModeFromOS(43-78)Stat(156-168)vendor/github.com/DeedleFake/p9/dir.go (1)
Dir(16-16)vendor/github.com/DeedleFake/p9/p9.go (3)
QID(42-46)QIDType(49-49)Version(10-10)
vendor/github.com/DeedleFake/p9/client.go (5)
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/proto/proto.go (1)
Proto(38-41)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)
cmd/crc/cmd/daemon.go (2)
pkg/crc/constants/constants.go (2)
Plan9TcpPort(60-60)GetHomeDir(164-170)pkg/fileserver/fs9p/server.go (1)
New9pServer(29-53)
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/msg.go (5)
vendor/github.com/DeedleFake/p9/proto/proto.go (2)
NewProto(44-60)Proto(38-41)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)
pkg/fileserver/fs9p/server.go (3)
vendor/github.com/DeedleFake/p9/fs.go (2)
FileSystem(21-34)FSConnHandler(162-167)vendor/github.com/DeedleFake/p9/proto/server.go (1)
Serve(17-42)pkg/crc/constants/constants.go (1)
Plan9Msize(59-59)
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)
🪛 markdownlint-cli2 (0.18.1)
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). (4)
- GitHub Check: verify-devcontainer
- GitHub Check: build-qe (linux, arm64)
- GitHub Check: Run OKD bundle with crc (1.24)
- GitHub Check: Konflux kflux-prd-rh02 / crc-binary-on-pull-request
🔇 Additional comments (15)
vendor/github.com/DeedleFake/p9/msg.go (6)
1-10: LGTM!Package declaration and imports are appropriate for the 9P protocol implementation.
12-42: LGTM!Message type identifiers correctly implement the 9P2000 protocol specification, including the appropriate placeholder for the unused Terror type.
44-72: LGTM!Protocol registry correctly maps all message type identifiers to their corresponding Go types.
74-77: LGTM!Proto() function provides appropriate access to the protocol implementation.
79-201: LGTM!Message struct definitions correctly implement the 9P2000 protocol. The special interface methods (P9NoTag, P9Msize, Error) are appropriately implemented.
278-279: LGTM!Empty
Rwstatstruct correctly represents the 9P protocol's Rwstat response, which carries no data.vendor/github.com/DeedleFake/p9/addr_unix.go (5)
1-1: LGTM: Build constraint is appropriate.The
// +build linux darwinconstraint correctly limits this Unix-specific implementation to Linux and macOS, complementing theaddr_other.gofallback for other platforms.
3-9: LGTM: Imports are appropriate.The standard library imports (os, os/user, path/filepath) are sufficient for the NamespaceDir() implementation.
18-21: LGTM: Panic behavior is documented and appropriate.The panic on
user.Current()failure (lines 19-20) is acceptable for a vendored library where user lookup is a critical prerequisite. The behavior is clearly documented in the function comments (lines 15-16).
23-26: LGTM: DISPLAY fallback is reasonable.The fallback to
:0whenDISPLAYis not set (lines 24-25) is appropriate for Unix systems where X11 display:0is the default.
17-29: Remove suggestion to use os.TempDir(): p9’s NamespaceDir deliberately hardcodes “/tmp” to follow Plan 9 conventions; changing it would break compatibility with other 9P tools.Likely an incorrect or invalid review comment.
cmd/crc/cmd/daemon.go (3)
14-14: LGTM!The
runtimeimport is appropriately added to support the Windows-specific conditional logic for 9P file sharing.
28-28: LGTM!The
fs9ppackage import is correctly added to support the new 9P server instantiation.
182-182: LGTM!Refactoring to use
net.JoinHostPortimproves clarity and handles IPv6 addresses correctly.Also applies to: 198-198
pkg/fileserver/fs9p/server.go (1)
29-53: LGTM!The constructor properly validates the exposed directory (absolute path, exists, is directory) before creating the server. Error messages are clear and the validation logic is sound.
| // 9p home directory sharing | ||
| if runtime.GOOS == "windows" { | ||
| listener9p, err := vn.Listen("tcp", net.JoinHostPort(configuration.GatewayIP, fmt.Sprintf("%d", constants.Plan9TcpPort))) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| server9p, err := fs9p.New9pServer(listener9p, constants.GetHomeDir()) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| if err := server9p.Start(); err != nil { | ||
| return err | ||
| } | ||
| } |
There was a problem hiding this comment.
❓ Verification inconclusive
Verify daemon shutdown properly stops the 9p server.
The 9p server is started but there's no corresponding server9p.Stop() call in the shutdown path. When the daemon receives SIGTERM (line 291-297), the 9p server may not be gracefully closed, potentially leaving the listener open or causing client-side issues.
Run the following script to check if there's cleanup logic for the 9p server:
🏁 Script executed:
#!/bin/bash
# Description: Search for 9p server cleanup/shutdown logic
# Check for Stop() calls on server9p or any defer/cleanup patterns
rg -n -A3 -B3 'server9p\.Stop|defer.*Close.*9p' cmd/crc/cmd/
# Look for cleanup in signal handling or shutdown paths
ast-grep --pattern 'case $_ := <-c:
$$$
'Length of output: 0
Ensure 9p server is stopped on daemon shutdown
Add a server9p.Stop() (or equivalent) call in the SIGTERM handler (around lines 291–297) to gracefully close the 9p server started at lines 253–266 in cmd/crc/cmd/daemon.go.
🤖 Prompt for AI Agents
In cmd/crc/cmd/daemon.go around lines 253 to 266, the 9p server started for
Windows is never stopped on shutdown; make server9p a variable scoped outside
the if-block (e.g., var server9p *fs9p.Server or the concrete type) so it is
accessible to the SIGTERM handler, assign it when New9pServer returns, and in
the SIGTERM handler (around lines 291–297) call server9p.Stop() (guarded by a
nil check) to gracefully shut it down; also handle and log any Stop() error and
ensure no double-stop occurs.
Server error channel is never monitored after startup.
Lines 263-265 call server9p.Start() which returns immediately after launching the goroutine. However, server9p.WaitForError() is never called, meaning runtime errors from proto.Serve (e.g., protocol errors, I/O failures) will be silently dropped into server9p.ErrChan with no monitoring or logging.
Consider one of these approaches:
Option 1: Monitor in the existing error channel
if err := server9p.Start(); err != nil {
return err
}
+ go func() {
+ if err := server9p.WaitForError(); err != nil {
+ errCh <- errors.Wrap(err, "9p server failed")
+ }
+ }()
}Option 2: Log errors without propagating to errCh
if err := server9p.Start(); err != nil {
return err
}
+ go func() {
+ if err := server9p.WaitForError(); err != nil {
+ logging.Errorf("9p server error: %v", err)
+ }
+ }()
}📝 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.
| // 9p home directory sharing | |
| if runtime.GOOS == "windows" { | |
| listener9p, err := vn.Listen("tcp", net.JoinHostPort(configuration.GatewayIP, fmt.Sprintf("%d", constants.Plan9TcpPort))) | |
| if err != nil { | |
| return err | |
| } | |
| server9p, err := fs9p.New9pServer(listener9p, constants.GetHomeDir()) | |
| if err != nil { | |
| return err | |
| } | |
| if err := server9p.Start(); err != nil { | |
| return err | |
| } | |
| } | |
| // 9p home directory sharing | |
| if runtime.GOOS == "windows" { | |
| listener9p, err := vn.Listen("tcp", net.JoinHostPort(configuration.GatewayIP, fmt.Sprintf("%d", constants.Plan9TcpPort))) | |
| if err != nil { | |
| return err | |
| } | |
| server9p, err := fs9p.New9pServer(listener9p, constants.GetHomeDir()) | |
| if err != nil { | |
| return err | |
| } | |
| if err := server9p.Start(); err != nil { | |
| return err | |
| } | |
| go func() { | |
| if err := server9p.WaitForError(); err != nil { | |
| errCh <- errors.Wrap(err, "9p server failed") | |
| } | |
| }() | |
| } |
🤖 Prompt for AI Agents
In cmd/crc/cmd/daemon.go around lines 253-266, server9p.Start() is invoked but
its error channel is never monitored; add a watcher after Start() that either
calls server9p.WaitForError() or ranges over server9p.ErrChan in a separate
goroutine, and on any error log it via the existing logger and forward it to the
daemon's central errCh (or if no errCh is available, log the error and handle
shutdown/cleanup accordingly) so runtime protocol/I/O errors are observed and
handled instead of being silently dropped.
| func (file *Remote) walk(p string) (*Remote, error) { | ||
| fid := file.client.nextFID() | ||
|
|
||
| w := []string{path.Clean(p)} | ||
| if w[0] != "/" { | ||
| w = strings.Split(w[0], "/") | ||
| } | ||
| if (len(w) == 1) && (w[0] == ".") { | ||
| w = nil | ||
| } | ||
| rsp, err := file.client.Send(&Twalk{ | ||
| FID: file.fid, | ||
| NewFID: fid, | ||
| Wname: w, | ||
| }) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| walk := rsp.(*Rwalk) | ||
|
|
||
| qid := file.qid | ||
| if len(walk.WQID) != 0 { | ||
| qid = walk.WQID[len(walk.WQID)-1] | ||
| } | ||
| if len(walk.WQID) != len(w) { | ||
| qid = QID{ | ||
| Type: 0xFF, | ||
| Version: 0xFFFFFFFF, | ||
| Path: 0xFFFFFFFFFFFFFFFF, | ||
| } | ||
| } | ||
|
|
||
| return &Remote{ | ||
| client: file.client, | ||
| fid: fid, | ||
| qid: qid, | ||
| }, nil |
There was a problem hiding this comment.
Walk builds illegal Wname entries and ignores partial failures
Line [37] sends components from strings.Split(path.Clean(p), "/"), so "/foo" becomes ["", "foo"] and "/" stays "/". Empty and slash-containing components are invalid; servers reject the walk, and we silently return a Remote with a bogus QID. Additionally, if the server only walks a prefix, we keep the new fid open with a sentinel QID instead of failing. Clean the path by trimming the leading slash and skipping empty segments, and if len(WQID) != len(w) clunk the new fid and return an error so callers don’t operate on the wrong directory.
- w := []string{path.Clean(p)}
- if w[0] != "/" {
- w = strings.Split(w[0], "/")
- }
- if (len(w) == 1) && (w[0] == ".") {
- w = nil
- }
+ cleaned := path.Clean(p)
+ var w []string
+ if cleaned != "." && cleaned != "/" {
+ cleaned = strings.TrimPrefix(cleaned, "/")
+ if cleaned != "" {
+ for _, part := range strings.Split(cleaned, "/") {
+ if part != "" {
+ w = append(w, part)
+ }
+ }
+ }
+ }
@@
- qid := file.qid
- if len(walk.WQID) != 0 {
- qid = walk.WQID[len(walk.WQID)-1]
- }
- if len(walk.WQID) != len(w) {
- qid = QID{
- Type: 0xFF,
- Version: 0xFFFFFFFF,
- Path: 0xFFFFFFFFFFFFFFFF,
- }
- }
+ if len(walk.WQID) != len(w) {
+ _, _ = file.client.Send(&Tclunk{FID: fid})
+ return nil, util.Errorf("walk %q incomplete", p)
+ }
+
+ qid := file.qid
+ if len(walk.WQID) != 0 {
+ qid = walk.WQID[len(walk.WQID)-1]
+ }📝 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.
| func (file *Remote) walk(p string) (*Remote, error) { | |
| fid := file.client.nextFID() | |
| w := []string{path.Clean(p)} | |
| if w[0] != "/" { | |
| w = strings.Split(w[0], "/") | |
| } | |
| if (len(w) == 1) && (w[0] == ".") { | |
| w = nil | |
| } | |
| rsp, err := file.client.Send(&Twalk{ | |
| FID: file.fid, | |
| NewFID: fid, | |
| Wname: w, | |
| }) | |
| if err != nil { | |
| return nil, err | |
| } | |
| walk := rsp.(*Rwalk) | |
| qid := file.qid | |
| if len(walk.WQID) != 0 { | |
| qid = walk.WQID[len(walk.WQID)-1] | |
| } | |
| if len(walk.WQID) != len(w) { | |
| qid = QID{ | |
| Type: 0xFF, | |
| Version: 0xFFFFFFFF, | |
| Path: 0xFFFFFFFFFFFFFFFF, | |
| } | |
| } | |
| return &Remote{ | |
| client: file.client, | |
| fid: fid, | |
| qid: qid, | |
| }, nil | |
| func (file *Remote) walk(p string) (*Remote, error) { | |
| fid := file.client.nextFID() | |
| // Build Wname by cleaning, trimming leading '/', and dropping empty segments. | |
| cleaned := path.Clean(p) | |
| var w []string | |
| if cleaned != "." && cleaned != "/" { | |
| cleaned = strings.TrimPrefix(cleaned, "/") | |
| if cleaned != "" { | |
| for _, part := range strings.Split(cleaned, "/") { | |
| if part != "" { | |
| w = append(w, part) | |
| } | |
| } | |
| } | |
| } | |
| rsp, err := file.client.Send(&Twalk{ | |
| FID: file.fid, | |
| NewFID: fid, | |
| Wname: w, | |
| }) | |
| if err != nil { | |
| return nil, err | |
| } | |
| walk := rsp.(*Rwalk) | |
| // If the server walked fewer components than requested, clunk and error out. | |
| if len(walk.WQID) != len(w) { | |
| _, _ = file.client.Send(&Tclunk{FID: fid}) | |
| return nil, util.Errorf("walk %q incomplete", p) | |
| } | |
| // Otherwise pick up the final QID and return the new Remote. | |
| qid := file.qid | |
| if len(walk.WQID) != 0 { | |
| qid = walk.WQID[len(walk.WQID)-1] | |
| } | |
| return &Remote{ | |
| client: file.client, | |
| fid: fid, | |
| qid: qid, | |
| }, nil | |
| } |
🤖 Prompt for AI Agents
In vendor/github.com/DeedleFake/p9/remote.go around lines 34–70, the walk builds
invalid Wname entries (leading "" from "/foo", "/" left as "/") and silently
returns a Remote with a sentinel QID on partial server walks; fix by normalizing
the path: path.Clean(p), then trim any leading "/" and split, skipping any empty
segments so Wname contains only valid components (handle root as empty slice).
After sending Twalk, if len(rsp.(*Rwalk).WQID) != len(w) then immediately clunk
the newly allocated fid on the server (use the client's clunk mechanism to free
fid) and return a non-nil error to the caller instead of returning a Remote with
a bogus QID.
| next, err := file.walk(p) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| rsp, err := file.client.Send(&Topen{ | ||
| FID: next.fid, | ||
| Mode: mode, | ||
| }) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| open := rsp.(*Ropen) | ||
|
|
||
| next.qid = open.QID | ||
|
|
||
| return next, nil | ||
| } |
There was a problem hiding this comment.
Release the new fid when Topen fails
If Send(Topen) returns an error, Line [90] bails without clunking the fid we just allocated in walk, leaking server-side resources. Call next.Close() (ignoring its error if necessary) before returning the original error.
rsp, err := file.client.Send(&Topen{
FID: next.fid,
Mode: mode,
})
if err != nil {
- return nil, err
+ _ = next.Close()
+ return nil, err
}📝 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.
| next, err := file.walk(p) | |
| if err != nil { | |
| return nil, err | |
| } | |
| rsp, err := file.client.Send(&Topen{ | |
| FID: next.fid, | |
| Mode: mode, | |
| }) | |
| if err != nil { | |
| return nil, err | |
| } | |
| open := rsp.(*Ropen) | |
| next.qid = open.QID | |
| return next, nil | |
| } | |
| next, err := file.walk(p) | |
| if err != nil { | |
| return nil, err | |
| } | |
| rsp, err := file.client.Send(&Topen{ | |
| FID: next.fid, | |
| Mode: mode, | |
| }) | |
| if err != nil { | |
| _ = next.Close() | |
| return nil, err | |
| } | |
| open := rsp.(*Ropen) | |
| next.qid = open.QID | |
| return next, nil | |
| } |
🤖 Prompt for AI Agents
In vendor/github.com/DeedleFake/p9/remote.go around lines 80 to 97, if
file.client.Send(&Topen{...}) returns an error the function returns without
clunking the fid allocated by file.walk, leaking server resources; call
next.Close() (or next.Close(contextless) as appropriate) and ignore any error
from that Close before returning the original Send error so the newly allocated
fid is released, then return nil, err.
| rsp, err := file.client.Send(&Tcreate{ | ||
| FID: next.fid, | ||
| Name: name, | ||
| Perm: perm, | ||
| Mode: mode, | ||
| }) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| create := rsp.(*Rcreate) | ||
|
|
||
| next.qid = create.QID | ||
|
|
||
| return next, nil | ||
| } |
There was a problem hiding this comment.
Also clunk the directory fid when Tcreate fails
Same leak in Create: if the server rejects the create, we never close the directory fid we just walked. Clunk it via next.Close() before returning.
rsp, err := file.client.Send(&Tcreate{
FID: next.fid,
Name: name,
Perm: perm,
Mode: mode,
})
if err != nil {
- return nil, err
+ _ = next.Close()
+ return nil, err
}📝 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.
| rsp, err := file.client.Send(&Tcreate{ | |
| FID: next.fid, | |
| Name: name, | |
| Perm: perm, | |
| Mode: mode, | |
| }) | |
| if err != nil { | |
| return nil, err | |
| } | |
| create := rsp.(*Rcreate) | |
| next.qid = create.QID | |
| return next, nil | |
| } | |
| rsp, err := file.client.Send(&Tcreate{ | |
| FID: next.fid, | |
| Name: name, | |
| Perm: perm, | |
| Mode: mode, | |
| }) | |
| if err != nil { | |
| _ = next.Close() | |
| return nil, err | |
| } | |
| create := rsp.(*Rcreate) | |
| next.qid = create.QID | |
| return next, nil | |
| } |
🤖 Prompt for AI Agents
In vendor/github.com/DeedleFake/p9/remote.go around lines 108 to 122, when the
Tcreate RPC fails we currently return without closing the directory fid returned
by walk (next), leaking the fid; call next.Close() (and ignore or log its error)
before returning the RPC error so the fid is clunked, i.e. invoke next.Close()
in the err != nil branch right before returning nil, err.
|
@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. |
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
Bug Fixes
Chores