9p file sharing #4866
9p file sharing #4866
Conversation
WalkthroughAdds a vendored 9P implementation and integrates Plan9 (9P) sharing: new fs9p server, vendored p9/proto client-server/FS code, constants and go.mod dependency, Windows daemon logic to serve host home over TCP:564, guest-side mount support, and driver changes to advertise 9P shares. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant Daemon
participant FS as fs9p Server
Note over Daemon: daemon startup (cross-platform listeners)
User->>Daemon: start daemon
Daemon->>Daemon: bind listeners (net.JoinHostPort)
alt Windows
Daemon->>FS: listen TCP on GatewayIP:564
FS-->>Daemon: Start() (serving host home)
Daemon->>FS: monitor WaitForError() (background)
Daemon->>FS: Stop() on shutdown
end
Daemon-->>User: signal startupDone
sequenceDiagram
autonumber
participant Starter as VM Starter
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 to GatewayIP:564 (or VSock gateway)
VM->>HostFS: 9P handshake & attach
HostFS-->>VM: serve filesystem ops (read/write/readdir)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 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.
| 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 | |
| } | |
| } |
redbeam
force-pushed
the
9p-deedlefake
branch
2 times, most recently
from
6362817 to
3093c61
Compare
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.
| 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.
654e75d to
1e6dbd0
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
pkg/crc/machine/start.go (1)
264-264: Fix the chown command syntax.This issue was previously flagged. The command
"chown core:core"is passed as a single argument, butRunPrivilegedexpects the command name followed by separate arguments.Apply this diff:
- 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 {
🧹 Nitpick comments (3)
pkg/fileserver/fs9p/server.go (3)
56-70: Consider adding a brief timeout to catch early startup errors.The non-blocking
selectat lines 63-69 may miss errors that occur immediately after the goroutine starts but before theselectexecutes. While the buffered channel mitigates this and most errors occur later, adding a small timeout (e.g., 100ms) would provide more reliable startup error detection.// 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: + 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
time.
72-82: Improve the zombie process warning documentation.The comment about "BAD THINGS" is concerning for production use. Consider rephrasing more professionally to help operators understand the implications and recommended precautions.
// Stop a running server. -// Please note that this does *BAD THINGS* to clients if they are still running -// when the server stops. Processes get stuck in I/O deep sleep and zombify, and -// nothing I do other than restarting the VM can remove the zombies. +// Stop a running server. +// WARNING: Stopping the server while clients are actively connected may cause +// client processes to become unresponsive due to 9P protocol limitations. +// VM restart may be required to recover. Ensure all clients have disconnected +// before calling Stop() when possible. func (s *Server) Stop() error {
84-88: Consider adding context support to WaitForError.The method blocks indefinitely. Adding a context parameter would allow callers to set timeouts or cancel the wait, improving robustness.
-// WaitForError from a running server. -func (s *Server) WaitForError() error { - err := <-s.ErrChan - return err -} +// WaitForError from a running server with context support. +func (s *Server) WaitForError(ctx context.Context) error { + select { + case err := <-s.ErrChan: + return err + case <-ctx.Done(): + return ctx.Err() + } +}You'll need to import
contextand update the caller incmd/crc/cmd/daemon.go.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
cmd/crc/cmd/daemon.go(5 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)
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/crc/constants/constants.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 (3)
cmd/crc/cmd/daemon.go (3)
pkg/crc/constants/constants.go (2)
Plan9TcpPort(60-60)GetHomeDir(164-170)pkg/fileserver/fs9p/server.go (1)
New9pServer(29-53)pkg/crc/logging/logging.go (1)
Warnf(100-102)
pkg/crc/machine/start.go (1)
pkg/os/exec.go (1)
RunPrivileged(48-59)
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)
⏰ 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). (2)
- GitHub Check: build (ubuntu-22.04, 1.24)
- GitHub Check: Konflux kflux-prd-rh02 / crc-binary-on-pull-request
🔇 Additional comments (2)
cmd/crc/cmd/daemon.go (1)
182-182: LGTM: Improved host:port construction.Using
net.JoinHostPortinstead offmt.Sprintfis more idiomatic and properly handles edge cases like IPv6 addresses.Also applies to: 198-198
pkg/fileserver/fs9p/server.go (1)
29-53: LGTM: Constructor properly validates and initializes.The constructor correctly validates the directory path, ensures it exists and is a directory, and initializes the server with a buffered error channel (addressing previous concerns about goroutine blocking).
redbeam
force-pushed
the
9p-deedlefake
branch
from
659a961 to
dd593f8
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (24)
pkg/fileserver/fs9p/server.go (3)
60-75: Startup race: wait briefly for early errors before returning from Start()The non-blocking select can miss immediate errors from proto.Serve/Accept. Wait a short time window.
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("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(150 * time.Millisecond): + logrus.Infof("started 9p server on %s for directory %s", s.Listener.Addr().String(), s.ExposedDir) + return nil + } }Add:
import "time".
77-87: Clarify Stop() behavior and wrap errors for operatorsMake the comment actionable and wrap the 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. Ensure clients disconnect before stopping the server. 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("stopped 9p server for directory %s", s.ExposedDir) return nil }
89-98: Avoid indefinite blocking: add context to WaitForErrorLet callers bound the wait and handle shutdown uniformly.
-// WaitForError from a running server. -func (s *Server) WaitForError() error { - err := <-s.ErrChan +// WaitForError from a running server with context support. +func (s *Server) WaitForError(ctx context.Context) error { + var err error + select { + case err = <-s.ErrChan: + case <-ctx.Done(): + return ctx.Err() + } // captures "accept tcp: endpoint is in invalid state" errors on exit var opErr *net.OpError if errors.As(err, &opErr) && strings.Contains(opErr.Error(), "endpoint is in invalid state") { return nil } return err }Add:
import "context". Update callers accordingly.vendor/github.com/DeedleFake/p9/proto/encoding.go (3)
65-71: Fix Size() for custom Encoder: count payload bytes, not slice headercase 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
146-153: Remove unsafe pointer cast; read uint16 length safelydefault: - d.read((*uint16)(unsafe.Pointer(&length))) + var len16 uint16 + d.read(&len16) + length = uint32(len16)
164-172: Avoid unsafe []byte -> string conversion; copy for safetybuf := make([]byte, int(length)) d.read(buf) - - v.SetString(*(*string)(unsafe.Pointer(&buf))) + v.SetString(string(buf))vendor/github.com/DeedleFake/p9/dir_darwin.go (1)
35-43: Fix ATime conversion on Darwin: Timespec has Sec/Nsec, no Unix()return DirEntry{ FileMode: ModeFromOS(fi.Mode()), - ATime: time.Unix(sys.Atimespec.Unix()), + ATime: time.Unix(sys.Atimespec.Sec, sys.Atimespec.Nsec), MTime: fi.ModTime(), Length: uint64(fi.Size()), EntryName: fi.Name(), UID: uname, GID: gname, }vendor/github.com/DeedleFake/p9/dir_linux.go (1)
35-43: Fix ATime conversion on Linux: Timespec has Sec/Nsec, no Unix()return DirEntry{ FileMode: ModeFromOS(fi.Mode()), - ATime: time.Unix(sys.Atim.Unix()), + ATime: time.Unix(sys.Atim.Sec, sys.Atim.Nsec), MTime: fi.ModTime(), Length: uint64(fi.Size()), EntryName: fi.Name(), UID: uname, GID: gname, }vendor/github.com/DeedleFake/p9/remote.go (4)
37-43: Normalize path components to avoid empty Wname entries.Trim leading slash and drop empties; handle "." and "/" as root (nil).
- 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) + } + } + } + }
58-66: Clunk new fid and error on incomplete walk.Avoid returning a Remote with bogus QID and leaking fid.
- 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) + }
85-92: Release fid if Topen fails.Prevents server-side fid leak.
rsp, err := file.client.Send(&Topen{ FID: next.fid, Mode: mode, }) if err != nil { - return nil, err + _ = next.Close() + return nil, err }
108-116: Also clunk directory fid if Tcreate fails.Prevents fid leak on error.
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 }vendor/github.com/DeedleFake/p9/proto/client.go (2)
90-97: Avoid tight/infinite read-error loop. Return or back off.Continuing on non-EOF read errors can spin forever if the conn stays bad.
- 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 + }
138-145: Never allocate reserved NoTag from tag allocator.Skip NoTag and any in-use tag, including on wrap-around.
- 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/proto.go (2)
85-94: Bound reader to payload (size-4) and guard short frames.Current N=size risks over-read into the next frame and lacks short-size guard.
- 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 inside t==nil.err isn’t set by TypeFromID; this block is dead code.
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) }vendor/github.com/DeedleFake/p9/proto/server.go (2)
70-78: Add continue after logging read error.Prevents processing invalid tmsg/tag.
if err != nil { if err == io.EOF { return } log.Printf("Error reading message: %v", err) + continue }
79-99: Avoid closure over loop vars tag/tmsg when switching to concurrent mode.Copy to locals before invoking mode() to prevent data races.
- mode(func() { - rmsg := handler.HandleMessage(tmsg) + ctag := tag + ctmsg := tmsg + mode(func() { + rmsg := handler.HandleMessage(ctmsg) if rmsg, ok := rmsg.(Msizer); ok { @@ - err := p.Send(c, tag, rmsg) + err := p.Send(c, ctag, rmsg)vendor/github.com/DeedleFake/p9/stat.go (1)
152-153: Avoid unsafe conversion; use string(buf).Safer and idiomatic; negligible overhead here.
- return *(*string)(unsafe.Pointer(&buf)) + return string(buf)vendor/github.com/DeedleFake/p9/msg.go (2)
207-217: Return immediately on write error to prevent partial/corrupt buffers.Stop after each failed proto.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: Same early-return fix for Twstat.P9Encode.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(), nilvendor/github.com/DeedleFake/p9/fs.go (3)
179-181: Remove unsafe string-to-bytes conversion.Unsafe violates Go’s guarantees; use safe conversion.
- sum := sha256.Sum256(*(*[]byte)(unsafe.Pointer(&p))) + sum := sha256.Sum256([]byte(p))
536-537: Writes must take exclusive lock (RLock is unsafe here).Concurrent writes under RLock can corrupt state.
- file.RLock() // Somewhat ironic that this doesn't require a - defer file.RUnlock() // full lock like read() does. + file.Lock() + defer file.Unlock()
719-724: Close(): lock per file before closing to avoid races.Ensure no concurrent ops touch file 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.file = nil + } + file.Unlock() return true }) return nil }
🧹 Nitpick comments (14)
vendor/github.com/DeedleFake/p9/README.md (2)
12-12: Fix markdown heading style inconsistency.Lines 12 and 21 use ATX-style headings (
###) but should use setext-style headings (underlined with===or---) to match the title format on lines 1-2.Apply this diff:
-### Server +Server +-------### Client +Client +------Also applies to: 21-21
17-17: Replace hard tabs with spaces in code examples.The code examples contain hard tabs (lines 17, 26, 32, 37, 43, 49) which should be replaced with spaces for consistent indentation across different editors and tools.
Apply this diff to the code blocks:
err := p9.ListenAndServe("tcp", "localhost:5640", p9.FSConnHandler(fsImplementation)) if err != nil { - log.Fatalf("Failed to start server: %v", err) + log.Fatalf("Failed to start server: %v", err) }_, err := c.Handshake(2048) if err != nil { - log.Fatalf("Failed to perform handshake: %v", err) + log.Fatalf("Failed to perform handshake: %v", err) }(Apply the same fix to all other hard-tabbed lines in both code examples.)
Also applies to: 26-26, 32-32, 37-37, 43-43, 49-49
vendor/github.com/DeedleFake/p9/internal/debug/nodebug.go (1)
1-1: Informational: Old-style build constraint in vendored code.The build constraint uses the deprecated
// +buildsyntax. Modern Go prefers//go:build !p9debug. However, since this is vendored code from an external dependency, it should not be modified here. If needed, the change should be submitted upstream to github.com/DeedleFake/p9.vendor/github.com/DeedleFake/p9/dir_windows.go (1)
9-27: Windows ATime: optional fallback when access time is disabledOn some setups LastAccessTime can be zero. Fallback to ModTime to avoid zero epoch timestamps in listings.
func infoToEntry(fi os.FileInfo) DirEntry { sys, _ := fi.Sys().(*syscall.Win32FileAttributeData) if sys == nil { return DirEntry{ FileMode: ModeFromOS(fi.Mode()), MTime: fi.ModTime(), Length: uint64(fi.Size()), EntryName: fi.Name(), } } - - return DirEntry{ - FileMode: ModeFromOS(fi.Mode()), - ATime: time.Unix(0, sys.LastAccessTime.Nanoseconds()), - MTime: fi.ModTime(), - Length: uint64(fi.Size()), - EntryName: fi.Name(), - } + at := fi.ModTime() + if ns := sys.LastAccessTime.Nanoseconds(); ns > 0 { + at = time.Unix(0, ns) + } + return DirEntry{ + FileMode: ModeFromOS(fi.Mode()), + ATime: at, + MTime: fi.ModTime(), + Length: uint64(fi.Size()), + EntryName: fi.Name(), + } }pkg/fileserver/fs9p/server.go (1)
31-58: Normalize and store a cleaned pathMinor: clean the exposed path to avoid surprises with trailing separators/.. segments.
func New9pServer(listener net.Listener, exposeDir string) (*Server, error) { // verify that exposeDir makes sense if !filepath.IsAbs(exposeDir) { return nil, fmt.Errorf("path to expose to machine must be absolute: %s", exposeDir) } - stat, err := os.Stat(exposeDir) + exposeDir = filepath.Clean(exposeDir) + stat, err := os.Stat(exposeDir) if err != nil { return nil, fmt.Errorf("cannot stat path to expose to machine: %w", err) }vendor/github.com/DeedleFake/p9/dir_darwin.go (1)
23-33: Optional: cache UID/GID lookups to avoid per-entry NSS callsUser/group lookups per file can be expensive. Consider a small LRU or map cache keyed by uid/gid strings during a directory walk.
vendor/github.com/DeedleFake/p9/dir_linux.go (1)
23-33: Optional: cache UID/GID lookups during directory scansSame as Darwin: memoize user.LookupId/group lookups to reduce NSS overhead.
vendor/github.com/DeedleFake/p9/dir_plan9.go (1)
39-42: Tighten error message for diagnostics.Clarify actual type received to ease debugging.
- return QID{}, errors.New("failed to get QID: FileInfo was not Dir") + return QID{}, errors.New("failed to get QID: FileInfo.Sys() was not *syscall.Dir")vendor/github.com/DeedleFake/p9/fs.go (2)
244-249: Handle Tflush gracefully (return Rflush or implement cancellation).Returning Rerror breaks clients relying on flush semantics.
func (h *fsHandler) flush(msg *Tflush) interface{} { - // TODO: Implement this. - return &Rerror{ - Ename: "flush is not supported", - } + // Best-effort acknowledge; actual cancellation not guaranteed. + return &Rflush{} }
499-512: Directory read offset handling deviates from spec — confirm or align.Spec requires dir reads at 0 or previous offset+len. Current code ignores non‑zero offsets.
Would you like a patch to track and validate offsets per FID?
vendor/github.com/DeedleFake/p9/remote.go (4)
198-200: Guard against pathological msize values.Ensure non-negative buffer size.
-func (file *Remote) maxBufSize() int { - return int(file.client.Msize() - IOHeaderSize) -} +func (file *Remote) maxBufSize() int { + ms := int(file.client.Msize()) + if ms <= IOHeaderSize { + return 0 + } + return ms - IOHeaderSize +}
273-275: Use io.ErrShortWrite for partial write, not EOF.EOF is for reads; short writes should return ErrShortWrite.
- if write.Count < uint32(len(data)) { - return int(write.Count), io.EOF - } + if write.Count < uint32(len(data)) { + return int(write.Count), io.ErrShortWrite + }
184-185: Return an error instead of panic on invalid whence.Avoid crashing caller; align with io.Seeker semantics.
- panic(util.Errorf("Invalid whence: %v", whence)) + return int64(file.pos), util.Errorf("invalid whence: %v", whence)
127-135: Minor: avoid shadowing receiver in Remove().Improves readability.
- file, err := file.walk(p) + next, err := file.walk(p) if err != nil { return err } // Close is not necessary. Remove is also a clunk. - return file.Remove("") + return next.Remove("")
📜 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 (16)
- vendor/github.com/DeedleFake/p9/internal/debug/debug.go
- vendor/github.com/DeedleFake/p9/addr_other.go
- pkg/crc/machine/libhvee/driver_windows.go
- vendor/modules.txt
- pkg/crc/machine/start.go
- vendor/github.com/DeedleFake/p9/p9.go
- vendor/github.com/DeedleFake/p9/dir_other.go
- vendor/github.com/DeedleFake/p9/doc.go
- go.mod
- vendor/github.com/DeedleFake/p9/client.go
- pkg/crc/constants/constants.go
- cmd/crc/cmd/daemon.go
- vendor/github.com/DeedleFake/p9/dir.go
- vendor/github.com/DeedleFake/p9/LICENSE
- vendor/github.com/DeedleFake/p9/encoding.go
- vendor/github.com/DeedleFake/p9/internal/util/util.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 (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/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)
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_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/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/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/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 (26)
Tversion(79-82)Tversion(84-84)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)
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/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/internal/debug/nodebug.go (1)
vendor/github.com/DeedleFake/p9/internal/debug/debug.go (1)
Log(10-12)
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/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 (1)
vendor/github.com/DeedleFake/p9/proto/proto.go (1)
Proto(38-41)
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 (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/dir_linux.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)
🪛 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: build (macOS-13, 1.24)
- GitHub Check: build-qe (darwin, amd64)
- GitHub Check: build (ubuntu-latest, 1.24)
- GitHub Check: Konflux kflux-prd-rh02 / crc-binary-on-pull-request
🔇 Additional comments (8)
vendor/github.com/DeedleFake/p9/README.md (1)
1-51: Good documentation structure for the 9P library.The README provides clear examples and explains the package's purpose well. The content is well-organized with practical server and client usage examples.
vendor/github.com/DeedleFake/p9/internal/debug/nodebug.go (1)
3-5: LGTM! Correct no-op debug implementation.The no-op
Logfunction correctly implements the conditional compilation pattern. When thep9debugbuild tag is not set, this version compiles instead of the actual logging implementation indebug.go, eliminating debug overhead at compile time. The function signature matches the active implementation, ensuring API consistency.vendor/github.com/DeedleFake/p9/addr_unix.go (3)
28-28: Path construction differs between Unix and non-Unix platforms.The Unix version hardcodes
/tmpusingfilepath.Join("/", "tmp", ...), while the non-Unix version (addr_other.go) usesos.TempDir(). This creates a platform-specific discrepancy:
- Unix/Darwin:
/tmp/ns.$USER.$DISPLAY- Windows/Other:
%TEMP%\ns.$USER.$DISPLAYThis is likely intentional (following Plan 9/Unix conventions), but verify that:
- Windows 9P clients/servers expect a different namespace directory path
- The hardcoded
/tmppath is appropriate for all Unix-like CRC environments (containers, custom setups)- The namespace directory is created if it doesn't exist (check calling code)
1-29: Vendored p9 library is current but upstream project is unmaintained.The vendored version v0.6.12 is documented in go.mod and is the latest available release. However, the upstream DeedleFake/p9 project has not been actively maintained since March 2021 (3.5+ years with no releases). No security advisories were detected for this library. Verify that using an unmaintained dependency is acceptable for your use case.
18-20: This review comment is incorrect and should be dismissed.The code under review is in the vendored
github.com/DeedleFake/p9library, not part of CRC's codebase. The panic inNamespaceDir()is only triggered if the code callsParseAddr()with a namespace-formatted address (like/program_name), which then callsGetNamespace(), which callsNamespaceDir().However, the CRC 9P server implementation in
pkg/fileserver/fs9p/server.godoes not useParseAddr(),GetNamespace(), orNamespaceDir()at all. It creates a filesystem directly withp9.Dir()and serves it viaproto.Serve()withp9.FSConnHandler(). This panic code path is unreachable in the CRC application. Additionally, the panic behavior is already documented in the upstream library's function comment (lines 15-16 ofaddr_unix.go), so it's an intentional design choice by the library maintainers.Likely an incorrect or invalid review comment.
vendor/github.com/DeedleFake/p9/addr.go (3)
8-8: LGTM: Standard 9P port is correctly defined.Port 564 is the IANA-assigned standard port for 9P2000, correctly used here for address parsing fallbacks and pseudo-port mapping.
70-72: GetNamespace inherits panic behavior from NamespaceDir().This function calls
NamespaceDir(), which panics ifuser.Current()fails (see addr_unix.go review). Any code callingGetNamespace()(orParseAddr()with namespace addresses) must be prepared for potential panics.Ensure that 9P server initialization code accounts for this behavior, especially in containerized or non-standard user environments.
32-66: I need to verify if the p9 library (and specifically ParseAddr) is actually used in the codebase. The initial search didn't find direct calls to ParseAddr from the DeedleFake/p9 package.<!-- [scratchpad_update]
[observations]
- Web search confirms ParseAddr is a utility function in the p9 library that parses address strings
- The p9 library appears to be used in the codebase (server.go has p9.FSConnHandler, proto.Serve references)
- ParseAddr converts a string into network and address pair for establishing connections
- The review comment concerns input validation for the function
[pending]
- Still need to confirm if ParseAddr is specifically called in the codebase
- Search didn't explicitly show if ParseAddr is invoked by the application code
- Need to refocus: Is this a vendored file that shouldn't be modified?
[actions]
- Run focused search on the p9 library usage in codebase
- Check if vendored code is expected to be modified or just a dependency
[scratchpad_update] -->Let me search more specifically for how the p9 library is used in the codebase:<!-- [scratchpad_update]
[task verify_review_comment_1/1][observations]
- Web search results clarify vendoring practices in Go
- Vendored code is a "copy" of dependencies in the project directory
- Convention: vendored code should NOT be modified because changes get wiped out on updates
- Modifying vendored dependencies is generally discouraged; better to fork and use replace directive
- The review comment is about vendored code: vendor/github.com/DeedleFake/p9/addr.go
[key_insight]
- Reviewing and requesting improvements to vendored code is fundamentally problematic
- Vendored files are not part of the project's codebase to be modified
- The review comment's requests cannot/should not be acted upon since it's vendored
[conclusion]
- The original review comment is attempting to request changes to vendored code
- This is outside the scope of code review for a repository
- The file is from an external library (DeedleFake/p9) and shouldn't be modified
- The review comment itself is incorrect in context
[done]
The review comment is flagged as because it attempts to suggest improvements to vendored third-party code, which is not part of the project's modifiable codebase.
[scratchpad_update] -->Based on my verification, I can now provide the final assessment:
The file
vendor/github.com/DeedleFake/p9/addr.gois vendored third-party code from the DeedleFake/p9 library. Vendored dependencies should not be modified in a repository—they represent external libraries with their own maintenance and versioning. If improvements are needed in the upstream library, the proper approach is to contribute directly to the DeedleFake/p9 project or use a fork with ago.modreplace directive, not to modify the vendored copy (which would be overwritten on dependency updates). The review comment's request for verification and changes does not apply to vendored code in this context.Likely an incorrect or invalid review comment.
redbeam
force-pushed
the
9p-deedlefake
branch
from
dd593f8 to
d56fb1a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
pkg/crc/machine/start.go (1)
264-264: Fix the chown command argument passing.The command arguments are not properly split.
RunPrivilegedexpects the command name followed by individual arguments, but"chown core:core"is passed as a single string.Apply this diff:
- 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 {
🧹 Nitpick comments (2)
pkg/fileserver/fs9p/server.go (2)
61-74: Start method correctly prevents early error detection race.The non-blocking select allows the server to start asynchronously while catching immediate startup failures. For most use cases, this is sufficient. If more robust startup synchronization is needed in the future, consider adding a small timeout (e.g., 100ms) to the select statement.
89-98: WaitForError handles shutdown gracefully.The special handling for "endpoint is in invalid state" errors during normal shutdown is appropriate. The method correctly blocks until an error occurs or the channel is closed.
For enhanced robustness, consider adding context support to prevent indefinite blocking, but this is not critical for the current use case.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
cmd/crc/cmd/daemon.go(5 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)
🚧 Files skipped from review as they are similar to previous changes (1)
- 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 (3)
cmd/crc/cmd/daemon.go (4)
pkg/crc/config/settings.go (1)
EnableSharedDirs(34-34)pkg/crc/constants/constants.go (2)
Plan9TcpPort(60-60)GetHomeDir(164-170)pkg/fileserver/fs9p/server.go (1)
New9pServer(34-58)pkg/crc/logging/logging.go (1)
Warnf(100-102)
pkg/crc/machine/start.go (2)
pkg/os/exec.go (1)
RunPrivileged(48-59)pkg/crc/constants/constants.go (1)
VSockGateway(42-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)
⏰ 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.24)
- GitHub Check: build (windows-2022, 1.24)
- GitHub Check: build (ubuntu-22.04, 1.24)
- GitHub Check: build (macOS-13, 1.24)
- GitHub Check: build (ubuntu-latest, 1.24)
- GitHub Check: build (macOS-14, 1.24)
- GitHub Check: build-qe (darwin, amd64)
- GitHub Check: Run OKD bundle with crc (1.24)
- GitHub Check: build-qe (linux, arm64)
- GitHub Check: build-qe (windows, amd64)
- GitHub Check: build-qe (darwin, arm64)
- GitHub Check: build-qe (linux, amd64)
- GitHub Check: verify-devcontainer
- GitHub Check: build-installer (windows-2022, 1.24)
- GitHub Check: build (macOS-13, 1.24)
- GitHub Check: build (windows-2022, 1.24)
- GitHub Check: build (macOS-14, 1.24)
- GitHub Check: Konflux kflux-prd-rh02 / crc-binary-on-pull-request
🔇 Additional comments (9)
pkg/crc/constants/constants.go (1)
59-60: LGTM!The constants are well-defined and appropriate for 9P protocol support. Port 564 is the standard IANA-assigned port for Plan 9, and the 1 MB message size is a reasonable choice for 9P operations.
pkg/crc/machine/start.go (2)
255-259: Good security practice for masking credentials.The error handling properly masks the password in error messages using
MaskedSecretError, preventing credential leakage in logs or error output.
267-267: Use VSockGateway constant appropriately.Good use of
constants.VSockGatewayinstead of a hardcoded IP. However, verify that theRunmethod correctly handles these as separate arguments (command, IP, target).Based on the pattern at lines 248 and 254, verify the arguments are passed correctly to the
Runmethod.cmd/crc/cmd/daemon.go (5)
14-14: LGTM!Appropriate import of
runtimepackage for OS detection in the Windows-specific 9P sharing logic.
28-28: LGTM!Appropriate import of the new
fs9ppackage for 9P server functionality.
182-182: Good use of net.JoinHostPort for proper address formatting.Using
net.JoinHostPortensures correct IPv6 and IPv4 address handling when constructing the listener address.
198-198: Consistent use of net.JoinHostPort.Good consistency with the gateway listener setup.
253-276: Well-structured 9P server lifecycle management.The Windows-specific 9P sharing implementation correctly:
- Checks both OS and configuration settings
- Uses appropriate constants for port configuration
- Handles errors at each initialization step
- Manages server lifecycle with proper cleanup via
defer- Monitors server errors in a background goroutine without busy-waiting
pkg/fileserver/fs9p/server.go (1)
34-58: Solid validation and initialization logic.The constructor properly validates:
- Path is absolute
- Directory exists and is accessible
- All required fields are initialized
The buffered channel prevents goroutine leaks on shutdown.
redbeam
force-pushed
the
9p-deedlefake
branch
from
d56fb1a to
a12218e
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pkg/fileserver/fs9p/server.go (1)
77-87: Consider runtime warning for shutdown risks.The comment correctly notes that stopping the server with active clients causes process zombification in the VM. While the
Stopmethod is properly integrated withdeferin the daemon, the operational risk isn't visible at runtime.Consider adding a structured warning log when
Stop()is called to make this risk visible to operators:func (s *Server) Stop() error { + logrus.Warnf("stopping 9p server for %s - active client processes may zombify until VM restart", s.ExposedDir) if err := s.Listener.Close(); err != nil { return err } - logrus.Infof("stopped 9p server for directory %s", s.ExposedDir) return nil }This makes the limitation discoverable in logs during troubleshooting.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
cmd/crc/cmd/daemon.go(5 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)
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/crc/machine/start.go
- pkg/crc/machine/libhvee/driver_windows.go
- pkg/crc/constants/constants.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 (2)
cmd/crc/cmd/daemon.go (4)
pkg/crc/config/settings.go (1)
EnableSharedDirs(34-34)pkg/crc/constants/constants.go (2)
Plan9TcpPort(60-60)GetHomeDir(164-170)pkg/fileserver/fs9p/server.go (1)
New9pServer(34-58)pkg/crc/logging/logging.go (1)
Warnf(100-102)
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)
⏰ 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-qe (windows, amd64)
- GitHub Check: build-qe (linux, amd64)
- GitHub Check: build-qe (linux, arm64)
- GitHub Check: build-qe (darwin, arm64)
- GitHub Check: build-qe (darwin, amd64)
- GitHub Check: build-installer (windows-2022, 1.24)
- GitHub Check: build (ubuntu-latest, 1.24)
- GitHub Check: verify-devcontainer
- GitHub Check: build (macOS-13, 1.24)
- GitHub Check: build (macOS-14, 1.24)
- GitHub Check: build (ubuntu-22.04, 1.24)
- GitHub Check: build (macOS-13, 1.24)
- GitHub Check: build (macOS-14, 1.24)
- GitHub Check: build (ubuntu-latest, 1.24)
- GitHub Check: Run OKD bundle with crc (1.24)
- GitHub Check: build (windows-2022, 1.24)
- GitHub Check: build (windows-2022, 1.24)
- GitHub Check: Konflux kflux-prd-rh02 / crc-binary-on-pull-request
🔇 Additional comments (6)
cmd/crc/cmd/daemon.go (3)
14-14: LGTM: Import additions support 9p integration.The
runtimeimport enables OS detection for Windows-specific 9p sharing, and thefs9pimport provides the new server implementation.Also applies to: 28-28
182-182: LGTM: Improved host:port construction.Using
net.JoinHostPortis more robust than string concatenation and properly handles both IPv4 and IPv6 addresses. Good consistency with the 9p listener initialization.Also applies to: 198-198
253-276: LGTM: Well-structured 9p sharing lifecycle.The Windows-specific 9p home directory sharing implementation properly handles:
- Conditional execution (OS + config check)
- Listener creation on the virtual network
- Server initialization and startup with error propagation
- Cleanup via defer to ensure
Stop()runs on exit- Background error monitoring with single
WaitForError()callThe use of
constants.GetHomeDir()can panic if the home directory is unavailable, but this is acceptable since sharing cannot proceed without it, and the failure occurs during startup (making it immediately visible).pkg/fileserver/fs9p/server.go (3)
34-58: LGTM: Robust constructor validation.The constructor performs thorough validation:
- Requires absolute paths (prevents ambiguity)
- Verifies directory existence via
os.Stat- Confirms path is a directory, not a file
- Creates buffered
ErrChan(size 1) to prevent goroutine blocking on shutdownError messages are clear and actionable. Well-designed initialization.
60-75: LGTM: Server startup with immediate error detection.The
Startmethod launchesproto.Servein a goroutine and uses a non-blocking select to detect immediate startup failures (e.g., invalid listener). The design is sound:proto.Serveloops onAccept(), so listener errors surface immediately, while operational errors during serving are caught byWaitForError().A past review suggested adding a 100ms timeout to the select for more defensive error detection. While the current approach works in practice, adding a timeout would eliminate the theoretical race window. Given the narrow scope of the race, this is an optional improvement.
89-98: LGTM: Error monitoring with graceful shutdown handling.
WaitForErrorblocks on the error channel and includes special handling for the Windows-specific "endpoint is in invalid state" error during graceful shutdown. The method works correctly with the current usage pattern indaemon.go(single call in a goroutine), where the channel is guaranteed to close afterproto.Serveexits.A past review suggested adding
context.Contexttimeout support for more defensive programming. The current implementation is functionally correct for the usage pattern, so context support would be an optional improvement if this method is reused in scenarios requiring timeout control.
redbeam
force-pushed
the
9p-deedlefake
branch
from
a12218e to
3d3599c
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (21)
pkg/fileserver/fs9p/server.go (3)
89-98: WaitForError blocks indefinitely without timeout.The method blocks forever waiting on
ErrChan. If the server never produces an error, callers will hang indefinitely with no way to cancel.Add context support for cancellation:
-func (s *Server) WaitForError() error { +func (s *Server) WaitForError(ctx context.Context) error { + select { + case err := <-s.ErrChan: - err := <-s.ErrChan // captures "accept tcp: endpoint is in invalid state" errors on exit var opErr *net.OpError if errors.As(err, &opErr) && strings.Contains(opErr.Error(), "endpoint is in invalid state") { return nil } return err + case <-ctx.Done(): + return ctx.Err() + } - return err }Update callers to pass a context (e.g., from daemon lifecycle).
60-75: Startup error detection may miss early failures.The non-blocking
selectwithdefaultat lines 68-74 may return success before the server goroutine encounters an error. Ifproto.Servefails immediately after the goroutine starts, the error might not be caught by the select.Consider adding a brief timeout to improve reliability:
// 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) + case <-time.After(100 * time.Millisecond): + logrus.Infof("started 9p server on %s for directory %s", s.Listener.Addr().String(), s.ExposedDir) + return nil default: logrus.Infof("started 9p server on %s for directory %s", s.Listener.Addr().String(), s.ExposedDir) return nil }
77-87: Document zombie process risk more prominently for operators.The comment about zombie processes is critical for production operations but easy to miss. When clients are forcefully disconnected, their processes enter uninterruptible sleep.
Enhance operational visibility:
func (s *Server) Stop() error { + logrus.Warnf("stopping 9p server for %s - active client connections may zombify", s.ExposedDir) if err := s.Listener.Close(); err != nil { return err } logrus.Infof("stopped 9p server for directory %s", s.ExposedDir) return nil }Additionally, document this limitation in user-facing documentation with recommended shutdown procedures.
vendor/github.com/DeedleFake/p9/client.go (1)
1-120: Vendored library contains critical issues flagged in previous reviews.This is a vendored dependency from
github.com/DeedleFake/p9. The past review comments identified several critical issues that remain unaddressed:
- Lines 45-47: FID generation can overflow causing conflicts
- Lines 61-62: Unsafe type assertion on
rsp.(*Rversion)can panic- Lines 84: Unsafe type assertion on
rsp.(*Rauth)can panic- Line 113: Unsafe type assertion on
rsp.(*Rattach)can panicSince this is vendored code, these issues should be reported and fixed upstream in the
github.com/DeedleFake/p9repository rather than patched locally.Consider opening an issue upstream or contributing fixes to the DeedleFake/p9 project for these type safety concerns.
vendor/github.com/DeedleFake/p9/proto/encoding.go (1)
1-195: Vendored encoding library has critical correctness and portability bugs.This vendored file from
github.com/DeedleFake/p9contains three unresolved issues from previous reviews:
- Lines 65-71: Custom
Encoderhandling incorrectly calculates size (measures slice header instead of data length)- Lines 150-155: Unsafe pointer cast may cause endianness issues on big-endian systems
- Line 173: Unsafe string conversion using
unsafe.Stringviolates Go memory safetyThese bugs affect correctness and portability of the 9P protocol implementation. Since this is vendored code, report these issues upstream to
github.com/DeedleFake/p9.Verify the version of the vendored library and check if newer versions address these issues.
#!/bin/bash # Check the vendored library version grep "github.com/DeedleFake/p9" go.modvendor/github.com/DeedleFake/p9/msg.go (1)
207-217: Error handling allows partial writes in P9Encode.In the vendored
Rstat.P9Encodemethod, if the firstproto.Writesucceeds but the second fails, the method returns partial data inbuf.Bytes(). This violates atomicity and could corrupt the protocol stream.The same issue exists in
Twstat.P9Encode(lines 240-255).Since this is vendored from
github.com/DeedleFake/p9, report this upstream.vendor/github.com/DeedleFake/p9/dir_linux.go (1)
37-37: Critical: Timespec.Unix() method doesn't exist on Linux.Line 37 calls
sys.Atim.Unix(), butsyscall.Timespecon Linux only hasSecandNsecfields, noUnix()method. This will cause a compilation error.This is vendored code from
github.com/DeedleFake/p9and must be fixed upstream.Verify this compiles on Linux:
#!/bin/bash # Check if this file compiles on Linux GOOS=linux go build -o /dev/null ./vendor/github.com/DeedleFake/p9/dir_linux.go 2>&1 || echo "Compilation failed as expected"vendor/github.com/DeedleFake/p9/proto/client.go (2)
90-97: Read error loop doesn't exit on persistent failures.In the vendored
readergoroutine, whenc.p.Receivereturns an error that's neither EOF nor context cancellation, the codecontinues, potentially looping forever on persistent connection errors.This is vendored from
github.com/DeedleFake/p9and should be fixed upstream.
138-145: Tag allocator can return reserved NoTag value.The tag allocation logic doesn't prevent returning
NoTag(0xFFFF) when the counter wraps around. This violates the protocol's reserved tag semantics and could cause message routing failures.This is vendored code from
github.com/DeedleFake/p9and must be fixed upstream.vendor/github.com/DeedleFake/p9/proto/proto.go (2)
101-108: Dead code: unreachable error check.Lines 103-105 check
if err != nilinside at == nilblock, buterris never assigned in that path. This is unreachable dead code.This is vendored from
github.com/DeedleFake/p9and should be cleaned up upstream.
85-94: LimitedReader may allow over-reads.The
LimitedReaderis set to readsizebytes (line 91), butsizeincludes the 4-byte length header already consumed. The reader should limit tosize - 4to prevent reading beyond the message payload.Additionally, no guard against
size < 4exists, which could cause underflow.This is vendored from
github.com/DeedleFake/p9and must be fixed upstream.vendor/github.com/DeedleFake/p9/fs.go (7)
499-512: Directory read offset deviates from 9P specSpec requires offset 0 or previous offset + previous read length. Current code ignores non‑zero offsets.
Suggestion:
- Track lastDirOff and lastDirCount per FID; validate msg.Offset.
- Serve data slices using the offset into the serialized dir buffer rather than ignoring it.
179-181: Avoid unsafe string-to-bytes in QID hashingUse a safe conversion; unsafe here isn’t needed.
- sum := sha256.Sum256(unsafe.Slice(unsafe.StringData(p), len(p))) + sum := sha256.Sum256([]byte(p)) path := binary.LittleEndian.Uint64(sum[:])Also remove the now-unused unsafe import at the top:
- "unsafe"
244-249: Tflush handlingReturning Rerror breaks clients that send flush. Either return Rflush or implement cancellation; minimally, acknowledge flush.
func (h *fsHandler) flush(msg *Tflush) any { - // TODO: Implement this. - return &Rerror{ - Ename: "flush is not supported", - } + // Best-effort acknowledgement; cancellation not guaranteed. + return &Rflush{} }
536-537: Writes must use exclusive lock, not RLockRLock during WriteAt can race with other ops and corrupt state.
- file.RLock() // Somewhat ironic that this doesn't require a - defer file.RUnlock() // full lock like read() does. + file.Lock() + defer file.Unlock()
566-574: Clunk uses RLock while mutating/closingClose mutates state; use exclusive lock.
- file.RLock() - defer file.RUnlock() + file.Lock() + defer file.Unlock() @@ - err := file.file.Close() + err := file.file.Close()
583-607: remove: don’t hold file lock across clunk; fix error check; snapshot before removeCall clunk without the file lock; check for protocol Rerror; then remove via snapshot of path/attachment.
func (h *fsHandler) remove(msg *Tremove) any { file, ok := h.getFile(msg.FID, false) if !ok { return &Rerror{ Ename: "unknown FID", } } - file.RLock() - defer file.RUnlock() - - rsp := h.clunk(&Tclunk{ - FID: msg.FID, - }) - if _, ok := rsp.(error); ok { - return rsp - } - - err := file.a.Remove(file.path) + // Snapshot without holding lock across clunk/remove. + file.RLock() + p := file.path + a := file.a + file.RUnlock() + + rsp := h.clunk(&Tclunk{FID: msg.FID}) + if rer, ok := rsp.(*Rerror); ok { + return rer + } + + err := a.Remove(p) if err != nil { return &Rerror{ Ename: err.Error(), } } return &Rremove{} }
719-729: Close should lock per-file and null the handlePrevent races with in-flight ops and double-closes.
func (h *fsHandler) Close() error { h.fids.Range(func(k, v any) 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.String; use string(buf)This conversion doesn’t need unsafe and may surprise maintainers/tools.
func (m FileMode) String() string { @@ - return unsafe.String(unsafe.SliceData(buf), len(buf)) + return string(buf) }Then drop the unused unsafe import in this file.
vendor/github.com/DeedleFake/p9/proto/server.go (2)
70-77: Missing continue after read errorOn read error (non-EOF), you log but still process tmsg/tag, which may be invalid. Skip the iteration.
tmsg, tag, err := p.Receive(c, msize) if err != nil { if err == io.EOF { return } log.Printf("Error reading message: %v", err) + continue }
79-99: Goroutine closes over loop vars tag/tmsgCopy tag and tmsg into locals before launching; otherwise values can race/mismatch when mode becomes async.
- 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) } })
🧹 Nitpick comments (1)
vendor/github.com/DeedleFake/p9/fs.go (1)
529-555: Optional: enforce write size vs msize for symmetry with readYou check read counts against msize but not writes. Clients shouldn’t be able to send oversized writes, but validating count improves robustness and error messages.
func (h *fsHandler) write(msg *Twrite) any { + if h.largeCount(uint32(len(msg.Data))) { + return &Rerror{Ename: "write too large"} + }
📜 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 due to trivial changes (1)
- vendor/github.com/DeedleFake/p9/doc.go
🚧 Files skipped from review as they are similar to previous changes (19)
- pkg/crc/constants/constants.go
- vendor/github.com/DeedleFake/p9/LICENSE
- vendor/github.com/DeedleFake/p9/dir_darwin.go
- go.mod
- vendor/github.com/DeedleFake/p9/addr_unix.go
- vendor/github.com/DeedleFake/p9/dir_plan9.go
- cmd/crc/cmd/daemon.go
- vendor/github.com/DeedleFake/p9/dir_other.go
- vendor/github.com/DeedleFake/p9/encoding.go
- pkg/crc/machine/libhvee/driver_windows.go
- vendor/modules.txt
- pkg/crc/machine/start.go
- vendor/github.com/DeedleFake/p9/addr.go
- vendor/github.com/DeedleFake/p9/dir_windows.go
- vendor/github.com/DeedleFake/p9/dir.go
- vendor/github.com/DeedleFake/p9/p9.go
- vendor/github.com/DeedleFake/p9/addr_other.go
- vendor/github.com/DeedleFake/p9/internal/util/util.go
- vendor/github.com/DeedleFake/p9/remote.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 (12)
vendor/github.com/DeedleFake/p9/internal/debug/debug.go (1)
vendor/github.com/DeedleFake/p9/internal/debug/nodebug.go (1)
Log(6-6)
vendor/github.com/DeedleFake/p9/internal/debug/nodebug.go (1)
vendor/github.com/DeedleFake/p9/internal/debug/debug.go (1)
Log(11-13)
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/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/proto/client.go (4)
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(11-13)
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/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 (1)
MessageHandler(135-139)vendor/github.com/DeedleFake/p9/internal/debug/debug.go (1)
Log(11-13)vendor/github.com/DeedleFake/p9/internal/debug/nodebug.go (1)
Log(6-6)vendor/github.com/DeedleFake/p9/msg.go (26)
Tversion(79-82)Tversion(84-84)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/proto/proto.go (3)
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)
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/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/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/dir_linux.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)
🪛 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). (18)
- GitHub Check: build-qe (darwin, arm64)
- GitHub Check: Run OKD bundle with crc (1.24)
- GitHub Check: build-qe (linux, arm64)
- GitHub Check: verify-devcontainer
- GitHub Check: build-qe (linux, amd64)
- GitHub Check: build-qe (darwin, amd64)
- GitHub Check: build-qe (windows, amd64)
- GitHub Check: build-installer (windows-2022, 1.24)
- GitHub Check: build (macOS-13, 1.24)
- GitHub Check: build (ubuntu-latest, 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: build (ubuntu-22.04, 1.24)
- GitHub Check: build (windows-2022, 1.24)
- GitHub Check: build (macOS-14, 1.24)
- GitHub Check: build (macOS-13, 1.24)
- GitHub Check: Konflux kflux-prd-rh02 / crc-binary-on-pull-request
🔇 Additional comments (4)
vendor/github.com/DeedleFake/p9/internal/debug/debug.go (1)
1-13: LGTM! Standard build-tag-gated debug logging.This vendored debug implementation follows a common Go pattern: the
p9debugbuild tag gates actual logging to stderr, while the no-op variant innodebug.gois compiled by default. The implementation is clean and appropriate for internal debug utilities.vendor/github.com/DeedleFake/p9/internal/debug/nodebug.go (1)
1-6: LGTM! Complementary no-op stub.The
!p9debugbuild tag ensures this no-op variant is compiled by default, providing zero runtime overhead when debug logging is disabled. The function signature correctly matches the active implementation indebug.go.vendor/github.com/DeedleFake/p9/README.md (1)
1-51: LGTM - vendored documentation.This is documentation from the vendored library. The markdown linting hints are minor formatting issues that should be addressed upstream if at all.
pkg/fileserver/fs9p/server.go (1)
34-58: LGTM - Constructor validates inputs correctly.The
New9pServerconstructor properly validates that:
exposeDiris an absolute pathexposeDirexists and is a directory- Creates a buffered
ErrChanto prevent goroutine blockingThe implementation is solid.
| func (h *fsHandler) wstat(msg *Twstat) any { | ||
| file, ok := h.getFile(msg.FID, false) | ||
| if !ok { | ||
| return &Rerror{ | ||
| Ename: "unknown FID", | ||
| } | ||
| } | ||
| file.RLock() | ||
| defer file.RUnlock() | ||
|
|
||
| changes := StatChanges{ | ||
| DirEntry: msg.Stat.DirEntry(), | ||
| } | ||
|
|
||
| err := file.a.WriteStat(file.path, changes) | ||
| if err != nil { | ||
| return &Rerror{ | ||
| Ename: err.Error(), | ||
| } | ||
| } | ||
|
|
||
| return &Rwstat{} | ||
| } |
There was a problem hiding this comment.
wstat rename: update in-memory path and use exclusive lock
If Name() changes, file.path must be updated to keep the FID consistent; also we’re mutating state, so use Lock.
func (h *fsHandler) wstat(msg *Twstat) any {
file, ok := h.getFile(msg.FID, false)
if !ok {
return &Rerror{
Ename: "unknown FID",
}
}
- file.RLock()
- defer file.RUnlock()
+ file.Lock()
+ defer file.Unlock()
@@
err := file.a.WriteStat(file.path, changes)
if err != nil {
return &Rerror{
Ename: err.Error(),
}
}
+
+ // Update local path on rename.
+ if newName, ok := changes.Name(); ok {
+ file.path = path.Join(path.Dir(file.path), newName)
+ }
return &Rwstat{}
}| func handleMessages(c net.Conn, p Proto, handler MessageHandler) { | ||
| var setter sync.Once | ||
|
|
||
| var msize uint32 | ||
| mode := func(f func()) { | ||
| f() | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Concurrent Send on a shared net.Conn — serialize or verify
Once mode switches to async, multiple goroutines may call p.Send on the same net.Conn, which can interleave writes unless p.Send is internally synchronized. Either guard sends with a per-connection mutex or confirm Proto.Send is concurrency-safe.
Optional serialization here:
func handleMessages(c net.Conn, p Proto, handler MessageHandler) {
var setter sync.Once
+ var sendMu sync.Mutex
var msize uint32
mode := func(f func()) {
f()
}
@@
- err := p.Send(c, ctag, rmsg)
+ sendMu.Lock()
+ err := p.Send(c, ctag, rmsg)
+ sendMu.Unlock()
if err != nil {
log.Printf("Error writing message: %v", err)
}Run to verify Send’s internals:
Also applies to: 94-97
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -nP '(?m)^func\s*\(\s*\w+\s+\*?Proto\s*\)\s*Send\s*\(' vendor/github.com/DeedleFake/p9/proto || true
rg -nP '(?m)\btype\s+Proto\b' vendor/github.com/DeedleFake/p9/proto || true
rg -nP '(?m)\bsendMu\b|\bmutex\b' vendor/github.com/DeedleFake/p9/proto || trueLength of output: 188
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Get Proto struct definition
echo "=== Proto struct definition ==="
sed -n '38,100p' vendor/github.com/DeedleFake/p9/proto/proto.go
echo ""
echo "=== Send method implementation ==="
sed -n '130,150p' vendor/github.com/DeedleFake/p9/proto/proto.go
echo ""
echo "=== handleMessages function around lines 61-67 ==="
sed -n '55,100p' vendor/github.com/DeedleFake/p9/proto/server.go
echo ""
echo "=== handleMessages function around lines 94-97 ==="
sed -n '85,110p' vendor/github.com/DeedleFake/p9/proto/server.goLength of output: 3680
Concurrent sends on shared net.Conn require synchronization — add per-connection mutex
Verification confirms the race condition is real:
Protostruct has no mutex field andSend()method has no internal locking- After
setter.Do()at lines 94-97 in server.go,modeswitches from synchronous to async (go f()) - Multiple goroutines then call
p.Send(c, tag, rmsg)on the same connectionc Proto.Send()performs an unsynchronizedio.Copy(w, buf)write to the connection, causing concurrent writes to interleave
Add var sendMu sync.Mutex guard around p.Send() calls as suggested:
func handleMessages(c net.Conn, p Proto, handler MessageHandler) {
var setter sync.Once
+ var sendMu sync.Mutex
var msize uint32
mode := func(f func()) {
f()
}
@@
- err := p.Send(c, tag, rmsg)
+ sendMu.Lock()
+ err := p.Send(c, tag, rmsg)
+ sendMu.Unlock()
if err != nil {
log.Printf("Error writing message: %v", err)
}Also apply at lines 94-97 context (same p.Send() call within the async goroutine).
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In vendor/github.com/DeedleFake/p9/proto/server.go around lines 61-67,
concurrent goroutines call p.Send() on the same net.Conn causing unsynchronized
writes; add a per-connection mutex (e.g., declare var sendMu sync.Mutex near the
top of handleMessages) and wrap every p.Send(c, ...) invocation with
sendMu.Lock() / sendMu.Unlock(), including the p.Send() in the async goroutine
executed after setter.Do() at lines ~94-97, to ensure all writes to the shared
connection are serialized.
|
@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. |
4 participants
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