Add 9P and remove SMB file sharing for Windows - #4879
Conversation
WalkthroughMigrates CRC's shared directory mechanism from Windows SMB to cross-platform 9P protocol. Introduces a Go-based 9P server with hvsock listener support for Windows and TCP fallback. Replaces SMB share creation, configuration management, and mount logic throughout the codebase with 9P equivalents. Adds p9 library dependency. Changes
Sequence DiagramsequenceDiagram
participant Daemon as CRC Daemon
participant Server as 9P Server
participant Hvsock as Hvsock Listener
participant TCP as TCP Listener
participant VM as Hyper-V VM
participant Client as Client
Daemon->>Server: New9pServer(hvsockListener, homeDir)
Daemon->>Hvsock: Create hvsock listener via GetHvsockListener()
Daemon->>Server: Start()
Server->>Hvsock: proto.Serve(listener, fsConnHandler)
activate Server
Server->>Server: WaitForError() goroutine
deactivate Server
Daemon->>TCP: Create TCP listener (GatewayIP:564)
Daemon->>Server: New9pServer(tcpListener, homeDir)
Daemon->>Server: Start()
Server->>TCP: proto.Serve(listener, fsConnHandler)
activate Server
Server->>Server: WaitForError() goroutine
deactivate Server
VM->>Hvsock: Attach via 9P
activate Hvsock
Hvsock-->>VM: Mounted (success path)
deactivate Hvsock
alt Hvsock unavailable
VM->>TCP: Fallback to 9P over TCP
activate TCP
TCP-->>VM: Mounted (fallback path)
deactivate TCP
end
Client->>VM: Access mounted directory
VM->>Server: 9P protocol requests (stat, read, etc.)
Server->>Server: Resolve path & filesystem ops
Server-->>VM: 9P responses
VM-->>Client: File operations complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Areas requiring extra attention:
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 23
🧹 Nitpick comments (25)
vendor/github.com/DeedleFake/p9/addr_other.go (2)
11-16: Doc comment misleading for non-Unix build tagThis file is compiled on non-Linux/non-Darwin platforms, yet the comment explains Unix-like behavior. Clarify the comment to avoid confusion.
Apply this diff:
-// current namespace. On Unix-like systems, this is -// /tmp/ns.$USER.$DISPLAY. +// current namespace. On Unix-like systems, the path is typically +// /tmp/ns.$USER.$DISPLAY, but for non-Unix platforms this implementation +// uses os.TempDir() as the base.
1-1: Add modern build constraint for forward-compatibilityConsider adding the //go:build form alongside the legacy // +build for clarity and future-proofing. This mirrors Go's recommended syntax since 1.17.
Apply this diff:
+//go:build !linux && !darwin // +build !linux,!darwinvendor/github.com/DeedleFake/p9/internal/debug/debug.go (2)
1-1: Add //go:build for consistencyAdd the modern //go:build constraint alongside the legacy form.
Apply this diff:
+//go:build p9debug // +build p9debug
10-12: Optional: append newline to logsCurrent logging will not append a newline, which can jumble outputs in stderr across multiple calls. If callers don't consistently include "\n", consider appending one here.
Apply this diff:
-func Log(str string, args ...interface{}) { - fmt.Fprintf(os.Stderr, str, args...) -} +func Log(str string, args ...interface{}) { + if len(args) == 0 && len(str) > 0 && str[len(str)-1] == '\n' { + fmt.Fprint(os.Stderr, str) + return + } + fmt.Fprintf(os.Stderr, str+"\n", args...) +}vendor/github.com/DeedleFake/p9/README.md (1)
1-52: Vendor docs violate markdownlint rules; exclude vendor from lintingmarkdownlint is flagging heading styles and hard tabs in vendored README. Vendor content should generally be excluded from linters to avoid false positives and churn.
Consider updating your markdownlint configuration to ignore vendor/:
Example .markdownlint-cli2.yaml at repo root:
- ignores:
- "vendor/**"
If you prefer to keep linting vendor content, we can convert headings to setext style and replace tabs with spaces in this file.
vendor/github.com/DeedleFake/p9/addr_unix.go (1)
1-1: Add //go:build for clarityAdd the modern build constraint along with the legacy tag.
Apply this diff:
+//go:build linux || darwin // +build linux darwinvendor/github.com/DeedleFake/p9/proto/encoding.go (2)
82-84: Avoid encoding uintptr in a wire protocolIncluding reflect.Uintptr introduces arch-dependent sizes. It's safer to reject uintptr or normalize it to a fixed width.
Apply this diff to drop uintptr support:
- case reflect.Uint8, reflect.Uint16, reflect.Uint32, reflect.Uint64, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64, reflect.Uintptr: + case reflect.Uint8, reflect.Uint16, reflect.Uint32, reflect.Uint64, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64:
171-171: Prefer safe []byte->string conversion over unsafe.PointerThe unsafe conversion ties the string lifetime to a transient []byte backing array and relies on runtime internals. For robustness, use string(buf). If performance is a concern, justify with benchmarks and limit to hot paths.
Apply this diff:
- v.SetString(*(*string)(unsafe.Pointer(&buf))) + v.SetString(string(buf))pkg/crc/constants/constants.go (1)
60-60: Validate msize across client implementationsConfirm that 1MiB msize works with the kernel 9p client(s) you target (Linux/macOS guests) and matches mount-time options. If necessary, consider making it configurable to ease tuning for performance/path MTU constraints.
vendor/github.com/DeedleFake/p9/dir_other.go (1)
1-1: Add go:build constraint for Go 1.17+ compatibilityThe legacy // +build form is still supported, but pairing it with //go:build improves tooling compatibility.
Apply this diff:
+//go:build !linux && !darwin && !plan9 && !windows // +build !linux,!darwin,!plan9,!windowsvendor/github.com/DeedleFake/p9/encoding.go (1)
16-30: Graceful EOF handling is good; consider treating ErrUnexpectedEOF as end-of-stream too.proto.Read may return io.ErrUnexpectedEOF on truncated directory streams. If your intent is “read until the stream ends,” it’s reasonable to treat both EOF and ErrUnexpectedEOF as a normal termination and return the accumulated entries.
Apply within this range:
- err := proto.Read(r, &stat) - if err != nil { - if err == io.EOF { - err = nil - } - return entries, err - } + err := proto.Read(r, &stat) + if err != nil { + if errors.Is(err, io.EOF) || errors.Is(err, io.ErrUnexpectedEOF) { + return entries, nil + } + return entries, err + }Additionally, add the import:
// add to imports "errors"vendor/github.com/DeedleFake/p9/internal/util/util.go (3)
8-13: Fix misleading comment about size semantics of N.io.LimitedReader already uses int64, which is larger than uint32 on all architectures. The current comment suggests uint32 “allows larger sizes,” which is inaccurate. Recommend clarifying the rationale and the cap.
-// LimitedReader is a reimplementation of io.LimitedReader with two -// main differences: -// -// * N is a uint32, allowing for larger sizes on 32-bit systems. -// * A custom error can be returned if N becomes zero. +// LimitedReader is similar to io.LimitedReader with two differences: +// +// * N is a uint32 (io.LimitedReader uses int64). This keeps the type small and +// consistent across architectures but caps the readable size at 4 GiB. +// * A custom error can be returned when the limit is exhausted.
27-31: Use equality check for unsigned counter.N is uint32; use N == 0 instead of N <= 0 for clarity.
- if lr.N <= 0 { + if lr.N == 0 { return 0, lr.err() }
41-44: Align doc with implementation for Errorf.The comment mentions “nil or io.EOF,” but the code only returns io.EOF directly. Either expand the implementation or narrow the comment. Suggest tightening the comment.
-// Errorf is a variant of fmt.Errorf that returns an error being -// wrapped directly if it is one of a number of specific values, such -// as nil or io.EOF. +// Errorf is a variant of fmt.Errorf that returns io.EOF directly if it +// appears among the arguments; otherwise it formats the error.vendor/github.com/DeedleFake/p9/dir_plan9.go (2)
10-31: Populate QID fields in DirEntry when available.When syscall.Dir is present, you can also fill DirEntry.Path and DirEntry.Version from sys.Qid to avoid an extra stat/QID lookup elsewhere.
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: Clarify error message to include the concrete type.Improve diagnosability by naming the expected concrete type.
- 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/dir_linux.go (2)
52-55: Clarify error message to include the concrete type.Improves diagnosability.
- if sys == nil { - return QID{}, errors.New("failed to get QID: FileInfo was not Stat_t") - } + if sys == nil { + return QID{}, errors.New("failed to get QID: FileInfo.Sys() was not *syscall.Stat_t") + }
23-33: Optional: Fallback to numeric UID/GID when lookups fail.LookupId/LookupGroupId can fail or be slow; consider falling back to numeric strings to retain information.
var uname string uid, err := user.LookupId(strconv.FormatUint(uint64(sys.Uid), 10)) if err == nil { uname = uid.Username + } else { + uname = strconv.FormatUint(uint64(sys.Uid), 10) } var gname string gid, err := user.LookupGroupId(strconv.FormatUint(uint64(sys.Gid), 10)) if err == nil { gname = gid.Name + } else { + gname = strconv.FormatUint(uint64(sys.Gid), 10) }vendor/github.com/DeedleFake/p9/proto/server.go (1)
80-93: Msizer handling: prevent double size negotiation warning spamThe setter.Do already guards the once-only behavior, but the outer check logs a warning even when the race resolves to first set wins. Consider moving the “already set” warning inside the Do block and only emit if setter wasn’t executed. Optional improvement.
vendor/github.com/DeedleFake/p9/proto/proto.go (1)
101-108: Dead branch on invalid type check.Inside
if t == nil { ... }the nestedif err != nil { return ... }can never trigger;errwasn’t modified after the previous successful read. This is dead code and reduces clarity.- 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/remote.go (2)
127-135: Variable shadowing obscures receiver; rename local for clarity.
file, err := file.walk(p)shadows the method receiver, reducing readability. Use a different name and avoid confusion.- 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("")
323-331: Same shadowing issue in Stat(); rename local.Avoid shadowing the receiver to keep code unambiguous.
- file, err := file.walk(p) + next, err := file.walk(p) if err != nil { return DirEntry{}, err } - defer file.Close() + defer next.Close() - return file.Stat("") + return next.Stat("")vendor/github.com/DeedleFake/p9/fs.go (2)
179-186: Avoid unsafe string-to-byte conversion when hashing QID path.Using
unsafeto convert string to []byte is brittle and can break across Go versions. The allocation saved here is negligible compared to a SHA-256. Prefer a safe conversion.- sum := sha256.Sum256(*(*[]byte)(unsafe.Pointer(&p))) + sum := sha256.Sum256([]byte(p)) path := binary.LittleEndian.Uint64(sum[:])Additionally remove the now-unused
unsafeimport from this file’s imports.
To support import cleanup, apply this change to the import block:// Remove the "unsafe" import from the import list in this file.
446-527: Directory read handling is intentionally spec-relaxed; document client expectations.The offset handling for directory reads is relaxed (offsets other than 0 are ignored). This is acceptable since the implementation returns the full entry list, but client-side code must not rely on precise offset semantics. Consider adding a short doc comment to the public-facing server wiring to set expectations.
vendor/github.com/DeedleFake/p9/stat.go (1)
135-153: Minor: unsafe-to-string conversion in String() is a trade-off.Using unsafe to avoid allocation is fine in vendored perf-sensitive code, but note it ties to Go’s internal string representation. If we ever fork this lib, consider a safe
string(buf)for maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these settings in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (43)
cmd/crc/cmd/daemon.go(4 hunks)cmd/crc/cmd/start.go(0 hunks)go.mod(1 hunks)packaging/windows/product.wxs.template(0 hunks)pkg/crc/config/settings.go(1 hunks)pkg/crc/constants/constants.go(1 hunks)pkg/crc/machine/driver.go(0 hunks)pkg/crc/machine/driver_darwin.go(0 hunks)pkg/crc/machine/driver_linux.go(0 hunks)pkg/crc/machine/driver_windows.go(0 hunks)pkg/crc/machine/libhvee/driver_windows.go(2 hunks)pkg/crc/machine/start.go(2 hunks)pkg/drivers/libhvee/libhvee_windows.go(0 hunks)pkg/drivers/libhvee/powershell_windows.go(0 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)
💤 Files with no reviewable changes (8)
- pkg/crc/machine/driver_darwin.go
- pkg/crc/machine/driver_windows.go
- cmd/crc/cmd/start.go
- pkg/drivers/libhvee/libhvee_windows.go
- pkg/drivers/libhvee/powershell_windows.go
- pkg/crc/machine/driver_linux.go
- pkg/crc/machine/driver.go
- packaging/windows/product.wxs.template
🧰 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:
vendor/github.com/DeedleFake/p9/README.mdcmd/crc/cmd/daemon.gopkg/fileserver/fs9p/server.govendor/github.com/DeedleFake/p9/fs.gopkg/fileserver/fs9p/shares.go
🧬 Code Graph Analysis (28)
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)
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/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/addr_unix.go (1)
vendor/github.com/DeedleFake/p9/addr_other.go (1)
NamespaceDir(17-29)
vendor/github.com/DeedleFake/p9/addr_other.go (1)
vendor/github.com/DeedleFake/p9/addr_unix.go (1)
NamespaceDir(17-29)
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/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/internal/debug/nodebug.go (1)
vendor/github.com/DeedleFake/p9/internal/debug/debug.go (1)
Log(10-12)
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/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/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)
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/proto/encoding.go (1)
vendor/github.com/DeedleFake/p9/internal/util/util.go (1)
Errorf(44-52)
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)
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(25-56)Mount9p(12-15)
vendor/github.com/DeedleFake/p9/remote.go (6)
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)
pkg/crc/config/settings.go (2)
pkg/crc/config/validations.go (1)
ValidateBool(19-25)pkg/crc/config/callbacks.go (1)
SuccessfullyApplied(36-38)
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(60-60)
vendor/github.com/DeedleFake/p9/p9.go (1)
vendor/github.com/DeedleFake/p9/stat.go (1)
FileMode(23-23)
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/nodebug.go (1)
Log(5-5)vendor/github.com/DeedleFake/p9/internal/debug/debug.go (1)
Log(10-12)vendor/github.com/DeedleFake/p9/msg.go (2)
Tversion(79-82)Tversion(84-84)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/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 (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/shares.go (1)
pkg/fileserver/fs9p/server.go (2)
Server(15-22)New9pServer(26-62)
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 (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/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 (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/nodebug.go (1)
Log(5-5)vendor/github.com/DeedleFake/p9/internal/debug/debug.go (1)
Log(10-12)vendor/github.com/DeedleFake/p9/p9.go (1)
NoTag(15-15)
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)
🪛 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). (6)
- GitHub Check: Konflux kflux-prd-rh02 / crc-binary-on-pull-request
- GitHub Check: build (macOS-14, 1.23)
- GitHub Check: Run OKD bundle with crc (1.23)
- GitHub Check: build (ubuntu-latest, 1.23)
- GitHub Check: build (windows-2022, 1.23)
- GitHub Check: build (macOS-13, 1.23)
🔇 Additional comments (26)
vendor/github.com/DeedleFake/p9/LICENSE (1)
1-21: MIT license file correctly included for vendored dependency.License content and attribution look correct for github.com/DeedleFake/p9.
vendor/modules.txt (1)
13-18: Vendoring metadata for p9 v0.6.12 looks consistent.Entries for root and subpackages are present and marked explicit.
vendor/github.com/DeedleFake/p9/internal/debug/nodebug.go (1)
1-5: No-op debug build variant is fine.Build tag and signature match the debug counterpart; safe to vendor as-is.
pkg/crc/machine/libhvee/driver_windows.go (2)
4-4: fmt import is justified by dynamic 9P tag generation.No issues.
39-45: No change needed — Windows uses TCP 9p, vsock support is disabled and 9p server does not rely on "dir%d" tagsShort summary: I verified that drivers set Tag = fmt.Sprintf("dir%d", i) client-side, but the fs9p server exposes a directory per net.Listener (no tag mapping) and vsock shares are disabled. Windows path already uses TCP/9p, so the original tag/vsock concern is not applicable.
Files to note:
- pkg/crc/machine/libhvee/driver_windows.go — sets Tag: fmt.Sprintf("dir%d", i) and Type: "9p"
- pkg/fileserver/fs9p/shares.go — StartVsockShares returns nil (vsock support removed); StartShares uses New9pServer(listener, path)
- pkg/fileserver/fs9p/server.go — New9pServer serves exposeDir on the provided net.Listener (no tag mapping)
- cmd/crc/cmd/daemon.go — creates a TCP listener and calls fs9p.StartShares(...)
- pkg/crc/machine/start.go — configureSharedDirs mounts "9p" via "9pfs 192.168.127.1" (TCP), not vsock
Conclusion: the original review suggestion to align 9p share tags and avoid vsock assumptions is incorrect for this codebase — Windows is already using the TCP/GVProxy 9p path and the server is listener-bound rather than tag-mapped.
Likely an incorrect or invalid review comment.
pkg/crc/machine/start.go (1)
209-211: LGTM: explanatory comment is accurateThe note about rpc.ServerError wrapping and the limitation of errors.Is() is correct and useful context here.
pkg/crc/config/settings.go (1)
87-88: Clarify EnableSharedDirs help text; do NOT enforce HostNetworkAccessShort summary: I verified the implementation — the 9P server is started on the CRC virtual-network gateway and the guest mounts that gateway address, so the shared-dir feature is served via the CRC virtual network (not by mounting host /). Therefore the proposed validator that forces HostNetworkAccess=true for user networking is incorrect: HostNetworkAccess controls NAT to the host loopback and is not required for the 9P server the daemon creates.
Files to note:
- pkg/crc/config/settings.go — AddSetting for EnableSharedDirs (update help text here)
- cmd/crc/cmd/daemon.go — vn.Listen on configuration.GatewayIP and fs9p.StartShares (9P server is bound to the virtual-network gateway)
- pkg/crc/machine/start.go — configureSharedDirs uses "9pfs 192.168.127.1" (guest mounts the gateway, mountpoint is not '/')
- pkg/fileserver/fs9p/server.go & shares.go — New9pServer / StartShares bind to the provided listener
- pkg/crc/constants/constants.go — Plan9Port = 564 (9P port)
Proposed help-text change (apply to pkg/crc/config/settings.go where EnableSharedDirs is registered):
- cfg.AddSetting(EnableSharedDirs, true, ValidateBool, SuccessfullyApplied, - "Mounts host's home directory at '/' in the CRC VM (true/false, default: true)") + cfg.AddSetting(EnableSharedDirs, true, ValidateBool, SuccessfullyApplied, + "Expose the host's home directory to the CRC VM via 9P (true/false, default: true). The share is served via the CRC virtual-network gateway (e.g. 192.168.127.1) and mounted inside the VM via 9P (not mounted at '/')")Validator guidance:
- Do not add a validator that requires HostNetworkAccess=true for NetworkMode=user. Verified evidence: daemon binds the 9P server to the CRC virtual network gateway (cmd/crc/cmd/daemon.go -> vn.Listen on configuration.GatewayIP) and the VM mounts that gateway address (pkg/crc/machine/start.go uses "9pfs 192.168.127.1"). HostNetworkAccess is a separate toggle that maps a virtual host IP to the host loopback and is not necessary for the 9P share.
Likely an incorrect or invalid review comment.
vendor/github.com/DeedleFake/p9/doc.go (1)
1-63: LGTM: Clear, useful package-level docsThe overview and examples set the right expectations for both client and server APIs. No code changes; nothing else to flag.
vendor/github.com/DeedleFake/p9/proto/encoding.go (1)
147-159: No change required — reads are already capped with LimitedReaderproto.Receive reads the message size, enforces msize, then wraps the connection with util.LimitedReader (N = size, E = ErrLargeMessage) and uses that reader for subsequent decoding, so decoder.decode runs against a length-limited reader.
- vendor/github.com/DeedleFake/p9/proto/proto.go: Receive(...) — reads size, checks msize, constructs lr := &util.LimitedReader{R: r, N: size, E: ErrLargeMessage} and calls Read(lr, ...)
- vendor/github.com/DeedleFake/p9/internal/util/util.go: LimitedReader implementation that returns the configured error when the limit is reached
- vendor/github.com/DeedleFake/p9/proto/encoding.go: decoder.decode handles slice lengths/allocations — safe when invoked via Receive
vendor/github.com/DeedleFake/p9/addr.go (1)
10-66: LGTM: ParseAddr covers common 9P forms and pseudo-port mappingSolid handling of “$namespace”, Unix sockets, host:port (including 9p/9fs), and proto!addr[!port] forms. No functional issues spotted.
vendor/github.com/DeedleFake/p9/p9.go (2)
21-39: LGTM: Open mode/flag constants consistent with 9P semanticsThe bit layout and the iota math are correct; no concerns.
41-66: LGTM: QID/QIDType and FileMode conversion look correctQIDType->FileMode shift aligns with 9P’s mode top bits. No issues.
pkg/crc/constants/constants.go (1)
59-60: Plan 9 constants addition looks goodUsing the standard 9P port (564) and a generous 1MiB msize is sensible for the initial integration.
cmd/crc/cmd/daemon.go (3)
27-27: Importing fs9p for 9P shares is appropriateThe new import aligns with the 9P home directory sharing introduced below.
181-185: Good: use net.JoinHostPort for listener addressThis is safer (IPv6-aware) and avoids string formatting pitfalls.
197-211: Good: consistent JoinHostPort usage for network listenerSame benefits here; consistent and correct.
vendor/github.com/DeedleFake/p9/dir_other.go (1)
7-14: LGTM: sane fallback entry mappingFor non-listed OSes, mapping mode/mtime/size/name is correct and minimal.
vendor/github.com/DeedleFake/p9/dir_windows.go (1)
9-27: Windows DirEntry mapping looks correct
- Safe type assertion on fi.Sys() and proper fallback when sys is nil.
- Using Filetime.Nanoseconds() with time.Unix(0, ...) is the idiomatic conversion.
vendor/github.com/DeedleFake/p9/dir_darwin.go (1)
46-62: Darwin QID generation approach is soundUsing inode for Path and deriving QID type from mode mirrors Linux behavior and is appropriate. Error on missing Stat_t keeps semantics clear.
vendor/github.com/DeedleFake/p9/encoding.go (1)
33-42: LGTM: WriteDir is straightforward and correct.Looped encode via proto.Write is clear and returns on first error as expected.
vendor/github.com/DeedleFake/p9/proto/client.go (2)
69-75: LGTM: clean shutdown pathClose() cancels the coordination goroutines and closes the underlying connection. This aligns with the reader/coord loop design.
176-208: Solid tagged/untagged messaging coordinationThe Send/reader/coord interplay correctly handles both tagged messages and the special untagged case (P9NoTag), with backpressure and cancellation paths.
vendor/github.com/DeedleFake/p9/dir.go (1)
18-21: Consider hardening against traversal in path resolutionDir.path simply joins the base with the provided p. If upstream always supplies normalized, component-wise paths (as expected by the 9P Walk semantics), this is fine. If not guaranteed, normalize and reject any attempt to escape the base (e.g., by resolving and verifying that the result has the base as a prefix).
Would you like me to add a safe-join helper that rejects absolute paths and any result escaping the base?
vendor/github.com/DeedleFake/p9/stat.go (1)
191-216: Stat encoding/decoding looks correct and spec-compliant.The size-prefix and LimitedReader enforcement align with 9P stat encoding, and error propagation is sound.
vendor/github.com/DeedleFake/p9/msg.go (2)
207-233: Rstat length-prefixing matches Stat.P9Encode; decode bounds are correct.The extra
+2accounts for Stat’s internal size field, and the LimitedReader bounds prevent overreads. This aligns with the 9P2000 spec.
44-72: Protocol registry is comprehensive and type-safe mapping looks good.The reflect-based registry setup matches the declared message types and provides a solid base for dispatch.
cfergeau
left a comment
cfergeau
left a comment
There was a problem hiding this comment.
Only took a quick look, but it’s unexpected to see the 9p code being added in the first commits before being disabled, and then to have the smb code removal.
Regarding the 9p code, some of the comments of coderabbit.ai (including in the vendor’ed code) deserve a closer look.
I based this branch on the 9p branch to avoid conflicts. This PR is expected to be merged as soon as 9p support is merged, so the 9p code will already be there.
I'll look at comments on my parts of the code, but what can be done about the vendored code? |
If they are actual issues, in particular issues with security impacts, if it’s not too hard to fix them, I’d file PRs upstream, this way we’ll get the fixes in our codebase when they are merged. |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packaging/windows/product.wxs.template (1)
95-103: Remove stray '~' in MSI condition stringspackaging/windows/product.wxs.template — occurrences of REMOVE~="ALL" at lines 96, 97, 98, 99, 100, 102. WiX conditions must be REMOVE="ALL" (no '
'); replace all REMOVE="ALL" with REMOVE="ALL".
♻️ Duplicate comments (11)
vendor/github.com/DeedleFake/p9/dir_darwin.go (1)
35-43: Fix Darwin build: Timespec.Unix() does not exist; use Sec/Nsec fieldsThis won’t compile on darwin; syscall.Timespec has Sec/Nsec, not Unix().
Please also send this fix upstream to DeedleFake/p9 so we don’t carry a long‑term vendor delta.
return DirEntry{ FileMode: ModeFromOS(fi.Mode()), - ATime: time.Unix(sys.Atimespec.Unix()), + ATime: time.Unix(int64(sys.Atimespec.Sec), int64(sys.Atimespec.Nsec)), MTime: fi.ModTime(), Length: uint64(fi.Size()), EntryName: fi.Name(), UID: uname, GID: gname, }#!/bin/bash # Confirm there are no other invalid Timespec.Unix() calls. rg -nP 'Atimespec\.Unix\(' -Spkg/crc/machine/libhvee/driver_windows.go (1)
39-45: Deterministic 9P tag mapping; keep in sync with server (duplicate)Sort the list before assigning tags to keep tag->share stable across runs and avoid drift with the 9P server’s aname. Also consider centralizing the tag format constant so both sides stay aligned.
@@ -import ( - "fmt" - "path/filepath" - "strings" -) +import ( + "fmt" + "path/filepath" + "strings" + "sort" +) @@ - for i, dir := range machineConfig.SharedDirs { + dirs := append([]string(nil), machineConfig.SharedDirs...) + sort.Strings(dirs) + for i, dir := range dirs { sharedDir := drivers.SharedDir{ Source: dir, Target: convertToUnixPath(dir), Tag: fmt.Sprintf("dir%d", i), Type: "9p", }cmd/crc/cmd/daemon.go (1)
253-266: Don’t drop 9P server handles; defer Stop() for clean shutdown (duplicate)Keep the returned servers and stop them to avoid goroutine/listener leaks.
// 9p home directory sharing if runtime.GOOS == "windows" { - if _, err := fs9p.StartHvsockShares([]fs9p.HvsockMount9p{{Path: constants.GetHomeDir(), HvsockGUID: constants.Plan9HvsockGUID}}); err != nil { + if servers, err := fs9p.StartHvsockShares([]fs9p.HvsockMount9p{{Path: constants.GetHomeDir(), 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))) if err != nil { return err } - if _, err := fs9p.StartShares([]fs9p.Mount9p{{Listener: listener9p, Path: constants.GetHomeDir()}}); err != nil { + if serversTCP, err := fs9p.StartShares([]fs9p.Mount9p{{Listener: listener9p, Path: constants.GetHomeDir()}}); err != nil { return err + } else { + defer func() { + for _, s := range serversTCP { + if stopErr := s.Stop(); stopErr != nil { + logging.Errorf("Error stopping 9p server: %v", stopErr) + } + } + }() } + } else { + defer func() { + for _, s := range servers { + if stopErr := s.Stop(); stopErr != nil { + logging.Errorf("Error stopping 9p server: %v", stopErr) + } + } + }() } }pkg/crc/machine/start.go (1)
244-256: 9P mount should be privileged, idempotent, and explicit; avoid brittle helperMounting requires root; using sshRunner.Run will likely fail. Also, add mountpoint check and use explicit 9P mount with options and tag for TCP fallback. Keep the hvsock attempt if needed, but run it privileged.
case "9p": - // change owner to core user to allow mounting to it as a non-root user - if _, _, err := sshRunner.RunPrivileged("Changing owner of mount directory", "chown core:core", mount.Target); err != nil { - return err - } - if _, _, err := sshRunner.Run("9pfs -V -p", fmt.Sprintf("%d", constants.Plan9HvsockPort), mount.Target); err != nil { - logging.Warnf("Failed to connect to 9p server over hvsock: %v", err) - logging.Warnf("Falling back to 9p over TCP") - if _, _, err := sshRunner.Run("9pfs 192.168.127.1", mount.Target); err != nil { - return err - } - } + // idempotency + if _, _, err := sshRunner.RunPrivileged("Check if already mounted", "mountpoint", "-q", mount.Target); err == nil { + logging.Debugf("Already mounted, skipping: %s", mount.Target) + continue + } + // change owner to core user to allow non-root access after mounting + if _, _, err := sshRunner.RunPrivileged("Changing owner of mount directory", "chown", "core:core", mount.Target); err != nil { + return err + } + // Try hvsock via helper (if present) with elevated privileges + if _, _, err := sshRunner.RunPrivileged("Mounting 9p over hvsock", "9pfs", "-V", "-p", fmt.Sprintf("%d", constants.Plan9HvsockPort), mount.Target); err != nil { + logging.Warnf("Failed to connect to 9p server over hvsock: %v", err) + logging.Warnf("Falling back to 9p over TCP") + opts := fmt.Sprintf("trans=tcp,port=%d,msize=%d,cache=loose,version=9p2000.L,aname=%s", constants.Plan9TcpPort, constants.Plan9Msize, mount.Tag) + if _, _, err := sshRunner.RunPrivileged( + fmt.Sprintf("Mounting %s (9p tag %s) over TCP", mount.Target, mount.Tag), + "mount", "-t", "9p", "-o", opts, "192.168.127.1", mount.Target, + ); err != nil { + return err + } + }Follow-ups:
- Consider gating the hvsock attempt behind a feature flag until client support stabilizes.
pkg/fileserver/fs9p/shares.go (1)
24-61: Fix loop-variable capture; close listener on per-mount failures; preallocate sliceAvoid capturing loop vars in defer/goroutine, and ensure the current mount’s listener is closed on errors before the defer is installed. Also preallocate the servers slice.
-func StartShares(mounts []Mount9p) (servers []*Server, defErr error) { - servers9p := []*Server{} +func StartShares(mounts []Mount9p) (servers []*Server, defErr error) { + servers = make([]*Server, 0, len(mounts)) 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) + _ = m.Listener.Close() // avoid leaking a listener on constructor failure + return nil, fmt.Errorf("serving directory %s on %s: %w", m.Path, m.Listener.Addr().String(), err) } - err = server.Start() + if err := server.Start(); err != nil { + _ = server.Stop() // close listener if start fails before defer is in place if err != nil { - return nil, fmt.Errorf("starting 9p server for directory %s: %w", m.Path, err) + return nil, fmt.Errorf("starting 9p server for directory %s: %w", m.Path, err) } - } - - servers9p = append(servers9p, server) + } + servers = append(servers, server) - defer func() { + s := server + defer func(s *Server) { if defErr != nil { - if err := server.Stop(); err != nil { + if err := s.Stop(); err != nil { logrus.Errorf("Error stopping 9p server: %v", err) } } - }() + }(s) - serverDir := m.Path + serverDir := m.Path + addr := m.Listener.Addr().String() - go func() { - if err := server.WaitForError(); err != nil { - logrus.Errorf("Error from 9p server on %s for %s: %v", m.Listener.Addr().String(), serverDir, err) + go func(s *Server, address, dir string) { + if err := s.WaitForError(); err != nil { + logrus.Errorf("Error from 9p server on %s for %s: %v", address, dir, 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", address, dir) } - }() + }(s, addr, serverDir) } - return servers9p, nil + return servers, nil }pkg/fileserver/fs9p/server.go (1)
42-51: Buffer ErrChan and treat net.ErrClosed as clean shutdown to avoid leaks/noiseUnbuffered ErrChan can block the server goroutine if nobody reads; also treat listener closure as non-error in WaitForError.
-import ( +import ( + "errors" "fmt" "net" "os" "path/filepath" @@ - errChan := make(chan error) + errChan := make(chan error, 1) @@ -func (s *Server) WaitForError() error { - err := <-s.ErrChan - return err -} +func (s *Server) WaitForError() error { + if err, ok := <-s.ErrChan; ok { + if errors.Is(err, net.ErrClosed) { + return nil + } + return err + } + return nil +}Also applies to: 83-87
vendor/github.com/DeedleFake/p9/proto/proto.go (1)
85-93: Off‑by‑4 framing: bound LimitedReader to size‑4 and guard undersized frames.9P size includes the size field; using N=size leaks 4 bytes into the next frame on boundaries.
Apply this diff:
if (msize > 0) && (size > msize) { return nil, NoTag, util.Errorf("receive: %w", ErrLargeMessage) } - lr := &util.LimitedReader{ - R: r, - N: size, - E: ErrLargeMessage, - } + if size < 4 { + return nil, NoTag, util.Errorf("receive: invalid frame size: %d", size) + } + rem := size - 4 + if rem < 3 { // need at least 1 byte type + 2 bytes tag + return nil, NoTag, util.Errorf("receive: frame too small for header: %d", size) + } + lr := &util.LimitedReader{ + R: r, + N: rem, + E: ErrLargeMessage, + }vendor/github.com/DeedleFake/p9/remote.go (4)
37-43: Build Twalk Wname correctly for absolute paths (no empty elements).Leading “/” currently yields an empty first element or “/” entry, which is invalid for Twalk.
- w := []string{path.Clean(p)} - if w[0] != "/" { - w = strings.Split(w[0], "/") - } - if (len(w) == 1) && (w[0] == ".") { - w = nil - } + p = path.Clean(p) + var w []string + switch { + case p == "" || p == ".": + w = nil + default: + if strings.HasPrefix(p, "/") { + p = strings.TrimPrefix(p, "/") + } + if p != "" { + w = strings.Split(p, "/") + } else { + w = nil + } + }
184-185: Don’t panic on invalid whence; return error.- panic(util.Errorf("Invalid whence: %v", whence)) + return int64(file.pos), util.Errorf("invalid whence: %v", whence)
198-201: Guard maxBufSize underflow; avoid infinite loops on small msize.int(msize-IOHeaderSize) can underflow or be 0, causing ReadAt/WriteAt to loop forever.
-func (file *Remote) maxBufSize() int { - return int(file.client.Msize() - IOHeaderSize) -} +func (file *Remote) maxBufSize() int { + m := file.client.Msize() + if m <= uint32(IOHeaderSize) { + // Minimum 1 to ensure forward progress; handshake should reject too-small msize. + return 1 + } + return int(m - uint32(IOHeaderSize)) +}Additionally, validate negotiated msize during handshake (in vendor/github.com/DeedleFake/p9/client.go) and fail if msize <= IOHeaderSize. Want a patch?
230-239: Prevent zero/negative chunk size in ReadAt.If maxBufSize ≤ 0, the loop never advances.
- size := len(buf) - if size > file.maxBufSize() { - size = file.maxBufSize() - } + size := len(buf) + max := file.maxBufSize() + if max <= 0 { + return 0, util.Errorf("msize too small: %d", file.client.Msize()) + } + if size > max { + size = max + }
🧹 Nitpick comments (10)
vendor/github.com/DeedleFake/p9/dir_darwin.go (2)
23-33: Preserve identity when name lookup fails by falling back to numeric UID/GIDAvoid empty UID/GID fields; default to the numeric IDs and override on successful lookup.
-var uname string -uid, err := user.LookupId(strconv.FormatUint(uint64(sys.Uid), 10)) -if err == nil { - uname = uid.Username -} +uname := strconv.FormatUint(uint64(sys.Uid), 10) +if u, err := user.LookupId(uname); err == nil && u != nil && u.Username != "" { + uname = u.Username +} -var gname string -gid, err := user.LookupGroupId(strconv.FormatUint(uint64(sys.Gid), 10)) -if err == nil { - gname = gid.Name -} +gname := strconv.FormatUint(uint64(sys.Gid), 10) +if g, err := user.LookupGroupId(gname); err == nil && g != nil && g.Name != "" { + gname = g.Name +}
52-55: Improve diagnostics when FileInfo.Sys() isn’t Stat_tIncluding the path and actual type eases debugging. Optional, but helpful.
-import ( - "errors" +import ( + "fmt" "os" "os/user" "strconv" "syscall" "time" ) @@ - sys, _ := fi.Sys().(*syscall.Stat_t) - if sys == nil { - return QID{}, errors.New("failed to get QID: FileInfo was not Stat_t") - } + sys, _ := fi.Sys().(*syscall.Stat_t) + if sys == nil { + return QID{}, fmt.Errorf("failed to get QID for %q: FileInfo.Sys()=%T; want *syscall.Stat_t", d.path(p), fi.Sys()) + }vendor/github.com/DeedleFake/p9/internal/debug/debug.go (1)
1-1: Add go:build tag and newline in debug outputInclude the modern build tag for Go 1.17+ and add a newline to avoid run‑on log lines.
-// +build p9debug +//go:build p9debug +// +build p9debug @@ func Log(str string, args ...interface{}) { - fmt.Fprintf(os.Stderr, str, args...) + _, _ = fmt.Fprintf(os.Stderr, str+"\n", args...) }Also applies to: 10-12
vendor/github.com/DeedleFake/p9/README.md (1)
1-52: Markdown lint noise from vendored docs: exclude vendor/Static analysis flags heading style/tabs here. Prefer excluding vendor/** from markdownlint rather than editing upstream docs.
pkg/crc/config/settings.go (1)
87-88: Help text: don’t claim mountpoint ‘/’ across OSesThe mount location differs by platform. Make the message neutral.
- cfg.AddSetting(EnableSharedDirs, true, ValidateBool, SuccessfullyApplied, - "Mounts host's home directory at '/' in the CRC VM (true/false, default: true)") + cfg.AddSetting(EnableSharedDirs, true, ValidateBool, SuccessfullyApplied, + "Enable sharing of the host home directory into the CRC VM (true/false, default: true)")pkg/fileserver/fs9p/shares_fallback.go (1)
5-13: Return a sentinel error instead of a formatted stringMakes detection easier for callers without string matching.
-import ( - "fmt" - "runtime" -) +import ( + "errors" + "runtime" +) + +var ErrHvsockUnsupported = errors.New("fs9p: StartHvsockShares is only supported on windows") @@ func StartHvsockShares(mounts []HvsockMount9p) ([]*Server, error) { - return nil, fmt.Errorf("StartHvsockShares() not implemented on %s", runtime.GOOS) + _ = runtime.GOOS // keep message parity with windows-only implementation if needed elsewhere + return nil, ErrHvsockUnsupported }pkg/crc/machine/start.go (2)
237-243: SELinux context: virtiofs uses container_file_t; 9P may need labeling or doc9P often doesn’t support SELinux contexts like virtiofs. If containers need access, ensure appropriate labeling/workaround is applied or document the limitation.
320-323: Leftover SMB creds in machine config?SharedDirPassword/Username are still passed into MachineConfig though SMB is removed. If unused, drop them to avoid confusion.
pkg/fileserver/fs9p/server.go (1)
61-69: Avoid flagging immediate net.ErrClosed as startup errorIf the listener gets closed quickly, Start() will surface net.ErrClosed as a startup error. Consider ignoring net.ErrClosed here as well to reduce false alarms.
vendor/github.com/DeedleFake/p9/proto/proto.go (1)
101-108: Remove dead check and return a clear error for unknown msg type.- 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) + }
📜 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 (44)
cmd/crc/cmd/daemon.go(5 hunks)cmd/crc/cmd/start.go(0 hunks)go.mod(2 hunks)packaging/windows/product.wxs.template(2 hunks)pkg/crc/config/settings.go(1 hunks)pkg/crc/constants/constants.go(1 hunks)pkg/crc/machine/driver.go(0 hunks)pkg/crc/machine/driver_darwin.go(0 hunks)pkg/crc/machine/driver_linux.go(0 hunks)pkg/crc/machine/driver_windows.go(0 hunks)pkg/crc/machine/libhvee/driver_windows.go(2 hunks)pkg/crc/machine/start.go(2 hunks)pkg/drivers/libhvee/libhvee_windows.go(0 hunks)pkg/drivers/libhvee/powershell_windows.go(0 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 with no reviewable changes (7)
- pkg/crc/machine/driver_linux.go
- pkg/crc/machine/driver_darwin.go
- pkg/crc/machine/driver_windows.go
- cmd/crc/cmd/start.go
- pkg/drivers/libhvee/libhvee_windows.go
- pkg/drivers/libhvee/powershell_windows.go
- pkg/crc/machine/driver.go
✅ 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 (20)
- vendor/github.com/DeedleFake/p9/LICENSE
- vendor/github.com/DeedleFake/p9/internal/debug/nodebug.go
- vendor/github.com/DeedleFake/p9/addr_unix.go
- vendor/github.com/DeedleFake/p9/proto/encoding.go
- vendor/github.com/DeedleFake/p9/addr.go
- vendor/github.com/DeedleFake/p9/proto/server.go
- vendor/github.com/DeedleFake/p9/dir_other.go
- vendor/github.com/DeedleFake/p9/encoding.go
- vendor/github.com/DeedleFake/p9/fs.go
- vendor/github.com/DeedleFake/p9/stat.go
- vendor/github.com/DeedleFake/p9/dir_plan9.go
- vendor/github.com/DeedleFake/p9/dir_windows.go
- vendor/github.com/DeedleFake/p9/addr_other.go
- vendor/github.com/DeedleFake/p9/dir.go
- go.mod
- vendor/github.com/DeedleFake/p9/dir_linux.go
- vendor/github.com/DeedleFake/p9/internal/util/util.go
- vendor/github.com/DeedleFake/p9/msg.go
- vendor/github.com/DeedleFake/p9/client.go
- vendor/github.com/DeedleFake/p9/p9.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_fallback.gopkg/fileserver/fs9p/shares_windows.gocmd/crc/cmd/daemon.gopkg/fileserver/fs9p/shares.gopkg/fileserver/fs9p/server.govendor/github.com/DeedleFake/p9/README.md
🧬 Code graph analysis (13)
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)
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)
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 (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)
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/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)
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)
pkg/crc/config/settings.go (2)
pkg/crc/config/validations.go (1)
ValidateBool(19-25)pkg/crc/config/callbacks.go (1)
SuccessfullyApplied(36-38)
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 (13)
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)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)
pkg/crc/machine/start.go (3)
pkg/os/exec.go (1)
RunPrivileged(48-59)pkg/crc/constants/constants.go (1)
Plan9HvsockPort(62-62)pkg/crc/logging/logging.go (1)
Warnf(100-102)
pkg/fileserver/fs9p/shares.go (1)
pkg/fileserver/fs9p/server.go (2)
Server(15-24)New9pServer(29-52)
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)
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)
🪛 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). (17)
- GitHub Check: build (windows-2022, 1.24)
- GitHub Check: build (macOS-14, 1.24)
- GitHub Check: build (ubuntu-22.04, 1.24)
- GitHub Check: build (macOS-14, 1.24)
- GitHub Check: build-qe (linux, arm64)
- GitHub Check: build (windows-2022, 1.24)
- GitHub Check: build-qe (windows, amd64)
- GitHub Check: build (ubuntu-latest, 1.24)
- GitHub Check: build (macOS-13, 1.24)
- GitHub Check: build-qe (darwin, amd64)
- GitHub Check: build-qe (linux, amd64)
- GitHub Check: build-qe (darwin, arm64)
- GitHub Check: build-installer (windows-2022, 1.24)
- GitHub Check: build (ubuntu-latest, 1.24)
- GitHub Check: Run OKD bundle with crc (1.24)
- GitHub Check: verify-devcontainer
- GitHub Check: Konflux kflux-prd-rh02 / crc-binary-on-pull-request
🔇 Additional comments (7)
pkg/crc/machine/libhvee/driver_windows.go (1)
41-45: LGTM: switch to 9P and drop SMB credsUsing Type="9p" with no Username/Password on Windows is correct for the new path.
pkg/crc/constants/constants.go (1)
59-62: LGTM: 9P constants addedValues look sane for 9P defaults and hvsock integration.
cmd/crc/cmd/daemon.go (1)
182-186: LGTM: net.JoinHostPortSafer IPv6‑aware address construction.
Also applies to: 198-202
pkg/crc/config/settings.go (1)
33-35: Do not remove SharedDirPassword — it's referenced in the code.
SharedDirPassword is referenced in pkg/crc/machine/start.go:321, pkg/crc/machine/types/types.go:37, and pkg/crc/machine/config/config.go:24; remove only after updating/removing those usages.Likely an incorrect or invalid review comment.
vendor/github.com/DeedleFake/p9/proto/client.go (1)
115-146: Skip NoTag (0xFFFF) when allocating tags to prevent protocol collisionWhen nextTag wraps it can equal NoTag; advance past NoTag before returning the next tag.
- var nextTag uint16 + var nextTag uint16 tags := make(map[uint16]chan interface{}) @@ - case c.nextTag <- nextTag: - for { - nextTag++ - if _, ok := tags[nextTag]; !ok { - break - } - } + case c.nextTag <- nextTag: + for { + nextTag++ + if nextTag == NoTag { + nextTag++ + } + if _, ok := tags[nextTag]; !ok { + break + } + }Given this is vendored, send the fix upstream so it persists across updates.
packaging/windows/product.wxs.template (2)
111-112: LGTM: Feature references include the new HVSOCK 9P component.
69-73: Resolved — GUIDs are alignedpkg/crc/constants/constants.go defines Plan9HvsockGUID = "00009000-FACB-11E6-BD58-64006A7986D3"; the same value is present in packaging/windows/product.wxs.template and used via constants.Plan9HvsockGUID in cmd/crc/cmd/daemon.go. No action required.
5959d98 to
f06fba0
Compare
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (17)
pkg/crc/machine/libhvee/driver_windows.go (1)
39-45: Past review concerns remain unaddressed: tag determinism and centralization.The previous review's concerns about tag/name symmetry, deterministic ordering, and centralizing the tag format are still applicable. The tag generation using
fmt.Sprintf("dir%d", i)remains:
- Non-deterministic: Tag assignment depends on iteration order; reordering
SharedDirswill change tags and break existing mounts.- Not centralized: The "dir%d" format is hardcoded here and must match the 9P server's share naming. If they diverge, mounts will fail.
The removal of the
Usernamefield is correct for 9p (which doesn't require Windows credentials), and theType: "9p"change aligns with the PR's migration from SMB to 9p.To verify tag format consistency with the 9P server, run:
#!/bin/bash # Description: Find where 9P share names/tags are defined on the server side. # Expected: Server should use matching "dir%d" format or derive from same source. # Search for 9P share/tag/aname definitions in fileserver code rg -n -C3 --type=go 'Share.*Tag|aname|dir%d|Plan9.*Tag' pkg/fileserver/ # Search for any tag format constants or helpers rg -n -C3 --type=go 'ShareTag|Plan9Tag|dir%d' pkg/crc/constants/ pkg/fileserver/vendor/github.com/DeedleFake/p9/addr_other.go (1)
23-28: Critical Windows path issue requires upstream fix.As flagged in the previous review, using DISPLAY values containing ":" (like the default ":0") creates invalid Windows paths, causing runtime failures. This is a critical correctness issue for Windows deployments.
Per the PR discussion, this vendored code issue should be fixed upstream. Please file a PR to the DeedleFake/p9 repository with the sanitization fix suggested in the previous comment (adding runtime.GOOS check and replacing ":" with "_" on Windows).
Based on PR discussion guidance.
cmd/crc/cmd/daemon.go (1)
253-280: Missing graceful shutdown for 9P servers.Both
server9pHvsock(line 260-264) andserver9pTCP(line 273-277) are started but their handles are not retained for cleanup. When the daemon shuts down, these servers will leak goroutines and listeners.Capture the server instances and defer their cleanup to ensure graceful shutdown.
For example:
// 9p home directory sharing if runtime.GOOS == "windows" { // 9p over hvsock listener9pHvsock, err := fs9p.GetHvsockListener(constants.Plan9HvsockGUID) if err != nil { return err } server9pHvsock, err := fs9p.New9pServer(listener9pHvsock, constants.GetHomeDir()) if err != nil { return err } if err := server9pHvsock.Start(); err != nil { return err } + defer func() { + if err := server9pHvsock.Stop(); err != nil { + logging.Errorf("Error stopping hvsock 9p server: %v", err) + } + }() // 9p over TCP (as a backup) listener9pTCP, err := vn.Listen("tcp", net.JoinHostPort(configuration.GatewayIP, fmt.Sprintf("%d", constants.Plan9TcpPort))) if err != nil { return err } server9pTCP, err := fs9p.New9pServer(listener9pTCP, constants.GetHomeDir()) if err != nil { return err } if err := server9pTCP.Start(); err != nil { return err } + defer func() { + if err := server9pTCP.Stop(); err != nil { + logging.Errorf("Error stopping TCP 9p server: %v", err) + } + }() }Note: This issue was previously flagged in an earlier review.
pkg/crc/machine/start.go (1)
244-256: Make 9P mount idempotent and clarify privilege/options (FUSE vs kernel 9p)
- Add a pre-check to skip if already mounted.
- If this uses a kernel 9p mount, run it privileged and pass
aname,msize,version,cache. If it’s a FUSE client, keep unprivileged but ensure tag/export is specified and considermsize.Example (kernel mount path):
case "9p": + // Skip if already mounted + if _, _, err := sshRunner.RunPrivileged("Check mountpoint", "mountpoint", "-q", mount.Target); err == nil { + logging.Debugf("Already mounted: %s", mount.Target) + continue + } // change owner to core user to allow mounting to it as a non-root user if _, _, err := sshRunner.RunPrivileged("Changing owner of mount directory", "chown", "core:core", mount.Target); err != nil { return err } - if _, _, err := sshRunner.Run("9pfs", "-V", "-p", fmt.Sprintf("%d", constants.Plan9HvsockPort), mount.Target); err != nil { + // Prefer explicit 9p kernel mount with options; if you intend to use FUSE 9pfs, please confirm and keep it. + opts := fmt.Sprintf("trans=tcp,port=%d,msize=%d,cache=loose,version=9p2000.L,aname=%s", constants.Plan9TcpPort, constants.Plan9Msize, mount.Tag) + if _, _, err := sshRunner.RunPrivileged( + fmt.Sprintf("Mounting %s (9p tag %s)", mount.Target, mount.Tag), + "mount", "-t", "9p", "-o", opts, constants.VSockGateway, mount.Target, + ); err != nil { logging.Warnf("Failed to connect to 9p server over hvsock: %v", err) logging.Warnf("Falling back to 9p over TCP") - if _, _, err := sshRunner.Run("9pfs", "192.168.127.1", mount.Target); err != nil { + if _, _, err := sshRunner.RunPrivileged("Mount 9p over TCP", "mount", "-t", "9p", "-o", opts, constants.VSockGateway, mount.Target); err != nil { return err } }If you intend to use a FUSE-based
9pfsclient, please confirm its CLI for specifying the export/tag and msize, and we’ll adjust accordingly instead of usingmount -t 9p.Run to find other combined-token calls:
#!/bin/bash rg -nP -C2 'Run(?:Privileged)?\([^,]+,\s*"[^",\s]+ [^"]+"' --type govendor/github.com/DeedleFake/p9/dir_linux.go (1)
35-43: Fix ATime construction: syscall.Timespec lacks Unix() (compile error)Use Sec/Nsec fields of
Stat_t.Atim.- ATime: time.Unix(sys.Atim.Unix()), + ATime: time.Unix(int64(sys.Atim.Sec), int64(sys.Atim.Nsec)),pkg/fileserver/fs9p/server.go (2)
71-80: Stop() is a hard close; consider graceful shutdown or document clearlyEither track connections and drain with a timeout, or document at call sites that Stop forcibly interrupts active I/O and callers should ignore net.ErrClosed from WaitForError.
3-13: Avoid goroutine leak; treat net.ErrClosed as clean shutdownMake ErrChan buffered and ignore net.ErrClosed in WaitForError. Adds errors import.
@@ -import ( +import ( + "errors" "fmt" "net" "os" "path/filepath" @@ - errChan := make(chan error) + errChan := make(chan error, 1) @@ -func (s *Server) WaitForError() error { - err := <-s.ErrChan - return err -} +func (s *Server) WaitForError() error { + if err, ok := <-s.ErrChan; ok { + if errors.Is(err, net.ErrClosed) { + return nil + } + return err + } + return nil +}Also applies to: 43-51, 83-87
vendor/github.com/DeedleFake/p9/dir_darwin.go (1)
35-43: Fix Darwin build: ATime construction uses non-existent Timespec.Unix()Use time.Unix(sec, nsec) with Timespec fields. Current code won’t compile on darwin.
return DirEntry{ FileMode: ModeFromOS(fi.Mode()), - ATime: time.Unix(sys.Atimespec.Unix()), + ATime: time.Unix(int64(sys.Atimespec.Sec), int64(sys.Atimespec.Nsec)), MTime: fi.ModTime(), Length: uint64(fi.Size()), EntryName: fi.Name(), UID: uname, GID: gname, }vendor/github.com/DeedleFake/p9/client.go (1)
3-9: Guard response type assertions to avoid panics (Handshake/Auth/Attach)Unchecked rsp.(*Type) will panic on protocol errors. Guard and return errors; add fmt import.
@@ -import ( +import ( "errors" + "fmt" "net" "sync/atomic" @@ - version := rsp.(*Rversion) + version, ok := rsp.(*Rversion) + if !ok { + return 0, fmt.Errorf("handshake: expected Rversion, got %T", rsp) + } @@ - rauth := rsp.(*Rauth) + rauth, ok := rsp.(*Rauth) + if !ok { + return nil, fmt.Errorf("auth: expected Rauth, got %T", rsp) + } @@ - attach := rsp.(*Rattach) + attach, ok := rsp.(*Rattach) + if !ok { + return nil, fmt.Errorf("attach: expected Rattach, got %T", rsp) + }Also applies to: 61-69, 84-91, 113-120
vendor/github.com/DeedleFake/p9/dir.go (2)
52-60: Avoid clobbering timestamps on partial ATime/MTime updatesCalling Chtimes with only one set overwrites the other with zero. Reject partial updates.
- 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 { + return errors.New("setting only one of ATime or MTime is not supported") + } + if ok1 && ok2 { + err := os.Chtimes(p, atime, mtime) if err != nil { return err } }
69-75: Prevent path traversal on rename (validate name is a base name)New name must not contain separators or traversal; keep rename within directory.
@@ -import ( +import ( "errors" "os" "path/filepath" + "strings" ) @@ - name, ok := changes.Name() + name, ok := changes.Name() if ok { - err := os.Rename(p, filepath.Join(base, filepath.FromSlash(name))) + // disallow separators/traversal; only a base name is allowed + if name != filepath.Base(name) || strings.ContainsRune(name, os.PathSeparator) { + return errors.New("invalid name: must be a base name without path separators") + } + err := os.Rename(p, filepath.Join(base, name)) if err != nil { return err } }Also applies to: 3-7
vendor/github.com/DeedleFake/p9/proto/server.go (1)
3-9: Don’t dispatch after Receive errors; exit on closed connectionsBreak/continue appropriately; avoid using stale msg/tag and busy-looping on closed sockets. Add errors import.
@@ -import ( +import ( + "errors" "io" "log" "net" "sync" ) @@ - tmsg, tag, err := p.Receive(c, msize) + tmsg, tag, err := p.Receive(c, msize) if err != nil { - if err == io.EOF { - return - } - - log.Printf("Error reading message: %v", err) + // Terminate on EOF or closed connection; otherwise log and continue. + if err == io.EOF || errors.Is(err, net.ErrClosed) { + return + } + log.Printf("Error reading message: %v", err) + continue }Also applies to: 69-78
packaging/windows/product.wxs.template (2)
64-68: Do the same for gvisor‑tap‑vsock for consistency.Prevent potential redirection for the existing vsock key as well.
- <Component Id="VsockRegistryEntry" Guid="*"> + <Component Id="VsockRegistryEntry" Guid="*" Win64="yes">
69-73: Write 9P HVSOCK registry to 64‑bit hive (avoid Wow6432 redirection).Mark component as 64‑bit so the key lands under the native HKLM hive.
- <Component Id="Hvsock9pRegistryEntry" Guid="*"> + <Component Id="Hvsock9pRegistryEntry" Guid="*" Win64="yes">vendor/github.com/DeedleFake/p9/remote.go (3)
37-43: Fix Wname building for absolute paths.Leading “/” yields empty element; “/” yields invalid path. Clean, trim leading slash, split, drop empties.
- w := []string{path.Clean(p)} - if w[0] != "/" { - w = strings.Split(w[0], "/") - } - if (len(w) == 1) && (w[0] == ".") { - w = nil - } + p = path.Clean(p) + var w []string + switch { + case p == "" || p == "." || p == "/": + w = nil + default: + w = strings.Split(strings.TrimPrefix(p, "/"), "/") + }
184-185: Don’t panic on invalid whence; return error.Library code should not abort the process.
- panic(util.Errorf("Invalid whence: %v", whence)) + return int64(file.pos), util.Errorf("invalid whence: %v", whence)
198-201: Avoid underflow/zero in maxBufSize; guard for tiny msize.If
Msize() <= IOHeaderSize, the subtraction underflows or returns 0; callers then loop forever.-func (file *Remote) maxBufSize() int { - return int(file.client.Msize() - IOHeaderSize) -} +func (file *Remote) maxBufSize() int { + m := file.client.Msize() + if m <= uint32(IOHeaderSize) { + return -1 + } + return int(m - uint32(IOHeaderSize)) +}
🧹 Nitpick comments (7)
vendor/github.com/DeedleFake/p9/internal/debug/nodebug.go (1)
1-1: Consider updating to modern build tag format (upstream).The old-style
// +build !p9debugconstraint is deprecated in favor of the//go:build !p9debugformat introduced in Go 1.17. Since this is vendored code, if you consider this worth addressing, you could file an upstream PR to modernize the build tags.Based on learnings (cfergeau's guidance on vendored code).
vendor/github.com/DeedleFake/p9/README.md (3)
4-5: Use HTTPS and pkg.go.dev for the docs badge/link (upstream change).Prefer pkg.go.dev over the legacy GoDoc URL and enforce HTTPS to avoid mixed-content/link-rot.
If we keep vendor pristine, consider an upstream PR. Locally, ensure link checkers ignore vendor paths.
15-19: Hard tabs inside fenced code blocks trigger MD010; decide policy.Either keep tabs (common in Go) and ignore vendor/** in markdownlint, or replace with spaces upstream. Avoid local edits to vendored files.
Also applies to: 24-51
12-12: Consistent heading style or vendor exclusion
The vendored README mixes ATX (### Server) with setext headings, triggering markdownlint’s style rules. Either update the upstream file to unify heading styles or addvendor/**to your.markdownlintignore(or CI exclude patterns) so vendored docs are skipped.pkg/crc/constants/constants.go (1)
59-62: Constants look good; consider documenting expected units and consumersAdd short comments for
Plan9Msize(bytes),Plan9TcpPort,Plan9HvsockGUID/Port, and ensure both client and server use the samemsize.vendor/github.com/DeedleFake/p9/stat.go (1)
187-193: Guard stat size overflow.
size()sums into uint16; very long names/IDs overflow silently. Validate before encode.func (s Stat) size() uint16 { - return uint16(47 + len(s.Name) + len(s.UID) + len(s.GID) + len(s.MUID)) + return uint16(47 + len(s.Name) + len(s.UID) + len(s.GID) + len(s.MUID)) } func (s Stat) P9Encode() (r []byte, err error) { + if int(47+len(s.Name)+len(s.UID)+len(s.GID)+len(s.MUID)) > 0xFFFF { + return nil, ErrLargeStat + }vendor/github.com/DeedleFake/p9/remote.go (1)
124-141: Minor: avoid shadowing the receiver in Remove().Using
file, err := file.walk(p)shadowsfile(receiver), hurting readability.- file, err := file.walk(p) + target, err := file.walk(p) if err != nil { return err } // Close is not necessary. Remove is also a clunk. - return file.Remove("") + return target.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 (44)
cmd/crc/cmd/daemon.go(5 hunks)cmd/crc/cmd/start.go(0 hunks)go.mod(1 hunks)packaging/windows/product.wxs.template(2 hunks)pkg/crc/config/settings.go(1 hunks)pkg/crc/constants/constants.go(1 hunks)pkg/crc/machine/driver.go(0 hunks)pkg/crc/machine/driver_darwin.go(0 hunks)pkg/crc/machine/driver_linux.go(0 hunks)pkg/crc/machine/driver_windows.go(0 hunks)pkg/crc/machine/libhvee/driver_windows.go(2 hunks)pkg/crc/machine/start.go(2 hunks)pkg/drivers/libhvee/libhvee_windows.go(0 hunks)pkg/drivers/libhvee/powershell_windows.go(0 hunks)pkg/fileserver/fs9p/server.go(1 hunks)pkg/fileserver/fs9p/server_fallback.go(1 hunks)pkg/fileserver/fs9p/server_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)vendor/modules.txt(1 hunks)
💤 Files with no reviewable changes (7)
- pkg/drivers/libhvee/powershell_windows.go
- pkg/drivers/libhvee/libhvee_windows.go
- pkg/crc/machine/driver_darwin.go
- pkg/crc/machine/driver.go
- pkg/crc/machine/driver_windows.go
- cmd/crc/cmd/start.go
- pkg/crc/machine/driver_linux.go
✅ 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 (12)
- vendor/github.com/DeedleFake/p9/addr_unix.go
- vendor/github.com/DeedleFake/p9/LICENSE
- vendor/github.com/DeedleFake/p9/proto/proto.go
- vendor/github.com/DeedleFake/p9/proto/encoding.go
- go.mod
- vendor/github.com/DeedleFake/p9/dir_plan9.go
- pkg/crc/config/settings.go
- vendor/github.com/DeedleFake/p9/p9.go
- vendor/modules.txt
- vendor/github.com/DeedleFake/p9/msg.go
- vendor/github.com/DeedleFake/p9/internal/util/util.go
- vendor/github.com/DeedleFake/p9/internal/debug/debug.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/server_windows.gopkg/fileserver/fs9p/server_fallback.go
🧬 Code graph analysis (21)
pkg/fileserver/fs9p/server_windows.go (1)
pkg/fileserver/fs9p/server_fallback.go (1)
GetHvsockListener(13-15)
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/dir_windows.go (1)
vendor/github.com/DeedleFake/p9/stat.go (3)
DirEntry(255-267)FileMode(23-23)ModeFromOS(43-78)
pkg/crc/machine/start.go (3)
pkg/os/exec.go (1)
RunPrivileged(48-59)pkg/crc/constants/constants.go (1)
Plan9HvsockPort(62-62)pkg/crc/logging/logging.go (1)
Warnf(100-102)
vendor/github.com/DeedleFake/p9/addr_other.go (1)
vendor/github.com/DeedleFake/p9/addr_unix.go (1)
NamespaceDir(17-29)
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/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/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/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/nodebug.go (1)
vendor/github.com/DeedleFake/p9/internal/debug/debug.go (1)
Log(10-12)
cmd/crc/cmd/daemon.go (4)
pkg/fileserver/fs9p/server_fallback.go (1)
GetHvsockListener(13-15)pkg/fileserver/fs9p/server_windows.go (1)
GetHvsockListener(14-29)pkg/crc/constants/constants.go (3)
Plan9HvsockGUID(61-61)GetHomeDir(166-172)Plan9TcpPort(60-60)pkg/fileserver/fs9p/server.go (1)
New9pServer(29-52)
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.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/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(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/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/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)
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/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/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/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_fallback.go (1)
pkg/fileserver/fs9p/server_windows.go (1)
GetHvsockListener(14-29)
🪛 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 (windows-2022, 1.24)
- GitHub Check: build (macOS-14, 1.24)
- GitHub Check: build (ubuntu-latest, 1.24)
- GitHub Check: Konflux kflux-prd-rh02 / crc-binary-on-pull-request
🔇 Additional comments (14)
vendor/github.com/DeedleFake/p9/internal/debug/nodebug.go (1)
3-5: LGTM! Clean no-op debug stub.The implementation correctly provides a no-op debug interface for non-debug builds. The function signature matches the active debug implementation, and the build constraint ensures proper conditional compilation.
pkg/crc/machine/libhvee/driver_windows.go (1)
4-4: LGTM: Import needed for dynamic tag generation.The
fmtimport is correctly added to support thefmt.Sprintfcall on line 43.vendor/github.com/DeedleFake/p9/addr.go (3)
32-66: LGTM - ParseAddr logic is sound.The multi-path parsing logic correctly handles namespace addresses, Unix sockets, TCP with ports, and the network!address[!port] format. The pseudo-port mapping for "9p" and "9fs" to standard port 564 is appropriate.
70-72: GetNamespace inherits Windows path issue from NamespaceDir().This function correctly constructs Unix socket paths for namespaces, but it will inherit the Windows path safety issue from
NamespaceDir()inaddr_other.go(where DISPLAY with ":" creates invalid Windows paths). Once that upstream issue is fixed, this function will work correctly on Windows.
76-78: LGTM - IsNamespaceAddr is correct.Simple and correct implementation for detecting namespace addresses by checking for the "$" prefix.
vendor/github.com/DeedleFake/p9/README.md (1)
1-51: Vendored p9 includes its LICENSE (v0.6.12); no changes required. Continue filing any fixes upstream and update the vendor commit rather than modifying vendored sources.pkg/fileserver/fs9p/server_windows.go (1)
12-29: LGTM! Clean HVSOCK listener implementation.The function correctly parses the HVSOCK GUID, creates a listener with a wildcard VMID to accept connections from any VM, and provides clear error messages with proper context wrapping.
vendor/github.com/DeedleFake/p9/proto/client.go (1)
1-224: Vendored 9P client looks reasonable.This is vendored code from
github.com/DeedleFake/p9. The implementation includes proper goroutine coordination, error handling, and resource cleanup. No obvious critical issues that would require immediate upstream reporting.Per the PR discussion, if issues are discovered later (particularly security-related), they should be reported upstream for proper propagation.
vendor/github.com/DeedleFake/p9/dir_other.go (1)
1-15: Vendored fallback implementation for non-major platforms.This is vendored code providing a minimal
os.FileInfo→DirEntryconversion for platforms other than Linux, Darwin, Plan9, and Windows. The implementation omits UID/GID/MUID/Path/Version fields (zero values). Since this PR targets Windows (which has its own platform-specific implementation), this fallback is unlikely to be exercised in crc.If issues arise with missing fields on edge-case platforms, they should be reported upstream.
pkg/fileserver/fs9p/server_fallback.go (1)
11-15: LGTM! Appropriate non-Windows stub.The fallback implementation correctly returns a not-implemented error for non-Windows platforms, allowing the codebase to compile across platforms while providing clear feedback if the function is mistakenly called.
vendor/github.com/DeedleFake/p9/dir_windows.go (1)
9-27: LGTM: safe type assertion and sane defaultsGraceful fallback when
Sys()lacks Win32 data; ATime filled when available; Mode/Length/Name always set.vendor/github.com/DeedleFake/p9/encoding.go (2)
16-30: LGTM: ReadDir handles EOF correctly and preserves partial resultsEOF normalization and accumulation are appropriate for directory streams.
33-41: LGTM: WriteDir writes sequentially and fails fastEarly return on first error is fine for callers.
vendor/github.com/DeedleFake/p9/dir_linux.go (1)
46-62: QID derivation looks correctUsing inode for Path and Mode-derived QID type is reasonable on Linux.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (20)
packaging/windows/product.wxs.template (1)
69-73: Mark Hvsock component as 64-bit to avoid registry redirection.Without
Win64="yes"the installer writes the GuestCommunicationServices key under Wow6432Node, so the 9P HVSOCK endpoint never registers. Please add the attribute (same fix previously suggested).- <Component Id="Hvsock9pRegistryEntry" Guid="*"> + <Component Id="Hvsock9pRegistryEntry" Guid="*" Win64="yes">vendor/github.com/DeedleFake/p9/dir_darwin.go (1)
35-43: Fix ATime construction to use Timespec fields
sys.Atimespechas noUnix()method on Darwin, so this code does not compile. BuildATimewith theSec/Nsecfields (cast toint64) instead.- ATime: time.Unix(sys.Atimespec.Unix()), + ATime: time.Unix(int64(sys.Atimespec.Sec), int64(sys.Atimespec.Nsec)),vendor/github.com/DeedleFake/p9/dir.go (1)
18-127: Critical security issues from previous review remain unaddressed.This vendored file still contains the following critical vulnerabilities identified in prior reviews:
Path traversal vulnerability (lines 18-20 and throughout):
filepath.Joinalone does not prevent ".." from escaping the served root directory, allowing clients to access arbitrary filesystem paths.Timestamp corruption (lines 52-60): Partial ATime/MTime updates can clobber timestamps when only one is provided.
Name change path traversal (lines 69-75): The rename operation allows path separators in names, enabling directory traversal.
Per the PR discussion and learnings, these issues should be addressed upstream in the DeedleFake/p9 repository. Consider filing PRs with the fixes suggested in the previous review comments, then updating the vendor when merged.
Based on learnings.
vendor/github.com/DeedleFake/p9/proto/proto.go (1)
85-94: Critical framing bug from previous review remains unaddressed.The
LimitedReaderbound is still set toN: sizeinstead ofN: size - 4. The 9P size field includes its own 4 bytes, so the remaining frame length after reading the size issize - 4. This off-by-4 error allows reads to leak 4 bytes into the next frame, breaking message boundaries and causing hard-to-debug protocol errors.As with other issues in this vendored code, consider filing an upstream PR with the fix suggested in the previous review comment.
Based on learnings.
vendor/github.com/DeedleFake/p9/fs.go (3)
179-185: Unsafe string→[]byte cast causes undefined behavior.Line 179 uses
*(*[]byte)(unsafe.Pointer(&p))to convert a string to[]bytefor hashing. This is invalid because string and slice headers have different layouts, resulting in undefined behavior that can corrupt memory or crash under GC/stack movement.The safe fix is simple:
sum := sha256.Sum256([]byte(p)).Based on learnings.
244-249: Tflush handler violates 9P spec by returning Rerror.The 9P specification requires that Tflush requests be acknowledged with Rflush, not Rerror. Even if the implementation cannot cancel in-flight operations, it should return
&Rflush{}as a best-effort acknowledgment.Based on learnings.
593-599: Type assertion bug prevents clunk error propagation.Line 596 checks
rsp.(error), buth.clunk()returns*Rerror, noterror. This assertion never succeeds, so clunk errors are silently ignored instead of being propagated to the client.The correct check is
if er, ok := rsp.(*Rerror); ok { return er }.Based on learnings.
vendor/github.com/DeedleFake/p9/proto/server.go (1)
70-99: Error handling bug allows handler invocation with invalid data.After a
Receiveerror at line 70, the code logs the error but continues to line 79, calling the handler with potentially garbagetmsgandtagvalues. This can cause panics or invalid protocol responses.The correct behavior is:
- On
io.EOFornet.ErrClosed: return/break to terminate the connection cleanly- On other errors: log and
continueto skip handler invocation and retry receivingThe previous review comment provides the complete fix.
Based on learnings.
vendor/github.com/DeedleFake/p9/remote.go (5)
34-71: Absolute path handling remains broken (duplicate).This issue was flagged in previous reviews:
walk()incorrectly splits absolute paths, producing empty elements or invalid "/" in Wname, violating the 9P protocol. Based on coding guidelines, file an upstream PR so the fix propagates when the vendor dependency is updated.
147-185: panic on invalid whence remains (duplicate).Previous review flagged that library code should not panic for invalid input. Return an error instead. File an upstream PR per project guidance.
198-200: maxBufSize() underflow guard missing (duplicate).Previous reviews identified that when
Msize() <= IOHeaderSize, the subtraction wraps/underflows, and returning 0 causes infinite loops in ReadAt/WriteAt. The extended comment in past reviews recommends validating negotiated msize at handshake (client.go) and clamping to a safe positive value here. File an upstream PR.
229-248: ReadAt loop guard missing (duplicate).Previous review flagged that if
maxBufSize() <= 0, the loop increments by zero or negative size, causing an infinite loop. Guard against non-positive chunk size before entering the loop. File an upstream PR.
288-308: WriteAt loop guard missing (duplicate).Previous review flagged the same infinite-loop risk as ReadAt. Guard against non-positive chunk size. File an upstream PR.
vendor/github.com/DeedleFake/p9/stat.go (1)
135-153: Unsafe string conversion creates dangling pointer (duplicate).Previous review correctly identified that
*(*string)(unsafe.Pointer(&buf))at line 152 creates a string header pointing to stack memory inbuf, which is freed when the function returns. This is a use-after-free bug. Usereturn string(buf)for safe conversion. File an upstream PR per project guidance.pkg/fileserver/fs9p/server.go (4)
3-13: Add missing import for errors.import ( + "errors" "fmt" "net" "os" "path/filepath"
71-81: Stop() is a hard close; consider graceful shutdown or tighten contract.If graceful isn’t feasible now, document at call sites and ensure callers either drain/ignore WaitForError (net.ErrClosed ignored per earlier change) and don’t leave clients stuck. Add a TODO with rationale and link to tracking issue.
42-50: Buffer ErrChan to avoid goroutine leak if nobody is receiving.Unbuffered channel can block the Serve goroutine on shutdown. Use capacity 1.
- fs := p9.FileSystem(p9.Dir(exposeDir)) - errChan := make(chan error) + fs := p9.FileSystem(p9.Dir(exposeDir)) + errChan := make(chan error, 1)
83-87: Treat net.ErrClosed as clean shutdown and handle closed channel.WaitForError should not surface net.ErrClosed when Stop() closes the listener; also handle closed channel.
-func (s *Server) WaitForError() error { - err := <-s.ErrChan - return err -} +func (s *Server) WaitForError() error { + if err, ok := <-s.ErrChan; ok { + if errors.Is(err, net.ErrClosed) { + return nil + } + return err + } + return nil +}vendor/github.com/DeedleFake/p9/proto/encoding.go (2)
33-38: Fail fast if Read target isn’t a non-nil pointer (prevents panic).Avoid panic when callers pass non-pointers or nil.
func Read(r io.Reader, v interface{}) error { - d := &decoder{r: r} - d.decode(reflect.ValueOf(v)) + d := &decoder{r: r} + rv := reflect.ValueOf(v) + if rv.Kind() != reflect.Ptr || rv.IsNil() { + return util.Errorf("proto.Read expects a non-nil pointer, got %T", v) + } + d.decode(rv) return d.err }
97-100: Guard against strings > 65535 bytes to avoid uint16 wrap/truncation.case reflect.String: - e.mode(uint16(v.Len())) - e.mode([]byte(v.String())) + if v.Len() > 0xFFFF { + e.err = util.Errorf("string too long: %d bytes", v.Len()) + return + } + e.mode(uint16(v.Len())) + e.mode([]byte(v.String()))
🧹 Nitpick comments (2)
pkg/fileserver/fs9p/server.go (2)
29-41: Guard against nil listener in New9pServer.Fail fast if a nil listener is passed.
func New9pServer(listener net.Listener, exposeDir string) (*Server, error) { + if listener == nil { + return nil, fmt.Errorf("listener must not be nil") + } // Verify that exposeDir makes sense.
55-69: Add an idempotency guard to Server.Start. Although existing callers only invoke Start once per instance (e.g. server9pHvsock and server9pTCP in cmd/crc/cmd/daemon.go), a repeated call would panic from closing ErrChan twice. Introduce astarted uint32flag and wrap Start withatomic.CompareAndSwapUint32(&s.started, 0, 1)to return an error on duplicate starts.
📜 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 (44)
cmd/crc/cmd/daemon.go(5 hunks)cmd/crc/cmd/start.go(0 hunks)go.mod(2 hunks)packaging/windows/product.wxs.template(2 hunks)pkg/crc/config/settings.go(1 hunks)pkg/crc/constants/constants.go(1 hunks)pkg/crc/machine/driver.go(0 hunks)pkg/crc/machine/driver_darwin.go(0 hunks)pkg/crc/machine/driver_linux.go(0 hunks)pkg/crc/machine/driver_windows.go(0 hunks)pkg/crc/machine/libhvee/driver_windows.go(2 hunks)pkg/crc/machine/start.go(2 hunks)pkg/drivers/libhvee/libhvee_windows.go(0 hunks)pkg/drivers/libhvee/powershell_windows.go(0 hunks)pkg/fileserver/fs9p/server.go(1 hunks)pkg/fileserver/fs9p/server_fallback.go(1 hunks)pkg/fileserver/fs9p/server_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)vendor/modules.txt(1 hunks)
💤 Files with no reviewable changes (7)
- pkg/crc/machine/driver_darwin.go
- pkg/crc/machine/driver_windows.go
- pkg/drivers/libhvee/powershell_windows.go
- pkg/drivers/libhvee/libhvee_windows.go
- pkg/crc/machine/driver.go
- cmd/crc/cmd/start.go
- pkg/crc/machine/driver_linux.go
✅ 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 (16)
- pkg/crc/constants/constants.go
- vendor/github.com/DeedleFake/p9/dir_other.go
- vendor/github.com/DeedleFake/p9/addr_unix.go
- pkg/crc/machine/start.go
- pkg/fileserver/fs9p/server_windows.go
- vendor/github.com/DeedleFake/p9/client.go
- go.mod
- vendor/github.com/DeedleFake/p9/proto/client.go
- vendor/modules.txt
- vendor/github.com/DeedleFake/p9/dir_linux.go
- pkg/crc/machine/libhvee/driver_windows.go
- vendor/github.com/DeedleFake/p9/LICENSE
- cmd/crc/cmd/daemon.go
- vendor/github.com/DeedleFake/p9/addr_other.go
- vendor/github.com/DeedleFake/p9/internal/debug/debug.go
- vendor/github.com/DeedleFake/p9/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/server_fallback.go
🧬 Code graph analysis (18)
vendor/github.com/DeedleFake/p9/internal/debug/nodebug.go (1)
vendor/github.com/DeedleFake/p9/internal/debug/debug.go (1)
Log(10-12)
pkg/crc/config/settings.go (2)
pkg/crc/config/validations.go (1)
ValidateBool(19-25)pkg/crc/config/callbacks.go (1)
SuccessfullyApplied(36-38)
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/dir_windows.go (1)
vendor/github.com/DeedleFake/p9/stat.go (3)
DirEntry(255-267)FileMode(23-23)ModeFromOS(43-78)
pkg/fileserver/fs9p/server_fallback.go (1)
pkg/fileserver/fs9p/server_windows.go (1)
GetHvsockListener(14-29)
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/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/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/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 (5)
QID(42-46)QIDType(49-49)Version(10-10)QTAuth(58-58)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 (25)
Tversion(79-82)Tversion(84-84)Rversion(86-89)Tauth(95-99)Rauth(101-103)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/encoding.go (1)
Read(34-38)
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)
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/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/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 (13)
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)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/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)
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/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/p9.go (1)
vendor/github.com/DeedleFake/p9/stat.go (1)
FileMode(23-23)
vendor/github.com/DeedleFake/p9/internal/util/util.go (1)
vendor/github.com/DeedleFake/p9/proto/encoding.go (1)
Read(34-38)
🪛 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). (1)
- GitHub Check: Konflux kflux-prd-rh02 / crc-binary-on-pull-request
🔇 Additional comments (11)
vendor/github.com/DeedleFake/p9/internal/debug/nodebug.go (1)
1-5: LGTM! Vendored no-op debug implementation is correct.This vendored file correctly implements a no-op debug logger that's compiled when the
p9debugbuild tag is not set, complementing the active implementation indebug.go. The build constraint and function signature are appropriate for conditional compilation of debug logging.pkg/crc/config/settings.go (2)
87-88: LGTM! Clean cross-platform shared directory configuration.The unconditional
EnableSharedDirssetting correctly replaces the OS-specific SMB configuration with a universal approach that works with 9p. The default value oftruemaintains backward compatibility, and the implementation properly uses standard validation and callback functions.
34-34: Keep SharedDirPassword constant as it’s still in use
SharedDirPasswordis referenced in pkg/crc/machine/config/config.go, pkg/crc/machine/types/types.go, and pkg/crc/machine/start.go and must not be removed.Likely an incorrect or invalid review comment.
pkg/fileserver/fs9p/server_fallback.go (1)
1-15: LGTM!The build tag separation and error message are appropriate. The stub correctly signals that HVSOCK is Windows-only.
vendor/github.com/DeedleFake/p9/addr.go (1)
32-66: LGTM!The address parsing logic correctly handles multiple address formats (namespace, Unix socket, TCP, and bang notation). The implementation is sound for a vendored 9P library.
vendor/github.com/DeedleFake/p9/dir_windows.go (1)
9-27: LGTM!The Windows-specific FileInfo-to-DirEntry conversion correctly handles both cases (with and without syscall data) and properly extracts ATime from Windows file attributes.
vendor/github.com/DeedleFake/p9/dir_plan9.go (2)
10-31: LGTM!The Plan9-specific FileInfo-to-DirEntry conversion correctly handles syscall.Dir metadata extraction, including proper time conversion for Plan9's uint32 Atime field.
33-49: LGTM!The QID extraction correctly stats the file, validates the syscall.Dir type, and constructs the QID from the native Plan9 Qid fields.
vendor/github.com/DeedleFake/p9/p9.go (1)
1-72: LGTM – Vendored 9P constants and types.The constants and types defined here are standard 9P protocol scaffolding. No issues identified.
vendor/github.com/DeedleFake/p9/stat.go (1)
1-351: Rest of stat.go looks reasonable.FileMode conversions, Stat/DirEntry types, and P9Encode/P9Decode implementations follow safe patterns and match the 9P protocol specification.
vendor/github.com/DeedleFake/p9/msg.go (1)
1-280: LGTM – 9P message types and protocol registry.Message type constants, structs, and encoding/decoding implementations follow the 9P2000 specification and use safe patterns. No issues identified.
|
/hold |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (15)
pkg/crc/machine/start.go (2)
246-246: Fix argv tokenization:chown core:corewill fail binary lookupThe
RunPrivilegedmethod expects the binary and each argument as separate tokens. Passing"chown core:core"as a single string will attempt to execute a binary named"chown core:core"and fail.Apply this diff to split the command into separate arguments:
- 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 {
249-251: Fix multiple critical issues with 9P mountingThis implementation has several critical issues:
- Argv tokenization:
"9pfs 192.168.127.1"will fail binary lookup (should be"9pfs", "192.168.127.1")- Missing privileges: Mounting requires root; using
Runinstead ofRunPrivilegedwill likely fail- Missing mount options: No trans, port, version, msize, cache specified
- Missing tag/aname: The 9P server can't resolve which export to mount without the tag
- Non-standard helper: Using
9pfsis brittle; prefer explicitmount -t 9pApply this diff to use explicit 9P mount with all required options:
- if _, _, err := sshRunner.Run("9pfs 192.168.127.1", mount.Target); err != nil { - return err + opts := fmt.Sprintf("trans=tcp,port=%d,msize=%d,cache=loose,version=9p2000.L,aname=%s", constants.Plan9Port, constants.Plan9Msize, mount.Tag) + if _, _, err := sshRunner.RunPrivileged( + fmt.Sprintf("Mounting %s (9p tag %s)", mount.Target, mount.Tag), + "mount", "-t", "9p", "-o", opts, "192.168.127.1", mount.Target, + ); err != nil { + return err }Note: This assumes
constants.Plan9Portandconstants.Plan9Msizeare defined. If not, use standard values (e.g., port 564, msize 65536).cmd/crc/cmd/daemon.go (1)
253-266: Stop and monitor the Windows 9p serverWe still start the 9p server but never tear it down or surface its failures; this leaks the listener/goroutine after
runreturns (tests or retries in the same process will fail with “address already in use”) and hides runtime faults. Please defer aStop()and forwardErrChanintoerrChso the daemon can shut down cleanly and react to 9p errors.if runtime.GOOS == "windows" { listener9p, err := vn.Listen("tcp", net.JoinHostPort(configuration.GatewayIP, fmt.Sprintf("%d", constants.Plan9TcpPort))) if err != nil { return err } server9p, err := fs9p.New9pServer(listener9p, constants.GetHomeDir()) if err != nil { return err } + defer func() { + if err := server9p.Stop(); err != nil { + logging.Errorf("error stopping 9p server: %v", err) + } + }() + go func() { + for err := range server9p.ErrChan { + errCh <- errors.Wrap(err, "9p server failed") + } + }() if err := server9p.Start(); err != nil { return err } }vendor/github.com/DeedleFake/p9/proto/encoding.go (2)
33-38: Guard against nil or non-pointer inputs in Read
Readstill callsreflect.Indirecton whatever comes in, so a nil interface or non-pointer will panic. Please fail fast before decoding.Apply this diff to harden the decoder:
func Read(r io.Reader, v interface{}) error { - d := &decoder{r: r} - d.decode(reflect.ValueOf(v)) + rv := reflect.ValueOf(v) + if rv.Kind() != reflect.Ptr || rv.IsNil() { + return util.Errorf("proto.Read expects a non-nil pointer, got %T", v) + } + + d := &decoder{r: r} + d.decode(rv) return d.err }
97-100: Enforce the 64 KiB string limit instead of truncatingCasting
lentouint16silently wraps for payloads >65 535 bytes, so the wire length prefix no longer matches the bytes we emit—subsequent fields decode incorrectly. Please detect oversize strings and return a descriptive error.Apply this diff to enforce the boundary:
case reflect.String: - e.mode(uint16(v.Len())) - e.mode([]byte(v.String())) + if v.Len() > 0xFFFF { + e.err = util.Errorf("string too long: %d bytes", v.Len()) + return + } + e.mode(uint16(v.Len())) + e.mode([]byte(v.String()))vendor/github.com/DeedleFake/p9/dir_darwin.go (1)
35-41: Restore darwin build: use Sec/Nsec when building ATime
sys.Atimespec.Unix()does not exist on Darwin, so this file will not compile. Please construct the timestamp with theSec/Nsecfields explicitly.Apply this diff to fix the build:
return DirEntry{ FileMode: ModeFromOS(fi.Mode()), - ATime: time.Unix(sys.Atimespec.Unix()), + ATime: time.Unix(int64(sys.Atimespec.Sec), int64(sys.Atimespec.Nsec)), MTime: fi.ModTime(), Length: uint64(fi.Size()), EntryName: fi.Name(), UID: uname, GID: gname,vendor/github.com/DeedleFake/p9/dir.go (3)
3-35: Keep requests inside the served root
Dir.pathjust doesfilepath.Join, so inputs like../../Windows/System32break out of the share. Every method here relies on it, so this is a full sandbox bypass.dir := p9.Dir("C:\\crc") _, _ = dir.Open("../../Windows/System32", p9.OREAD) // escapes rootPlease harden
path()to validate the resolved path stays under the base and thread that error through all call sites (Stat, WriteStat, Open, Create, Remove, etc.).@@ -import ( - "errors" - "os" - "path/filepath" -) +import ( + "errors" + "os" + "path/filepath" + "strings" +) @@ -func (d Dir) path(p string) string { - return filepath.Join(string(d), filepath.FromSlash(p)) +func (d Dir) path(p string) (string, error) { + base := string(d) + joined := filepath.Join(base, filepath.FromSlash(p)) + rel, err := filepath.Rel(base, joined) + if err != nil { + return "", err + } + if rel == ".." || strings.HasPrefix(rel, ".."+string(os.PathSeparator)) { + return "", errors.New("path escapes base") + } + return joined, nil } @@ - fi, err := os.Stat(d.path(p)) + tpath, err := d.path(p) + if err != nil { + return DirEntry{}, err + } + fi, err := os.Stat(tpath)Apply the same pattern anywhere
d.pathis used.Also applies to: 41-126
39-68: Reject partial ATime/MTime updatesPassing only one of ATime/MTime drops the other to the zero time via
os.Chtimes, silently corrupting metadata.- 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 { + return errors.New("setting only one of ATime or MTime is not supported") + } + if ok1 && ok2 { + err := os.Chtimes(tpath, atime, mtime)
69-75: Validate rename targets stay in the same directory
filepath.Join(base, filepath.FromSlash(name))accepts inputs with separators and.., letting clients move files outsidebase.- name, ok := changes.Name() - if ok { - err := os.Rename(p, filepath.Join(base, filepath.FromSlash(name))) + name, ok := changes.Name() + if ok { + if name != filepath.Base(name) || strings.ContainsRune(name, os.PathSeparator) { + return errors.New("invalid name: must be a base name without path separators") + } + err := os.Rename(tpath, filepath.Join(base, name))vendor/github.com/DeedleFake/p9/proto/server.go (1)
3-8: Don’t dispatch after a failed ReceiveAfter
p.Receivefails you still call the handler with zeroedtmsg/tag, which can panic or send garbage, and you loop forever onnet.ErrClosed.-import ( +import ( + "errors" "io" "log" "net" "sync" ) @@ - if err != nil { - if err == io.EOF { - return - } - - log.Printf("Error reading message: %v", err) - } + if err != nil { + if err == io.EOF || errors.Is(err, net.ErrClosed) { + return + } + log.Printf("Error reading message: %v", err) + continue + }Also applies to: 69-99
vendor/github.com/DeedleFake/p9/proto/proto.go (1)
85-114: Fix 9P framing off-by-4
sizealready includes the 4-byte length field, so bounding the reader withN: sizelets reads spill into the next frame. Frames under 4 bytes should be rejected.- if (msize > 0) && (size > msize) { + if (msize > 0) && (size > msize) { return nil, NoTag, util.Errorf("receive: %w", ErrLargeMessage) } - - lr := &util.LimitedReader{ - R: r, - N: size, - E: ErrLargeMessage, - } + if size < 4 { + return nil, NoTag, util.Errorf("receive: invalid frame size: %d", size) + } + lr := &util.LimitedReader{ + R: r, + N: size - 4, + E: ErrLargeMessage, + }vendor/github.com/DeedleFake/p9/remote.go (3)
34-67: Produce valid Twalk componentsLeading slashes turn into empty
Wnameentries (["", "foo"]) and/becomes["/"], both invalid. Absolute roots should walk zero components; everything else must split into non-empty names.- w := []string{path.Clean(p)} - if w[0] != "/" { - w = strings.Split(w[0], "/") - } - if (len(w) == 1) && (w[0] == ".") { - w = nil - } + p = path.Clean(p) + var w []string + switch { + case p == "", p == ".": + // stay on current fid + case strings.HasPrefix(p, "/"): + p = strings.TrimPrefix(p, "/") + if p != "" { + w = strings.FieldsFunc(p, func(r rune) bool { return r == '/' }) + } + default: + w = strings.FieldsFunc(p, func(r rune) bool { return r == '/' }) + }
151-185: Return an error for invalid whenceLibrary code shouldn’t panic on bad input; return the current position and an error instead.
default: - panic(util.Errorf("Invalid whence: %v", whence)) + return int64(file.pos), util.Errorf("invalid whence: %v", whence) }
198-308: Guard negotiated msize before chunking
int(client.Msize() - IOHeaderSize)underflows when msize ≤ header, and ReadAt/WriteAt then loop with size≤0. Fail fast with a clear error.func (file *Remote) maxBufSize() int { - return int(file.client.Msize() - IOHeaderSize) + msize := file.client.Msize() + if msize <= uint32(IOHeaderSize) { + return 0 + } + return int(msize - uint32(IOHeaderSize)) } @@ func (file *Remote) ReadAt(buf []byte, off int64) (int, error) { - size := len(buf) - if size > file.maxBufSize() { - size = file.maxBufSize() - } + max := file.maxBufSize() + if max <= 0 { + return 0, util.Errorf("msize too small: %d", file.client.Msize()) + } + size := len(buf) + if size > max { + size = max + } @@ func (file *Remote) WriteAt(data []byte, off int64) (int, error) { - size := len(data) - if size > file.maxBufSize() { - size = file.maxBufSize() - } + max := file.maxBufSize() + if max <= 0 { + return 0, util.Errorf("msize too small: %d", file.client.Msize()) + } + size := len(data) + if size > max { + size = max + }vendor/github.com/DeedleFake/p9/stat.go (1)
3-12: Remove unsafe string conversion
return *(*string)(unsafe.Pointer(&buf))hands back a string referencing stack memory. Use the standard conversion and drop the unsafe import.-import ( - "bytes" - "errors" - "io" - "os" - "time" - "unsafe" +import ( + "bytes" + "errors" + "io" + "os" + "time" @@ - return *(*string)(unsafe.Pointer(&buf)) + return string(buf)Also applies to: 135-153
🧹 Nitpick comments (1)
vendor/github.com/DeedleFake/p9/internal/debug/nodebug.go (1)
1-5: LGTM! Vendored no-op debug implementation is correct.The no-op
Logfunction correctly provides zero-overhead debug logging when thep9debugbuild tag is not set. The implementation matches the expected pattern for conditional compilation of debug helpers.Note: The build constraint uses the older
// +buildsyntax rather than the newer//go:builddirective introduced in Go 1.17. While both syntaxes remain valid, the newer directive is preferred. However, since this is vendored code, any such improvements should be contributed upstream per the guidance from cfergeau.
📜 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 (42)
cmd/crc/cmd/daemon.go(5 hunks)cmd/crc/cmd/start.go(0 hunks)go.mod(1 hunks)packaging/windows/product.wxs.template(0 hunks)pkg/crc/config/settings.go(1 hunks)pkg/crc/constants/constants.go(1 hunks)pkg/crc/machine/driver.go(0 hunks)pkg/crc/machine/driver_darwin.go(0 hunks)pkg/crc/machine/driver_linux.go(0 hunks)pkg/crc/machine/driver_windows.go(0 hunks)pkg/crc/machine/libhvee/driver_windows.go(2 hunks)pkg/crc/machine/start.go(2 hunks)pkg/drivers/libhvee/libhvee_windows.go(0 hunks)pkg/drivers/libhvee/powershell_windows.go(0 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 with no reviewable changes (8)
- pkg/crc/machine/driver_darwin.go
- pkg/drivers/libhvee/powershell_windows.go
- cmd/crc/cmd/start.go
- pkg/crc/machine/driver.go
- pkg/drivers/libhvee/libhvee_windows.go
- packaging/windows/product.wxs.template
- pkg/crc/machine/driver_linux.go
- pkg/crc/machine/driver_windows.go
✅ 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)
- go.mod
- pkg/crc/config/settings.go
- vendor/github.com/DeedleFake/p9/dir_linux.go
- vendor/github.com/DeedleFake/p9/dir_windows.go
- vendor/github.com/DeedleFake/p9/encoding.go
- pkg/fileserver/fs9p/server.go
- vendor/github.com/DeedleFake/p9/internal/util/util.go
- vendor/github.com/DeedleFake/p9/internal/debug/debug.go
- vendor/github.com/DeedleFake/p9/addr_other.go
- vendor/github.com/DeedleFake/p9/fs.go
- pkg/crc/machine/libhvee/driver_windows.go
- pkg/crc/constants/constants.go
- vendor/github.com/DeedleFake/p9/addr_unix.go
- vendor/github.com/DeedleFake/p9/msg.go
- vendor/github.com/DeedleFake/p9/client.go
- vendor/github.com/DeedleFake/p9/p9.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 (14)
pkg/crc/machine/start.go (1)
pkg/os/exec.go (1)
RunPrivileged(48-59)
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/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/internal/debug/nodebug.go (1)
vendor/github.com/DeedleFake/p9/internal/debug/debug.go (1)
Log(10-12)
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)
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/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.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/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/proto.go (3)
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/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/dir_other.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)
🪛 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). (2)
- GitHub Check: build (macOS-14, 1.24)
- GitHub Check: Konflux kflux-prd-rh02 / crc-binary-on-pull-request
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
pkg/crc/machine/start.go (1)
244-251: 9P mount: still non‑privileged, wrong argv, missing options/aname (tag)Mount needs root and proper 9P opts. Also split argv; passing
"chown core:core"and"9pfs 192.168.127.1"as single tokens will fail lookup. Use explicit mount with aname=mount.Tag and constants.Apply:
case "9p": - // change owner to core user to allow mounting to it as a non-root user - if _, _, err := sshRunner.RunPrivileged("Changing owner of mount directory", "chown core:core", mount.Target); err != nil { - return err - } - if _, _, err := sshRunner.Run("9pfs 192.168.127.1", mount.Target); err != nil { - return err - } + // change owner to core user to allow non-root access after mounting + if _, _, err := sshRunner.RunPrivileged("Changing owner of mount directory", "chown", "core:core", mount.Target); err != nil { + return err + } + opts := fmt.Sprintf("trans=tcp,port=%d,msize=%d,cache=loose,version=9p2000.L,aname=%s", + constants.Plan9TcpPort, constants.Plan9Msize, mount.Tag) + if _, _, err := sshRunner.RunPrivileged( + fmt.Sprintf("Mounting %s (9p tag %s)", mount.Target, mount.Tag), + "mount", "-t", "9p", "-o", opts, "192.168.127.1", mount.Target, + ); err != nil { + return err + }Optional follow-ups:
- Idempotency: skip if already mounted (e.g.,
mountpoint -q), or unmount/retry. I can propose a patch.
🧹 Nitpick comments (1)
pkg/fileserver/fs9p/server.go (1)
3-13: Ignore net.ErrClosed on shutdown in WaitForErrorClosing the listener causes proto.Serve to return net.ErrClosed; that shouldn’t surface as an error after Stop(). Handle it explicitly.
@@ -import ( +import ( + "errors" "fmt" "net" "os" "path/filepath" @@ -func (s *Server) WaitForError() error { - err := <-s.ErrChan - return err -} +func (s *Server) WaitForError() error { + if err, ok := <-s.ErrChan; ok { + if errors.Is(err, net.ErrClosed) { + return nil + } + return err + } + return nil +}Also applies to: 84-88
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
cmd/crc/cmd/daemon.go(5 hunks)cmd/crc/cmd/start.go(0 hunks)packaging/windows/product.wxs.template(0 hunks)pkg/crc/config/settings.go(1 hunks)pkg/crc/constants/constants.go(1 hunks)pkg/crc/machine/driver.go(0 hunks)pkg/crc/machine/driver_darwin.go(0 hunks)pkg/crc/machine/driver_linux.go(0 hunks)pkg/crc/machine/driver_windows.go(0 hunks)pkg/crc/machine/libhvee/driver_windows.go(2 hunks)pkg/crc/machine/start.go(2 hunks)pkg/drivers/libhvee/libhvee_windows.go(0 hunks)pkg/drivers/libhvee/powershell_windows.go(0 hunks)pkg/fileserver/fs9p/server.go(1 hunks)
💤 Files with no reviewable changes (8)
- pkg/crc/machine/driver_windows.go
- cmd/crc/cmd/start.go
- packaging/windows/product.wxs.template
- pkg/crc/machine/driver.go
- pkg/crc/machine/driver_darwin.go
- pkg/drivers/libhvee/powershell_windows.go
- pkg/crc/machine/driver_linux.go
- pkg/drivers/libhvee/libhvee_windows.go
🚧 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 (4)
pkg/crc/machine/start.go (1)
pkg/os/exec.go (1)
RunPrivileged(48-59)
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/config/settings.go (2)
pkg/crc/config/validations.go (1)
ValidateBool(19-25)pkg/crc/config/callbacks.go (1)
SuccessfullyApplied(36-38)
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). (9)
- GitHub Check: build (ubuntu-latest, 1.24)
- GitHub Check: build (macOS-14, 1.24)
- GitHub Check: build-qe (darwin, arm64)
- GitHub Check: build-qe (linux, amd64)
- GitHub Check: build (ubuntu-latest, 1.24)
- GitHub Check: build (ubuntu-22.04, 1.24)
- GitHub Check: build (macOS-13, 1.24)
- GitHub Check: Run OKD bundle with crc (1.24)
- GitHub Check: Konflux kflux-prd-rh02 / crc-binary-on-pull-request
🔇 Additional comments (2)
cmd/crc/cmd/daemon.go (2)
182-196: LGTM: use net.JoinHostPortSafer and clearer than manual fmt for host:port.
198-212: LGTM: host virtual IP listener uses net.JoinHostPortConsistent, correct.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
packaging/windows/product.wxs.template (1)
69-73: AddWin64="yes"to prevent registry redirection on 64-bit Windows.This component registers a HVSOCK registry key for Guest Communication Services. Without the
Win64="yes"attribute, the registry entry may be redirected toWow6432Nodeon 64-bit Windows, preventing the 64-bit Guest Communication Services from discovering and using the 9P endpoint.- <Component Id="Hvsock9pRegistryEntry" Guid="*"> + <Component Id="Hvsock9pRegistryEntry" Guid="*" Win64="yes"> <RegistryKey Root="HKLM" Key="Software\Microsoft\Windows NT\CurrentVersion\Virtualization\GuestCommunicationServices\00009000-FACB-11E6-BD58-64006A7986D3"> <RegistryValue Type="string" Name="ElementName" Value="fs9p-hvsock" KeyPath="yes"/> </RegistryKey> </Component>pkg/crc/machine/start.go (1)
249-249: Fix argv tokenization and clarify the "2" argument.The first argument
"9pfs -V -p"is passed as a single string, which will cause binary lookup to fail sinceRun()expects separate argv tokens. Additionally, the"2"argument between the port and mount target is unclear.Apply this diff to fix the argv tokenization:
- if _, _, err := sshRunner.Run("9pfs -V -p", fmt.Sprintf("%d", constants.Plan9HvsockPort), "2", mount.Target); err != nil { + if _, _, err := sshRunner.Run("9pfs", "-V", "-p", fmt.Sprintf("%d", constants.Plan9HvsockPort), mount.Target); err != nil {If the
"2"represents a tag/share index or other parameter, please document its purpose or verify it's needed.
🧹 Nitpick comments (1)
pkg/fileserver/fs9p/server.go (1)
90-98: Consider checking fornet.ErrCloseddirectly instead of string matching.The current implementation uses string matching on
net.OpErrorto detect shutdown errors. This is fragile compared to checkingerrors.Is(err, net.ErrClosed). If the Windows-specific error differs fromnet.ErrClosed, document why string matching is necessary.Consider this more robust approach:
func (s *Server) WaitForError() error { err := <-s.ErrChan - // captures "accept tcp: endpoint is in invalid state" errors on exit + // Treat listener closed errors as normal shutdown + if errors.Is(err, net.ErrClosed) { + return nil + } + // Windows-specific: captures "accept tcp: endpoint is in invalid state" errors var opErr *net.OpError if errors.As(err, &opErr) && strings.Contains(opErr.Error(), "endpoint is in invalid state") { return nil } return err }If the string matching is required for Windows-specific errors, add a comment explaining why.
📜 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 (17)
cmd/crc/cmd/daemon.go(5 hunks)cmd/crc/cmd/start.go(0 hunks)go.mod(2 hunks)packaging/windows/product.wxs.template(2 hunks)pkg/crc/config/settings.go(1 hunks)pkg/crc/constants/constants.go(1 hunks)pkg/crc/machine/driver.go(0 hunks)pkg/crc/machine/driver_darwin.go(0 hunks)pkg/crc/machine/driver_linux.go(0 hunks)pkg/crc/machine/driver_windows.go(0 hunks)pkg/crc/machine/libhvee/driver_windows.go(2 hunks)pkg/crc/machine/start.go(2 hunks)pkg/drivers/libhvee/libhvee_windows.go(0 hunks)pkg/drivers/libhvee/powershell_windows.go(0 hunks)pkg/fileserver/fs9p/server.go(1 hunks)pkg/fileserver/fs9p/server_fallback.go(1 hunks)pkg/fileserver/fs9p/server_windows.go(1 hunks)
💤 Files with no reviewable changes (7)
- pkg/crc/machine/driver.go
- pkg/crc/machine/driver_darwin.go
- pkg/drivers/libhvee/libhvee_windows.go
- pkg/drivers/libhvee/powershell_windows.go
- pkg/crc/machine/driver_windows.go
- pkg/crc/machine/driver_linux.go
- cmd/crc/cmd/start.go
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/crc/config/settings.go
- pkg/crc/constants/constants.go
- pkg/crc/machine/libhvee/driver_windows.go
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: redbeam
Repo: crc-org/crc PR: 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
Repo: crc-org/crc PR: 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/server_windows.gopkg/crc/machine/start.gopkg/fileserver/fs9p/server_fallback.gocmd/crc/cmd/daemon.gopkg/fileserver/fs9p/server.gogo.mod
🧬 Code graph analysis (5)
pkg/fileserver/fs9p/server_windows.go (1)
pkg/fileserver/fs9p/server_fallback.go (1)
GetHvsockListener(13-15)
pkg/crc/machine/start.go (3)
pkg/os/exec.go (1)
RunPrivileged(48-59)pkg/crc/constants/constants.go (2)
Plan9HvsockPort(62-62)VSockGateway(42-42)pkg/crc/logging/logging.go (1)
Warnf(100-102)
pkg/fileserver/fs9p/server_fallback.go (1)
pkg/fileserver/fs9p/server_windows.go (1)
GetHvsockListener(14-29)
cmd/crc/cmd/daemon.go (6)
pkg/crc/config/settings.go (1)
EnableSharedDirs(33-33)pkg/fileserver/fs9p/server_fallback.go (1)
GetHvsockListener(13-15)pkg/fileserver/fs9p/server_windows.go (1)
GetHvsockListener(14-29)pkg/crc/constants/constants.go (3)
Plan9HvsockGUID(61-61)GetHomeDir(166-172)Plan9TcpPort(60-60)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). (8)
- GitHub Check: build (macOS-13, 1.24)
- GitHub Check: build (macOS-14, 1.24)
- GitHub Check: build (macOS-13, 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 (windows-2022, 1.24)
- GitHub Check: Konflux kflux-prd-rh02 / crc-binary-on-pull-request
🔇 Additional comments (8)
packaging/windows/product.wxs.template (1)
111-111: Component reference correctly added to feature.The
Hvsock9pRegistryEntryreference is properly included inDefaultFeature, ensuring the 9P registry entry is installed as part of the core installation. Once theWin64attribute issue above is resolved, this change is good to go.go.mod (1)
7-7: LGTM! 9P dependencies properly added.The addition of
github.com/DeedleFake/p9and promotion ofgithub.com/linuxkit/virtsockto direct dependencies correctly support the 9P-based file sharing implementation that replaces SMB on Windows.Also applies to: 31-31
pkg/fileserver/fs9p/server_fallback.go (1)
1-15: LGTM! Standard cross-platform fallback.The non-Windows stub implementation correctly returns a clear not-implemented error and follows standard Go cross-platform patterns.
pkg/fileserver/fs9p/server_windows.go (1)
14-29: LGTM! Windows hvsock listener properly implemented.The Windows-specific implementation correctly parses the hvsock GUID, creates a listener with
GUIDWildcardVMID (accepting connections from any VM), and provides proper error wrapping.cmd/crc/cmd/daemon.go (2)
182-182: LGTM! Cleaner address construction.Using
net.JoinHostPortis more idiomatic and explicit thanfmt.Sprintffor constructing network addresses.Also applies to: 198-198
253-300: LGTM! Windows 9P server setup properly implemented.The Windows-specific 9P server setup correctly:
- Gates on
EnableSharedDirsconfig and Windows OS- Provides both hvsock (primary) and TCP (backup) transport options
- Manages server lifecycle with proper
Start(),defer Stop(), and error monitoring goroutines- Logs errors appropriately
All concerns from previous reviews have been addressed.
pkg/fileserver/fs9p/server.go (2)
34-58: LGTM! Server initialization properly validates inputs.The constructor correctly validates that
exposeDiris absolute and exists as a directory, creates a buffered error channel to prevent goroutine blocking, and initializes the 9P filesystem wrapper.
61-75: LGTM! Server startup with immediate error check.The
Start()method launches the server asynchronously and performs an immediate error check viaselectwithdefault. While this creates a narrow race window where errors occurring after the check won't be returned here, the pattern is acceptable since callers useWaitForError()in a background goroutine to catch runtime errors.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (20)
pkg/fileserver/fs9p/server.go (1)
89-98: Consider also treatingnet.ErrClosedas a clean shutdown in WaitForErrorRight now
WaitForErrorspecial-cases only*net.OpErrorwhose string contains “endpoint is in invalid state”. For normal listeners closed viaStop,Acceptmay returnnet.ErrClosed, which you probably also want to treat as a non-error.You can do this cheaply by adding an
errors.Is(err, net.ErrClosed)check before the string match:func (s *Server) WaitForError() error { - 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 + err := <-s.ErrChan + if errors.Is(err, net.ErrClosed) { + return nil + } + // 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 }This keeps the hvsock-specific workaround while also handling the standard closed-listener case more cleanly.
pkg/crc/machine/libhvee/driver_windows.go (1)
21-33: 9P tag generation: keep it deterministic and in sync with server share namesUsing
fmt.Sprintf("dir%d", i)is fine as long as the 9P server uses the same tag naming scheme and the order ofmachineConfig.SharedDirsis stable. To reduce coupling and surprises:
- Derive the tag format from a shared constant/helper used by both the client and the 9P server.
- Optionally sort a copy of
machineConfig.SharedDirsbefore tagging to make tag assignment deterministic across runs.Not a blocker, but worth aligning to avoid subtle mount issues if ordering changes.
vendor/github.com/DeedleFake/p9/proto/encoding.go (1)
33-38: Harden proto (de)serialization: guardRead, bound-check lengths, and fixSizefor stringsThere are a few correctness issues here that are worth addressing:
Readcan panic on non-pointer or nilv
decodeunconditionally takesv.Addr()afterreflect.Indirect, which panics ifvis not a non-nil pointer. Add a fast guard:func Read(r io.Reader, v any) error {
- d := &decoder{r: r}
- d.decode(reflect.ValueOf(v))
- return d.err
- rv := reflect.ValueOf(v)
- if rv.Kind() != reflect.Ptr || rv.IsNil() {
return util.Errorf("proto.Read expects a non-nil pointer, got %T", v)- }
- d := &decoder{r: r}
- d.decode(rv)
- return d.err
}2. **Slice and string lengths silently wrap at 64 KiB** Non-`[]byte` slices and strings are length-prefixed with `uint16`, but there’s no bounds checking: ```go case reflect.Array, reflect.Slice: switch v.Type().Elem().Kind() { case reflect.Uint8: e.mode(uint32(v.Len())) default: e.mode(uint16(v.Len())) } ... case reflect.String: e.mode(uint16(v.Len())) e.mode([]byte(v.String()))For lengths > 65535 this will wrap and produce invalid on-wire data. You can fail fast instead:
case reflect.Array, reflect.Slice: switch v.Type().Elem().Kind() { case reflect.Uint8: e.mode(uint32(v.Len())) default: - e.mode(uint16(v.Len())) + if v.Len() > 0xFFFF { + e.err = util.Errorf("slice too long: %d elements", v.Len()) + return + } + e.mode(uint16(v.Len())) } ... case reflect.String: - e.mode(uint16(v.Len())) - e.mode([]byte(v.String())) + if v.Len() > 0xFFFF { + e.err = util.Errorf("string too long: %d bytes", v.Len()) + return + } + e.mode(uint16(v.Len())) + s := v.String() + if e.w == nil { + // Size mode: account for string bytes without using binary.Size on []byte. + e.n += uint32(len(s)) + } else { + e.mode([]byte(s)) + }
Sizeis incorrect for values containing strings
As written,Size()usesbinary.Sizeunder the hood. For strings, the branche.mode([]byte(v.String()))causesbinary.Sizeto see a slice and return-1, soSizeends up accumulating^uint32(0)instead of the actual string length. The adjusted string branch above fixes this by counting bytes directly in “size mode” (e.w == nil) rather than routing throughbinary.Sizeon a slice.These changes keep the API the same while preventing panics and subtle on-wire corruption for large payloads.
Also applies to: 81-109, 124-183
vendor/github.com/DeedleFake/p9/client.go (3)
61-69: Unchecked type assertion can panic on protocol errors.The direct type assertion
rsp.(*Rversion)will panic if the server returns an unexpected response type (e.g.,*Rerror). A guarded assertion would be safer.This issue was previously flagged. Since this is vendored code, consider filing an upstream PR if fixes are feasible, as recommended by cfergeau in the PR comments.
84-91: Unchecked type assertion in Auth can panic.Same issue as Handshake:
rsp.(*Rauth)should use a guarded assertion to handle protocol errors gracefully.
113-120: Unchecked type assertion in Attach can panic.
rsp.(*Rattach)should use a guarded assertion to prevent panics on unexpected response types.vendor/github.com/DeedleFake/p9/proto/client.go (1)
138-145: Tag allocation can emit reserved NoTag and spin indefinitely.The loop at lines 139-144 doesn't skip
NoTag(0xFFFF) and will spin forever when all tags are in use. Per the 9P protocol, NoTag is reserved and must never be used for tagged messages.This issue was previously flagged. The suggested fix was to skip NoTag explicitly and bail out or wait when the full tag space is exhausted.
vendor/github.com/DeedleFake/p9/proto/server.go (1)
69-99: Handler called with invalid message after Receive error.After a non-EOF
Receiveerror at line 71, the code logs the error at line 76 but then falls through to line 79, calling the handler with a potentially garbagetmsgandtag. This can cause panics or send invalid responses. Acontinuestatement is needed after logging.This issue was previously flagged. The fix should add
continueafter line 76 to avoid dispatching invalid messages.if err != nil { if err == io.EOF { return } log.Printf("Error reading message: %v", err) + continue }vendor/github.com/DeedleFake/p9/fs.go (2)
244-249: Tflush should respond with Rflush per 9P spec.The 9P protocol specification requires that Tflush always receives an Rflush response, even if the flush cannot actually cancel in-flight operations. Returning Rerror violates the protocol and may cause client misbehavior.
This was previously flagged and marked as addressed, but the code still returns
&Rerror. The fix should return&Rflush{}with a comment noting best-effort semantics.func (h *fsHandler) flush(msg *Tflush) any { - // TODO: Implement this. - return &Rerror{ - Ename: "flush is not supported", - } + // TODO: Track and cancel in-flight ops by tag. + // Best-effort: we acknowledge the flush but don't actually cancel operations. + return &Rflush{} }
593-599: Type assertionrsp.(error)never matches; clunk errors are silently ignored.
*Rerrordoes not implement theerrorinterface, so the check at line 596 always fails. This means clunk errors fromh.clunk()are never propagated, andremove()proceeds even when clunk fails.rsp := h.clunk(&Tclunk{ FID: msg.FID, }) - if _, ok := rsp.(error); ok { + if _, ok := rsp.(*Rerror); ok { return rsp }vendor/github.com/DeedleFake/p9/remote.go (5)
34-71: walk() sends invalid Wname for root path "/".When
pis "/" or an absolute path,path.Clean(p)returns "/", and sincew[0] == "/", the code skips the split and sendsWname = ["/"]. The 9P Twalk message requires Wname elements to be valid path components (no slashes, no empty strings). Sending "/" as a name element violates the protocol.This was previously flagged and marked as addressed, but the logic appears unchanged. The fix should handle root path and absolute paths correctly:
- w := []string{path.Clean(p)} - if w[0] != "/" { - w = strings.Split(w[0], "/") - } - if (len(w) == 1) && (w[0] == ".") { - w = nil - } + p = path.Clean(p) + var w []string + switch { + case p == "" || p == "." || p == "/": + w = nil + default: + p = strings.TrimPrefix(p, "/") + w = strings.Split(p, "/") + }
183-185: Panic on invalid whence; return error instead.Library code should not panic for invalid input. Returning an error allows callers to handle the situation gracefully.
- panic(util.Errorf("Invalid whence: %v", whence)) + return int64(file.pos), util.Errorf("invalid whence: %v", whence)
198-200: maxBufSize() can underflow or return non-positive value.If
Msize()is less than or equal toIOHeaderSize(24), the subtraction underflows (uint32 wrap) or returns 0/negative after int conversion. This causesReadAt/WriteAtloops to hang or behave incorrectly.This was previously flagged. The fix should guard against non-positive results:
func (file *Remote) maxBufSize() int { - return int(file.client.Msize() - IOHeaderSize) + m := file.client.Msize() + if m <= uint32(IOHeaderSize) { + return 1 // Avoid infinite loop; callers should validate msize at handshake + } + return int(m - uint32(IOHeaderSize)) }
229-243: ReadAt loop can hang if maxBufSize() returns 0.When
min(len(buf), file.maxBufSize())returns 0, the loop makes no progress (start += 0), causing an infinite loop. Add a guard to bail early.func (file *Remote) ReadAt(buf []byte, off int64) (int, error) { size := min(len(buf), file.maxBufSize()) + if size <= 0 { + return 0, util.Errorf("msize too small: %d", file.client.Msize()) + } var total int
282-296: WriteAt loop has same infinite-loop risk as ReadAt.Same issue: if
maxBufSize()returns 0, the loop hangs. Apply the same guard.func (file *Remote) WriteAt(data []byte, off int64) (int, error) { size := min(len(data), file.maxBufSize()) + if size <= 0 { + return 0, util.Errorf("msize too small: %d", file.client.Msize()) + } var total intvendor/github.com/DeedleFake/p9/proto/proto.go (1)
85-94: Off-by-4 framing bug remains unresolved in vendored library.The LimitedReader is still bounded to
sizeinstead ofsize-4. Since the 9P size field includes its own 4 bytes, the current code allows reads to leak 4 bytes into the next frame, breaking message framing.As noted in the PR discussion, vendored code issues should be fixed upstream. Please file a PR with the DeedleFake/p9 project to bound reads correctly:
N: size - 4with a guard forsize < 4.vendor/github.com/DeedleFake/p9/dir.go (3)
18-20: Critical path traversal vulnerability remains in vendored Dir implementation.The
path()helper still usesfilepath.Joinwithout validation, allowing ".." to escape the served root directory. Every filesystem operation (Stat, WriteStat, Open, Create, Remove) is affected and can access files outside the intended share.For example: accessing
/share/../../etc/passwdwould resolve to/etc/passwd, bypassing the share boundary.Given this is vendored code, please file an upstream PR with DeedleFake/p9 to add path validation (e.g., using
filepath.Relto detect ".." escape attempts and rejecting them).Also applies to: 22-35, 41-51, 61-68, 69-75, 95-103, 105-122, 124-127
52-60: Partial timestamp updates can corrupt file times.When only one of ATime or MTime is provided,
os.Chtimesis called with a zero-value for the other timestamp, which can clobber the unset field instead of preserving it.Consider filing an upstream PR to reject partial updates (return error when
ok1 != ok2) or fetch current values for missing timestamps.
69-75: Name change allows path traversal via directory separators.Using
filepath.FromSlash(name)without validation allows slashes or ".." in the new name to move files outside the current directory. 9P wstat name changes should only rename within the same directory.Please file an upstream PR to validate that
namecontains no path separators and equalsfilepath.Base(name).vendor/github.com/DeedleFake/p9/stat.go (1)
135-153: Unsafe string conversion from stack slice should use safe copy.Line 152 uses
unsafe.String(unsafe.SliceData(buf), len(buf))which creates a string header pointing to the stack-allocated slice. If the returned string escapes and the stack frame is reused, this can cause memory corruption or read invalid data.Please file an upstream PR to change line 152 to
return string(buf)for a safe heap copy.
🧹 Nitpick comments (3)
vendor/github.com/DeedleFake/p9/README.md (1)
1-51: Vendored README markdown issues are better handled via tooling configmarkdownlint’s heading-style and hard-tab warnings are against upstream vendored content; I’d avoid editing this README just for style and instead configure your markdown tooling to ignore
vendor/**if these warnings are noisy.pkg/crc/constants/constants.go (1)
59-62: Plan9 constants look fine; consider documenting msize usageValues are sensible for 9P (1 MiB msize, port 564, fixed GUID/port). It may help future readers to add a short comment that
Plan9Msizeis the negotiated 9P message size and should match both server and guest configuration.vendor/github.com/DeedleFake/p9/proto/client.go (1)
90-97: Read errors (except EOF) are silently swallowed, continuing the loop.When
Receivefails with a non-EOF error (e.g., timeout from the 10-second deadline at line 80), the error is ignored and the loop continues at line 96. While timeouts are expected, other transient errors may hide real problems. Consider logging non-timeout errors or distinguishing error types.
📜 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 (48)
cmd/crc/cmd/daemon.go(5 hunks)cmd/crc/cmd/start.go(0 hunks)go.mod(2 hunks)packaging/windows/product.wxs.template(2 hunks)pkg/crc/config/settings.go(1 hunks)pkg/crc/constants/constants.go(1 hunks)pkg/crc/machine/driver.go(0 hunks)pkg/crc/machine/driver_darwin.go(0 hunks)pkg/crc/machine/driver_linux.go(0 hunks)pkg/crc/machine/driver_windows.go(0 hunks)pkg/crc/machine/libhvee/driver.go(1 hunks)pkg/crc/machine/libhvee/driver_test.go(1 hunks)pkg/crc/machine/libhvee/driver_windows.go(2 hunks)pkg/crc/machine/start.go(2 hunks)pkg/drivers/libhvee/libhvee_windows.go(0 hunks)pkg/drivers/libhvee/powershell_windows.go(0 hunks)pkg/fileserver/fs9p/server.go(1 hunks)pkg/fileserver/fs9p/server_fallback.go(1 hunks)pkg/fileserver/fs9p/server_windows.go(1 hunks)test/e2e/features/9pfs.feature(1 hunks)test/e2e/testsuite/testsuite.go(3 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 with no reviewable changes (7)
- cmd/crc/cmd/start.go
- pkg/crc/machine/driver.go
- pkg/crc/machine/driver_linux.go
- pkg/drivers/libhvee/libhvee_windows.go
- pkg/drivers/libhvee/powershell_windows.go
- pkg/crc/machine/driver_darwin.go
- pkg/crc/machine/driver_windows.go
✅ Files skipped from review due to trivial changes (2)
- vendor/github.com/DeedleFake/p9/LICENSE
- vendor/github.com/DeedleFake/p9/addr_unix.go
🚧 Files skipped from review as they are similar to previous changes (12)
- vendor/github.com/DeedleFake/p9/dir_other.go
- vendor/github.com/DeedleFake/p9/internal/debug/debug.go
- pkg/crc/machine/start.go
- vendor/github.com/DeedleFake/p9/addr.go
- vendor/github.com/DeedleFake/p9/dir_linux.go
- test/e2e/testsuite/testsuite.go
- vendor/github.com/DeedleFake/p9/doc.go
- test/e2e/features/9pfs.feature
- vendor/github.com/DeedleFake/p9/dir_windows.go
- pkg/fileserver/fs9p/server_windows.go
- vendor/modules.txt
- vendor/github.com/DeedleFake/p9/addr_other.go
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: redbeam
Repo: crc-org/crc PR: 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
Repo: crc-org/crc PR: 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/server_fallback.gocmd/crc/cmd/daemon.govendor/github.com/DeedleFake/p9/README.mdpkg/fileserver/fs9p/server.govendor/github.com/DeedleFake/p9/p9.govendor/github.com/DeedleFake/p9/msg.govendor/github.com/DeedleFake/p9/fs.govendor/github.com/DeedleFake/p9/remote.gogo.mod
🧬 Code graph analysis (14)
pkg/fileserver/fs9p/server_fallback.go (1)
pkg/fileserver/fs9p/server_windows.go (1)
GetHvsockListener(14-29)
vendor/github.com/DeedleFake/p9/dir_darwin.go (2)
vendor/github.com/DeedleFake/p9/stat.go (3)
DirEntry(255-267)FileMode(23-23)ModeFromOS(43-78)vendor/github.com/DeedleFake/p9/p9.go (3)
QID(42-46)QIDType(49-49)Version(10-10)
pkg/crc/machine/libhvee/driver_test.go (1)
pkg/crc/machine/libhvee/driver.go (1)
ConvertToUnixPath(9-20)
pkg/crc/config/settings.go (2)
pkg/crc/config/validations.go (1)
ValidateBool(19-25)pkg/crc/config/callbacks.go (1)
SuccessfullyApplied(36-38)
vendor/github.com/DeedleFake/p9/proto/encoding.go (1)
vendor/github.com/DeedleFake/p9/internal/util/util.go (1)
Errorf(44-52)
pkg/fileserver/fs9p/server.go (4)
vendor/github.com/DeedleFake/p9/fs.go (2)
FileSystem(21-34)FSConnHandler(162-167)vendor/github.com/DeedleFake/p9/dir.go (1)
Dir(16-16)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/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/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/internal/debug/nodebug.go (1)
vendor/github.com/DeedleFake/p9/internal/debug/debug.go (1)
Log(11-13)
vendor/github.com/DeedleFake/p9/p9.go (1)
vendor/github.com/DeedleFake/p9/stat.go (1)
FileMode(23-23)
vendor/github.com/DeedleFake/p9/fs.go (4)
vendor/github.com/DeedleFake/p9/stat.go (2)
DirEntry(255-267)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/internal/debug/debug.go (1)
Log(11-13)vendor/github.com/DeedleFake/p9/internal/debug/nodebug.go (1)
Log(6-6)
pkg/crc/machine/libhvee/driver_windows.go (2)
vendor/github.com/crc-org/machine/libmachine/drivers/base.go (1)
SharedDir(27-35)pkg/crc/machine/libhvee/driver.go (1)
ConvertToUnixPath(9-20)
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 (2)
NewClient(30-32)Client(22-26)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/p9.go (1)
NoTag(15-15)
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: Konflux kflux-prd-rh02 / crc-binary-on-pull-request
- GitHub Check: build (ubuntu-latest, 1.24)
- GitHub Check: build (ubuntu-22.04, 1.24)
- GitHub Check: build (macOS-26, 1.24)
- GitHub Check: build (ubuntu-latest, 1.24)
- GitHub Check: build (macOS-15, 1.24)
- GitHub Check: verify-devcontainer
- GitHub Check: build-qe (linux, amd64)
- GitHub Check: build (macOS-15-intel, 1.24)
- GitHub Check: build (macOS-15, 1.24)
- GitHub Check: Run OKD bundle with crc (1.24)
- GitHub Check: build-qe (windows, amd64)
- GitHub Check: build-qe (darwin, arm64)
- GitHub Check: build-qe (darwin, amd64)
- GitHub Check: build (windows-2022, 1.24)
- GitHub Check: build-qe (linux, arm64)
- GitHub Check: build-installer (windows-2022, 1.24)
🔇 Additional comments (18)
vendor/github.com/DeedleFake/p9/internal/debug/nodebug.go (1)
1-6: Build tags and no-op debug logger look correctThe
!p9debugbuild constraints and the no-opLogimplementation cleanly mirror the real logger indebug.goand should work as intended to strip debug output in normal builds. No issues found.go.mod (1)
5-31: 9P and virtsock dependency wiring looks consistentAdding
github.com/DeedleFake/p9and makinggithub.com/linuxkit/virtsocka direct requirement matches the new 9P/hvsock server usage; nothing else needed here.pkg/crc/config/settings.go (1)
87-88: EnableSharedDirs config text and default align with 9P behaviorThe unified
EnableSharedDirssetting and updated description (“into the CRC VM”) match the new 9P-based sharing semantics; this looks good.vendor/github.com/DeedleFake/p9/encoding.go (1)
1-42: ReadDir/WriteDir implementations are straightforward and correctEOF handling, error propagation, and
Stat↔DirEntryconversions look sane for these helpers; fine to keep as-is in the vendored library.pkg/fileserver/fs9p/server.go (1)
31-75: 9P server construction and startup flow look solidValidating an absolute, existing directory, wrapping it as
p9.Dir, and using a bufferedErrChanto carry theproto.Serveresult is a good pattern; startup behavior and logging look reasonable.packaging/windows/product.wxs.template (1)
69-73: 9P hvsock GCS registry entry and feature wiring look correctThe
Hvsock9pRegistryEntrycomponent and itsDefaultFeaturereference cleanly register thefs9p-hvsockGuestCommunicationService under the expected GUID, matching the new Windows 9P/hvsock flow.Also applies to: 111-111
pkg/crc/machine/libhvee/driver.go (1)
8-19: ConvertToUnixPath correctly mirrors Podman’s/mnt/<drive>/...mappingThe normalization logic (ToSlash + drive-letter detection +
/mnt/<lowercase>prefix) matches Podman’s expectations for Windows paths; this is a good shared helper for 9P-based mounts.pkg/fileserver/fs9p/server_fallback.go (1)
11-15: Non-Windows GetHvsockListener stub is appropriateThe
!windowsbuild-tagged stub makes the API available but clearly reports that hvsock isn’t implemented off-Windows, which is the right trade-off here.pkg/crc/machine/libhvee/driver_test.go (1)
9-11: Tests correctly cover Windows path conversion casesBoth upper- and lower-case drive letters are validated against
ConvertToUnixPath, matching the helper’s behavior. Looks good.cmd/crc/cmd/daemon.go (1)
182-201: 9P daemon wiring and listeners look correctSwitching to
net.JoinHostPortfor the vn listeners is cleaner and avoids manual string formatting. The Windows-only 9P setup is properly gated onEnableSharedDirs, starts both hvsock and TCP servers against the home directory, and ensures cleanup viaStop()plus backgroundWaitForError()logging. No issues from a lifecycle or gating standpoint.Also applies to: 253-300
vendor/github.com/DeedleFake/p9/dir_plan9.go (1)
1-49: Plan9 DirEntry/QID helpers are straightforward and safeThe Plan9-specific
infoToEntryandGetQIDimplementations safely handle the presence/absence of*syscall.Dirand map QID fields directly from the kernel struct. This looks correct and idiomatic for the platform.vendor/github.com/DeedleFake/p9/dir_darwin.go (1)
12-62: Darwin DirEntry/QID implementation looks consistent and robustThe Darwin-specific mapping correctly enriches
DirEntrywith ATime/UID/GID when*syscall.Stat_tis available and falls back gracefully otherwise.GetQID’s use ofModeFromOS(fi.Mode()).QIDType()andsys.Inois consistent with the other platform backends.vendor/github.com/DeedleFake/p9/client.go (1)
45-47: LGTM: Atomic FID generation.The
nextFIDimplementation correctly uses atomic operations for thread-safe FID allocation.vendor/github.com/DeedleFake/p9/proto/client.go (1)
35-56: LGTM: Client initialization and lifecycle.The client properly initializes channels, spawns reader/coord goroutines, and uses context cancellation for cleanup.
vendor/github.com/DeedleFake/p9/proto/server.go (1)
17-42: LGTM: Server connection handling.The
Servefunction properly handles connection lifecycle with deferred close, optional connect/disconnect hooks, and per-connection message handlers.vendor/github.com/DeedleFake/p9/fs.go (2)
179-186: Safe string-to-byte conversion using Go 1.20+ idiom.The code uses
unsafe.Slice(unsafe.StringData(p), len(p))which is the idiomatic Go 1.20+ way to convert a string to[]bytewithout copying. This is safe and avoids the undefined behavior of the older*(*[]byte)(unsafe.Pointer(&p))pattern that was previously flagged.
665-717: LGTM: Message dispatch and handler routing.The
HandleMessagefunction properly routes all 9P message types to their handlers with appropriate error handling for unknown types.vendor/github.com/DeedleFake/p9/remote.go (1)
339-341: LGTM: Readdir implementation.The
Readdirfunction correctly uses a buffered reader sized tomaxBufSize()for efficient directory reading.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (18)
vendor/github.com/DeedleFake/p9/addr.go (1)
41-48: IPv6 host:port parsing is broken.
strings.SplitN(addr, ":", 2)incorrectly parses IPv6 addresses like[fe80::1]:564, splitting at the first colon and mangling the address. This prevents IPv6 9P connections from working.This issue was flagged in a previous review. Consider filing an upstream PR to use
net.SplitHostPortfor correct parsing:+import "net" ... - parts := strings.SplitN(addr, ":", 2) - if len(parts) == 2 { - if (parts[1] == "9p") || (parts[1] == "9fs") { - parts[1] = standardPort - } - return "tcp", strings.Join(parts, ":") - } + if strings.Contains(addr, ":") { + if host, port, err := net.SplitHostPort(addr); err == nil { + switch port { + case "9p", "9fs": + port = standardPort + } + return "tcp", net.JoinHostPort(host, port) + } + }vendor/github.com/DeedleFake/p9/proto/proto.go (1)
85-94: Off-by-4 framing bug:LimitedReaderbound should besize-4.The 9P protocol's size field includes its own 4 bytes. After reading
size, the remaining frame issize-4bytes. UsingN: sizeallows reads to leak 4 bytes into the next frame, causing message framing errors.This was flagged in a previous review. Apply this fix upstream or locally:
+ if size < 4 { + return nil, NoTag, util.Errorf("receive: invalid frame size: %d", size) + } lr := &util.LimitedReader{ R: r, - N: size, + N: size - 4, E: ErrLargeMessage, }pkg/crc/machine/start.go (1)
249-255: Fix argv tokenization:"9pfs -V -p"passed as single token will fail.The
sshRunner.Runfunction expects the binary and arguments as separate parameters. Passing"9pfs -V -p"as a single string will cause the shell to look for a binary literally named"9pfs -V -p", which will fail.Apply this diff to fix the argument parsing:
- if _, _, err := sshRunner.Run("9pfs -V -p", fmt.Sprintf("%d", constants.Plan9HvsockPort), "2", mount.Target); err != nil { + if _, _, err := sshRunner.Run("9pfs", "-V", "-p", fmt.Sprintf("%d", constants.Plan9HvsockPort), "2", mount.Target); err != nil {vendor/github.com/DeedleFake/p9/proto/encoding.go (1)
85-100: Silent integer overflow for large slices/strings—data corruption risk.Non-byte slices and strings encode their length as
uint16, but there's no bounds check. Values exceeding 65535 elements/bytes will silently wrap, producing corrupted wire data.This is vendored code, but the issue is significant. Consider filing an upstream PR or adding local bounds checks:
case reflect.String: + if v.Len() > 0xFFFF { + e.err = util.Errorf("string too long: %d bytes", v.Len()) + return + } e.mode(uint16(v.Len())) e.mode([]byte(v.String()))cmd/crc/cmd/daemon.go (1)
253-300: Windows 9P home-dir sharing is correctly gated and cleaned up; error propagation could be enhancedThe 9P servers are:
- Only started on Windows when EnableSharedDirs is true.
- Started over both hvsock and TCP with separate listeners.
- Stopped via defers and monitored via WaitForError with appropriate logging.
This addresses earlier concerns about unconditional exposure and lack of cleanup. If you ever want 9P failures to be fatal for the daemon rather than just logged, you could route WaitForError results into errCh instead of (or in addition to) logging, but that’s an optional behavior change.
pkg/crc/machine/libhvee/driver_windows.go (1)
4-29: 9P share configuration is correct; consider stabilizing tag naming/orderUsing ConvertToUnixPath and Type "9p" aligns this driver with the new 9P backend. However, tags based on the loop index (
dir%d) depend on the order ofmachineConfig.SharedDirs; if that slice comes from an unordered source or is reordered, tag→path mapping can change and potentially diverge from the server’s expected aname list.Consider:
- Building a sorted copy of
machineConfig.SharedDirsbefore tagging, and/or- Centralizing the tag format in a shared helper/constant used by both the 9P server and this client-side configuration.
That would make tag naming deterministic and keep client/server naming in sync.
vendor/github.com/DeedleFake/p9/dir_darwin.go (1)
35-43: Duplicate: ATime construction may have compilation issueThis was already flagged in a previous review. The call
time.Unix(sys.Atimespec.Unix())relies onsyscall.Timespec.Unix()returning(sec, nsec)which is then passed directly totime.Unix(sec, nsec). This syntax is valid in Go when the return values match the function parameters exactly.However, since this is vendored code, any fixes should be submitted upstream as noted in the PR comments.
test/e2e/testsuite/testsuite.go (1)
589-600: Gate 9P test step registration to Windows platform.The 9P file sharing test steps call
libhvee.ConvertToUnixPath()unconditionally during step definition registration. While the feature file may be tagged@windows, step definitions are registered at initialization time regardless of platform tags. This is consistent with the pattern used elsewhere in this file (lines 972, 1036, 1043, 1084).Apply this diff:
// 9P file sharing checks + if runtime.GOOS == "windows" { mountedHomeDir := libhvee.ConvertToUnixPath(constants.GetHomeDir()) s.Step(`^home directory mount exists in VM$`, func() error { return directoryExistInVM(mountedHomeDir) }) s.Step(`^filesystem is mounted$`, filesystemIsMounted) s.Step(`^listing files in mounted home directory should succeed$`, func() error { return listingFilesInDirectoryShouldSucceed(mountedHomeDir) }) s.Step(`^basic file operations in mounted home directory should succeed$`, func() error { return basicFileOperationsInDirectoryShouldSucceed(mountedHomeDir) }) s.Step(`^basic directory operations in mounted home directory should succeed$`, func() error { return basicDirectoryOperationsInDirectoryShouldSucceed(mountedHomeDir) }) + }vendor/github.com/DeedleFake/p9/client.go (3)
61-69: Duplicate: Unguarded type assertion in Handshake can panicThis was already flagged in a previous review. The direct type assertion
rsp.(*Rversion)can panic if the server returns an unexpected response type. Since this is vendored code, any fix should be submitted upstream per the PR discussion.
84-91: Duplicate: Unguarded type assertion in Auth can panicThis was already flagged in a previous review. The direct type assertion
rsp.(*Rauth)can panic on unexpected response types.
113-120: Duplicate: Unguarded type assertion in Attach can panicThis was already flagged in a previous review. The direct type assertion
rsp.(*Rattach)can panic on protocol mismatches.vendor/github.com/DeedleFake/p9/proto/client.go (1)
138-145: Duplicate: Tag allocation can emit reserved NoTag and spin indefinitelyThis was already flagged in a previous review. The tag allocation loop doesn't skip the reserved
NoTagvalue (0xFFFF) and will spin forever if all tags are exhausted. Since this is vendored code, the fix should be submitted upstream.vendor/github.com/DeedleFake/p9/fs.go (2)
244-249: Vendored code: flush must respond with Rflush per 9P spec.The implementation returns
Rerrorbut the 9P specification requires responding withRflush. This was flagged in past reviews as addressed, but the code still showsRerror.If this is intended for upstream, the 9P spec requires acknowledging flush requests with
Rflush, even if cancellation is not supported.Based on learnings, file PRs upstream so fixes can propagate into vendored copies.
583-608: Vendored code: wrong type assertion misses clunk errors.Line 596 uses
rsp.(error)which will never match becauseclunk()returns*Rerrorfor errors, noterror. This means clunk failures during remove are silently ignored.Apply this fix:
- if _, ok := rsp.(error); ok { - return rsp - } + if er, ok := rsp.(*Rerror); ok { + return er + }Based on learnings, if this is a real issue (security-related or functional), file a PR upstream so fixes can propagate into vendored copies.
vendor/github.com/DeedleFake/p9/proto/server.go (1)
61-100: Vendored code: handler called with invalid data after receive errors.After a
Receiveerror (lines 71-77), the code logs but continues to line 79, invoking the handler with invalidtmsgandtag. This can cause panics or send invalid responses.Apply this fix:
for { tmsg, tag, err := p.Receive(c, msize) if err != nil { if err == io.EOF { return } log.Printf("Error reading message: %v", err) + continue }Also consider handling
net.ErrClosedlikeEOFfor clean shutdowns. Add"errors"to imports and checkerrors.Is(err, net.ErrClosed).Based on learnings, file a PR upstream for this critical fix.
vendor/github.com/DeedleFake/p9/dir.go (3)
18-20: Vendored code: critical path traversal vulnerability.The
path()method allows ".." to escape the served root (e.g.,/share/../../etc/passwd). This affects all filesystem operations (Stat, Open, Create, Remove, etc.) that use this method.This must be fixed by validating that resolved paths remain within the base directory:
-func (d Dir) path(p string) string { - return filepath.Join(string(d), filepath.FromSlash(p)) -} +func (d Dir) path(p string) (string, error) { + base := string(d) + joined := filepath.Join(base, filepath.FromSlash(p)) + rel, err := filepath.Rel(base, joined) + if err != nil { + return "", err + } + if rel == ".." || strings.HasPrefix(rel, ".."+string(os.PathSeparator)) { + return "", errors.New("path escapes base") + } + return joined, nil +}All callers must be updated to handle the error return. Based on learnings, file an urgent upstream PR for this security vulnerability.
52-60: Vendored code: partial time updates can corrupt timestamps.Lines 54-59 call
os.Chtimeswhen only one of ATime/MTime is set, passing a zero-value for the other. This can set invalid timestamps or unexpectedly overwrite the unmodified time.Reject partial updates:
atime, ok1 := changes.ATime() mtime, ok2 := changes.MTime() - if ok1 || ok2 { + if ok1 != ok2 { + return errors.New("setting only one of ATime or MTime is not supported") + } + if ok1 && ok2 { err := os.Chtimes(p, atime, mtime) if err != nil { return err } }Based on learnings, file an upstream PR for this fix.
69-75: Vendored code: name change allows path traversal.Line 71 uses
filepath.Join(base, filepath.FromSlash(name))without validating thatnameis a simple basename. This allows slashes or ".." to move files outside the intended directory.Validate the name:
name, ok := changes.Name() if ok { + if name != filepath.Base(name) || strings.ContainsRune(name, os.PathSeparator) { + return errors.New("invalid name: must be a base name without path separators") + } err := os.Rename(p, filepath.Join(base, name)) if err != nil { return err } }Add
"strings"to imports. Based on learnings, file an upstream PR for this security issue.
🧹 Nitpick comments (6)
vendor/github.com/DeedleFake/p9/proto/proto.go (1)
101-108: Dead code: unreachableif err != nilcheck.Line 103 checks
if err != nilbuterrwas already checked and returned on line 98. This condition is unreachable. While minor, it suggests incomplete error handling logic.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/README.md (1)
1-51: Vendor README: prefer tweaking markdownlint config over editing vendored textThis README mirrors the upstream p9 project; the MD003/MD010 markdownlint warnings are purely stylistic. To avoid divergence from upstream, it’s better to exclude vendor paths from markdownlint (or relax those rules there) rather than reformatting this file.
pkg/crc/constants/constants.go (1)
59-62: Plan9 constants look reasonable; optional doc comments*The new Plan9Msize/Plan9TcpPort/Plan9HvsockGUID/Plan9HvsockPort constants are well-grouped with other networking-related constants. For future maintainers, consider brief comments (e.g., “default 9P msize in bytes”, “default 9P TCP port”) to document where these values come from and how they’re used.
vendor/github.com/DeedleFake/p9/client.go (1)
45-47: Consider FID overflow behavior.The
nextFID()function uses atomic increment but doesn't handle the case when the uint32 counter wraps around to 0, potentially reusing FIDs that are still in use. For long-running connections with many file operations, this could cause conflicts.vendor/github.com/DeedleFake/p9/proto/client.go (1)
78-109: Reader silently continues on non-EOF I/O errors.When
c.p.Receivefails with an error that is neither a context cancellation nor EOF (line 92-96), the reader silently continues to the next iteration. This swallows potentially important errors (e.g., malformed messages, connection issues) without logging, making debugging difficult.Consider logging non-EOF errors before continuing:
if err != nil { if (ctx.Err() != nil) || (err == io.EOF) { return } + log.Printf("Receive error (continuing): %v", err) continue }vendor/github.com/DeedleFake/p9/fs.go (1)
169-186: Vendored code: unsafe conversion is valid but could be simplified.Line 179 uses
unsafe.Slice(unsafe.StringData(p), len(p))which is valid Go 1.20+ code. However, a simpler[]byte(p)would be more idiomatic and avoid unsafe. If this is a real performance concern and feasible to fix, consider filing an upstream PR.Based on learnings, this is vendored code and should be fixed upstream if needed.
📜 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 (48)
cmd/crc/cmd/daemon.go(5 hunks)cmd/crc/cmd/start.go(0 hunks)go.mod(2 hunks)packaging/windows/product.wxs.template(2 hunks)pkg/crc/config/settings.go(1 hunks)pkg/crc/constants/constants.go(1 hunks)pkg/crc/machine/driver.go(0 hunks)pkg/crc/machine/driver_darwin.go(0 hunks)pkg/crc/machine/driver_linux.go(0 hunks)pkg/crc/machine/driver_windows.go(0 hunks)pkg/crc/machine/libhvee/driver.go(1 hunks)pkg/crc/machine/libhvee/driver_test.go(1 hunks)pkg/crc/machine/libhvee/driver_windows.go(2 hunks)pkg/crc/machine/start.go(2 hunks)pkg/drivers/libhvee/libhvee_windows.go(0 hunks)pkg/drivers/libhvee/powershell_windows.go(0 hunks)pkg/fileserver/fs9p/server.go(1 hunks)pkg/fileserver/fs9p/server_fallback.go(1 hunks)pkg/fileserver/fs9p/server_windows.go(1 hunks)test/e2e/features/9pfs.feature(1 hunks)test/e2e/testsuite/testsuite.go(3 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 with no reviewable changes (7)
- pkg/drivers/libhvee/powershell_windows.go
- pkg/crc/machine/driver.go
- cmd/crc/cmd/start.go
- pkg/crc/machine/driver_windows.go
- pkg/crc/machine/driver_linux.go
- pkg/crc/machine/driver_darwin.go
- pkg/drivers/libhvee/libhvee_windows.go
✅ Files skipped from review due to trivial changes (1)
- vendor/github.com/DeedleFake/p9/LICENSE
🚧 Files skipped from review as they are similar to previous changes (13)
- vendor/github.com/DeedleFake/p9/addr_unix.go
- vendor/github.com/DeedleFake/p9/dir_windows.go
- vendor/github.com/DeedleFake/p9/addr_other.go
- pkg/fileserver/fs9p/server_windows.go
- vendor/github.com/DeedleFake/p9/doc.go
- vendor/github.com/DeedleFake/p9/dir_linux.go
- vendor/modules.txt
- vendor/github.com/DeedleFake/p9/encoding.go
- test/e2e/features/9pfs.feature
- pkg/crc/config/settings.go
- vendor/github.com/DeedleFake/p9/remote.go
- vendor/github.com/DeedleFake/p9/dir_other.go
- vendor/github.com/DeedleFake/p9/internal/util/util.go
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: redbeam
Repo: crc-org/crc PR: 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
Repo: crc-org/crc PR: 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/crc/machine/start.gopkg/fileserver/fs9p/server_fallback.gocmd/crc/cmd/daemon.gotest/e2e/testsuite/testsuite.gopkg/fileserver/fs9p/server.govendor/github.com/DeedleFake/p9/proto/server.gogo.modvendor/github.com/DeedleFake/p9/fs.govendor/github.com/DeedleFake/p9/msg.govendor/github.com/DeedleFake/p9/p9.govendor/github.com/DeedleFake/p9/README.md
🧬 Code graph analysis (17)
vendor/github.com/DeedleFake/p9/internal/debug/nodebug.go (1)
vendor/github.com/DeedleFake/p9/internal/debug/debug.go (1)
Log(11-13)
pkg/crc/machine/start.go (3)
pkg/os/exec.go (1)
RunPrivileged(48-59)pkg/crc/constants/constants.go (2)
Plan9HvsockPort(62-62)VSockGateway(42-42)pkg/crc/logging/logging.go (1)
Warnf(100-102)
pkg/fileserver/fs9p/server_fallback.go (1)
pkg/fileserver/fs9p/server_windows.go (1)
GetHvsockListener(14-29)
cmd/crc/cmd/daemon.go (5)
pkg/crc/config/settings.go (1)
EnableSharedDirs(33-33)pkg/fileserver/fs9p/server_windows.go (1)
GetHvsockListener(14-29)pkg/crc/constants/constants.go (2)
Plan9HvsockGUID(61-61)Plan9TcpPort(60-60)pkg/fileserver/fs9p/server.go (1)
New9pServer(34-58)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 (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/internal/debug/debug.go (1)
vendor/github.com/DeedleFake/p9/internal/debug/nodebug.go (1)
Log(6-6)
pkg/crc/machine/libhvee/driver_test.go (1)
pkg/crc/machine/libhvee/driver.go (1)
ConvertToUnixPath(10-25)
test/e2e/testsuite/testsuite.go (3)
pkg/crc/machine/libhvee/driver.go (1)
ConvertToUnixPath(10-25)pkg/crc/constants/constants.go (1)
GetHomeDir(166-172)test/extended/util/util.go (1)
SendCommandToVM(237-254)
pkg/fileserver/fs9p/server.go (5)
vendor/github.com/DeedleFake/p9/fs.go (2)
FileSystem(21-34)FSConnHandler(162-167)vendor/github.com/DeedleFake/p9/dir.go (1)
Dir(16-16)vendor/github.com/DeedleFake/p9/proto/server.go (1)
Serve(17-42)vendor/github.com/DeedleFake/p9/msg.go (1)
Proto(75-77)pkg/crc/constants/constants.go (1)
Plan9Msize(59-59)
vendor/github.com/DeedleFake/p9/addr.go (2)
vendor/github.com/DeedleFake/p9/addr_other.go (1)
NamespaceDir(18-30)vendor/github.com/DeedleFake/p9/addr_unix.go (1)
NamespaceDir(18-30)
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/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)
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 (5)
vendor/github.com/DeedleFake/p9/stat.go (2)
DirEntry(255-267)FileMode(23-23)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)
Log(11-13)vendor/github.com/DeedleFake/p9/msg.go (2)
Tversion(79-82)Tversion(84-84)vendor/github.com/DeedleFake/p9/client.go (1)
ErrUnsupportedVersion(14-14)
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/p9.go (1)
vendor/github.com/DeedleFake/p9/stat.go (1)
FileMode(23-23)
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). (16)
- GitHub Check: verify-devcontainer
- GitHub Check: build (ubuntu-22.04, 1.24)
- GitHub Check: build (ubuntu-latest, 1.24)
- GitHub Check: build (macOS-15, 1.24)
- GitHub Check: build (windows-2022, 1.24)
- GitHub Check: build-qe (linux, amd64)
- GitHub Check: build-qe (windows, amd64)
- GitHub Check: build-qe (darwin, amd64)
- GitHub Check: build (macOS-26, 1.24)
- GitHub Check: build-installer (windows-2022, 1.24)
- GitHub Check: build-qe (linux, arm64)
- GitHub Check: build-qe (darwin, arm64)
- GitHub Check: build (ubuntu-latest, 1.24)
- GitHub Check: build (macOS-15, 1.24)
- GitHub Check: Run OKD bundle with crc (1.24)
- GitHub Check: Konflux kflux-prd-rh02 / crc-binary-on-pull-request
🔇 Additional comments (20)
vendor/github.com/DeedleFake/p9/internal/debug/debug.go (1)
1-13: LGTM — Standard conditional debug logging pattern.This vendored file correctly implements build-tag-conditional debug logging. The dual build tag format (Go 1.17+ and legacy) ensures compatibility across Go versions, and the function signature matches the no-op variant in
nodebug.go.vendor/github.com/DeedleFake/p9/internal/debug/nodebug.go (1)
1-6: LGTM — Correct no-op implementation for non-debug builds.The build tags correctly invert the
p9debugcondition, and the empty function body ensures zero runtime overhead. The function signature matches the debug variant, maintaining API consistency.go.mod (1)
7-7: Dependency addition aligns with PR objectives.The
github.com/DeedleFake/p9 v0.7.1dependency is appropriate for the 9P file sharing implementation. Note that a previous review flagged the library as "experimental" per its README—ensure adequate integration testing is performed before production deployment.vendor/github.com/DeedleFake/p9/msg.go (1)
44-77: Protocol registry and message types look correct.The message type definitions follow the 9P2000 protocol specification. The registry correctly maps type IDs to Go structs, and the custom
P9Encode/P9Decodeimplementations forRstatandTwstatproperly handle the nested size prefix required by the protocol.vendor/github.com/DeedleFake/p9/stat.go (1)
155-251: Stat serialization implementation looks correct.The
Statstruct and itsP9Encode/P9Decodemethods correctly implement the 9P stat format with proper size prefix handling and field ordering per the protocol specification.pkg/crc/machine/start.go (1)
321-322: These fields are user-configurable settings and should be retained for backward compatibility.
SharedDirPasswordandSharedDirUsernameare defined in the settings configuration (pkg/crc/config/settings.go) and can be set by users in config files. While these SMB-related credentials are not currently used by the 9P implementation, removing them would break configuration parsing for existing deployments. The fields should remain as-is during the ongoing migration from SMB to 9P file sharing.Likely an incorrect or invalid review comment.
pkg/crc/machine/libhvee/driver_test.go (1)
10-11: Tests correctly exercise the exported ConvertToUnixPath helperSwitching the test to use ConvertToUnixPath keeps it aligned with the public API while preserving the expected semantics across platforms.
pkg/fileserver/fs9p/server_fallback.go (1)
1-15: Non-Windows GetHvsockListener stub is appropriateProviding a stub that always errors on non-Windows keeps the API available for all builds while making any accidental non-Windows use fail fast with a clear, GOOS-specific error message.
cmd/crc/cmd/daemon.go (1)
182-212: Use of net.JoinHostPort for HTTP listeners is a solid portability improvementReplacing manual string formatting with net.JoinHostPort for the gateway and host virtual IP listeners avoids subtle issues with IPv6/port formatting and improves clarity.
pkg/crc/machine/libhvee/driver.go (1)
1-25: ConvertToUnixPath helper cleanly encapsulates Windows path normalizationThe function correctly:
- Normalizes separators (filepath.ToSlash on Windows, backslash replacement elsewhere).
- Detects drive-letter paths and maps them to
/mnt/<lowercase-drive>...as Podman expects.- Leaves non–drive-letter paths untouched.
Centralizing this logic as an exported helper reduces duplication and keeps driver-specific code simpler.
vendor/github.com/DeedleFake/p9/dir_plan9.go (1)
1-49: Plan9 DirEntry/QID helpers handle syscall.Dir safely
infoToEntrycleanly handles both*syscall.Dir-backed and genericos.FileInfovalues without panicking, andDir.GetQIDvalidates that the underlyingSys()value is a*syscall.Dirbefore constructing the QID. This looks correct for Plan9 stat integration and provides a graceful error when the expected metadata is missing.vendor/github.com/DeedleFake/p9/dir_darwin.go (1)
46-62: LGTM!The
GetQIDfunction correctly handles the case whereFileInfo.Sys()doesn't return a*syscall.Stat_t, and properly extracts the inode for the QID path.test/e2e/testsuite/testsuite.go (1)
1370-1480: LGTM! Well-structured 9P file sharing test helpers.The helper functions provide comprehensive coverage for validating 9P mount functionality:
directoryExistInVMvalidates mount point existencefilesystemIsMountedconfirms 9P filesystem is mounted- File operations cover the full CRUD cycle plus permissions and rename
- Directory operations cover creation, listing, and deletion
The use of
path.Joinis appropriate since these commands execute inside the VM.packaging/windows/product.wxs.template (2)
69-73: LGTM! Registry entry for 9P over hvsock.The new
Hvsock9pRegistryEntrycomponent follows the same pattern as the existingVsockRegistryEntryand correctly registers the fs9p-hvsock service under the Hyper-V Guest Communication Services registry path.
111-111: LGTM!The ComponentRef is correctly added to the DefaultFeature to ensure the 9P hvsock registry entry is installed.
vendor/github.com/DeedleFake/p9/proto/client.go (1)
165-208: LGTM! Send method has proper cancellation handling.The
Sendmethod correctly handles client closure at multiple points:
- Before acquiring a tag
- Before registering the message
- After receiving the response
The cancellation path properly cleans up the allocated tag via
cancelMsg.pkg/fileserver/fs9p/server.go (3)
34-58: LGTM: Constructor properly validates inputs and addresses goroutine leak concern.The buffered channel (line 49) correctly prevents goroutine leaks when
WaitForErroris not called, addressing the concern from past reviews.
60-75: LGTM: Start method uses appropriate pattern for error detection.The
selectwithdefault(lines 68-74) provides a best-effort check for immediate startup failures. While there's a theoretical race window, this pattern is reasonable for detecting synchronous errors during server initialization.
77-87: Documentation addresses known limitation.The warning comment (lines 78-80) clearly documents the forceful shutdown behavior. This addresses the graceful shutdown concern from past reviews by explicitly documenting the contract.
vendor/github.com/DeedleFake/p9/p9.go (1)
1-71: LGTM: Clean constant and type definitions.The foundational 9P types and constants are well-defined with appropriate documentation. No issues found.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (18)
pkg/crc/machine/libhvee/driver_windows.go (1)
21-32: LGTM! SMB to 9P migration looks correct.The transition from CIFS/SMB to 9P is properly implemented:
- Type correctly changed to
"9p"- Username field appropriately removed (not needed for 9P)
- Path conversion delegated to the shared
ConvertToUnixPathhelperThe concern about tag determinism (sorting
SharedDirsbefore assigning indexed tags) was already raised in a previous review.pkg/crc/machine/start.go (1)
244-255: Fix argv tokenization on line 249: combined command string will fail.The first
sshRunner.Runcall on line 249 passes"9pfs -V -p"as a single token, which will fail binary lookup sinceRunexpects separate argv elements. The subsequent arguments suggest the intent to split, but the command portion is still combined.Apply this diff to split the command properly:
- if _, _, err := sshRunner.Run("9pfs -V -p", fmt.Sprintf("%d", constants.Plan9HvsockPort), "2", mount.Target); err != nil { + if _, _, err := sshRunner.Run("9pfs", "-V", "-p", fmt.Sprintf("%d", constants.Plan9HvsockPort), "2", mount.Target); err != nil {Note: A past review comment indicated this was addressed in commits 2d84a72 to 19db067, but the issue persists in the current code.
vendor/github.com/DeedleFake/p9/dir.go (1)
1-260: Vendored code with known security issues reported upstream.This vendored file contains critical path traversal vulnerabilities that were flagged in previous reviews and reported upstream as DeedleFake/p9 issue #82. The key issues are:
- Lines 18-20:
Dir.path()allows..to escape the served root- Lines 52-60: Partial ATime/MTime updates can corrupt timestamps
- Lines 69-75: Name change allows path traversal
Until upstream merges a fix, consider applying local patches to the vendored copy to mitigate these security risks, especially if the 9P server will be exposed to untrusted clients.
vendor/github.com/DeedleFake/p9/dir_darwin.go (1)
35-43: Compile-time error:time.Unixsignature mismatch.
time.Unix(sys.Atimespec.Unix())is invalid—Atimespec.Unix()returns two values(sec int64, nsec int64), but you're passing only one argument totime.Unix.Apply this diff:
return DirEntry{ FileMode: ModeFromOS(fi.Mode()), - ATime: time.Unix(sys.Atimespec.Unix()), + ATime: time.Unix(sys.Atimespec.Sec, sys.Atimespec.Nsec), MTime: fi.ModTime(),vendor/github.com/DeedleFake/p9/proto/encoding.go (2)
34-38: Panic risk on non-pointer input—consider upstream fix.
Read()will panic if passed a non-pointer or nil value becausedecode()callsv.Addr()on the indirect value. This is vendored code, so per project guidelines, consider filing a PR upstream.
85-100: Length overflow risks for slices and strings already flagged.Non-byte slices and strings with lengths exceeding 65535 will silently truncate when cast to
uint16. These concerns were raised in prior reviews.vendor/github.com/DeedleFake/p9/client.go (3)
61-69: Unguarded type assertion can panic on protocol errors.If the server returns an unexpected response type,
rsp.(*Rversion)will panic. This was flagged in prior reviews—consider a guarded assertion.
84-91: Unguarded type assertion for Auth response.Same concern as Handshake:
rsp.(*Rauth)can panic if the response type is unexpected.
113-120: Unguarded type assertion for Attach response.Same concern:
rsp.(*Rattach)can panic on protocol mismatches.vendor/github.com/DeedleFake/p9/proto/client.go (1)
138-145: Tag exhaustion andNoTaghandling issue already flagged.The coordinator can spin forever if all tags are in use, and can return
NoTag(0xFFFF) which is reserved. This was identified in prior reviews.vendor/github.com/DeedleFake/p9/proto/server.go (1)
3-8: Avoid dispatching on failedReceiveand terminate cleanly on closed connections
handleMessagescurrently logs non-EOF errors fromp.Receivebut still callshandler.HandleMessage(tmsg)with whatever was left intmsg/tag, and it doesn’t terminate onnet.ErrClosed. This can panic or send bogus responses, and it may busy‑loop on a closed connection.Consider:
-import ( - "io" - "log" - "net" - "sync" -) +import ( + "errors" + "io" + "log" + "net" + "sync" +) @@ func handleMessages(c net.Conn, p Proto, handler MessageHandler) { @@ - for { - tmsg, tag, err := p.Receive(c, msize) - if err != nil { - if err == io.EOF { - return - } - - log.Printf("Error reading message: %v", err) - } - - mode(func() { + for { + tmsg, tag, err := p.Receive(c, msize) + if err != nil { + // EOF or closed connection: stop serving this client. + if errors.Is(err, io.EOF) || errors.Is(err, net.ErrClosed) { + return + } + // Other errors: log and stop; do not dispatch a handler + // with a failed/partial message. + log.Printf("Error reading message: %v", err) + return + } + + mode(func() { rmsg := handler.HandleMessage(tmsg)This ensures the handler is only called for successfully decoded messages and that closed connections don’t spin the loop.
Also applies to: 61-99
vendor/github.com/DeedleFake/p9/fs.go (1)
147-150: Tflush should reply withRflush, notRerror
flushcurrently returns anRerror(“flush is not supported”), and the comment aboveFSHandlerstill says Tflush is “not handled”. Per 9P, flush requests must be acknowledged withRflush; even if you can’t cancel in‑flight I/O, you should still respond success‑fully as best effort.Suggested change:
-// BUG: Tflush requests are not currently handled at all by this -// implementation due to no clear method of stopping a pending call to -// ReadAt or WriteAt. +// BUG: Tflush requests are only acknowledged; in-flight ReadAt/WriteAt +// operations are not actually cancelled. func (h *fsHandler) flush(msg *Tflush) any { - // TODO: Implement this. - return &Rerror{ - Ename: "flush is not supported", - } + // Best-effort: acknowledge the flush without attempting to cancel + // already in-flight operations. + return &Rflush{} }This keeps behavior compatible with the spec while documenting that cancellation is not implemented.
Also applies to: 244-249
vendor/github.com/DeedleFake/p9/remote.go (5)
34-71: Fixwalkpath handling and guard response type assertionFor absolute paths,
walkcan send invalidWname(empty elements or"/") and it blindly assertsrsp.(*Rwalk), which will panic if the server replies with*Rerroror anything unexpected.Recommend:
func (file *Remote) walk(p string) (*Remote, error) { fid := file.client.nextFID() - w := []string{path.Clean(p)} - if w[0] != "/" { - w = strings.Split(w[0], "/") - } - if (len(w) == 1) && (w[0] == ".") { - w = nil - } + p = path.Clean(p) + var w []string + switch { + case p == "", p == ".", p == "/": + // Root/no-op walk uses empty Wname. + w = nil + default: + if strings.HasPrefix(p, "/") { + p = strings.TrimPrefix(p, "/") + } + if p != "" && p != "." { + w = strings.Split(p, "/") + } + } rsp, err := file.client.Send(&Twalk{ FID: file.fid, NewFID: fid, Wname: w, }) if err != nil { return nil, err } - walk := rsp.(*Rwalk) + if er, ok := rsp.(*Rerror); ok { + return nil, er + } + walk, ok := rsp.(*Rwalk) + if !ok { + return nil, util.Errorf("walk: expected *Rwalk, got %T", rsp) + }This avoids invalid Wname components and safely handles protocol errors instead of panicking.
79-97: Guard all response type assertions and surfaceRerrorinstead of panickingSimilar to
walk, several methods assume a successful typed response and will panic if the server sends*Rerroror any other message type:
- Line 92:
open := rsp.(*Ropen)- Line 117:
create := rsp.(*Rcreate)- Line 211:
read := rsp.(*Rread)- Line 265:
write := rsp.(*Rwrite)- Line 327:
stat := rsp.(*Rstat)They also silently ignore server-side
Rerror.Pattern to apply (example for
Open):rsp, err := file.client.Send(&Topen{ FID: next.fid, Mode: mode, }) if err != nil { return nil, err } - open := rsp.(*Ropen) + if er, ok := rsp.(*Rerror); ok { + return nil, er + } + open, ok := rsp.(*Ropen) + if !ok { + return nil, util.Errorf("open: expected *Ropen, got %T", rsp) + }Do the same
*Rerrorcheck and guarded assertion forCreate,readPart,writePart, andStat. This turns server-provided errors into Go errors instead of crashes.Also applies to: 101-122, 202-218, 256-271, 311-330
126-141: Propagate server-side errors fromRemoveandCloseBoth
Remove(whenp == "") andCloseignore the response body, so anRerrorfrom the server is silently dropped; callers only see transport errors.Suggested adjustments:
func (file *Remote) Remove(p string) error { @@ - _, err := file.client.Send(&Tremove{ - FID: file.fid, - }) - return err + rsp, err := file.client.Send(&Tremove{ + FID: file.fid, + }) + if err != nil { + return err + } + if er, ok := rsp.(*Rerror); ok { + return er + } + return nil } @@ func (file *Remote) Close() error { - _, err := file.client.Send(&Tclunk{ - FID: file.fid, - }) - return err + rsp, err := file.client.Send(&Tclunk{ + FID: file.fid, + }) + if err != nil { + return err + } + if er, ok := rsp.(*Rerror); ok { + return er + } + return nil }This keeps the API honest about server failures (permission denied, busy, etc.).
Also applies to: 298-305
147-185:Seekshould return an error instead of panicking on invalidwhenceOn an unsupported
whence,Seekpanics:panic(util.Errorf("Invalid whence: %v", whence))Library code should not panic on invalid input; return an error and leave
posunchanged:case io.SeekEnd: @@ file.pos = uint64(npos) return npos, nil } - panic(util.Errorf("Invalid whence: %v", whence)) + return int64(file.pos), util.Errorf("invalid whence: %v", whence)This matches idiomatic
io.Seekerbehavior and avoids crashing callers.
198-243: GuardmaxBufSizeand chunk sizes to avoid underflow and infinite loops
maxBufSizecurrently does:return int(file.client.Msize() - IOHeaderSize)If
Msize() <= IOHeaderSize, this underflows (uint32) or returns 0, and:
ReadAt/WriteAtcomputesize := min(len(...), file.maxBufSize())which can be 0, so thefor start += sizeloop never progresses (infinite loop).Readdirpasses a rawmaxBufSize()(possibly 0 or huge) tobufio.NewReaderSize, risking nonsensical buffer sizes.Recommended:
func (file *Remote) maxBufSize() int { - return int(file.client.Msize() - IOHeaderSize) + m := file.client.Msize() + if m <= uint32(IOHeaderSize) { + return 0 + } + return int(m - uint32(IOHeaderSize)) } @@ func (file *Remote) ReadAt(buf []byte, off int64) (int, error) { - size := min(len(buf), file.maxBufSize()) + max := file.maxBufSize() + if max <= 0 { + return 0, util.Errorf("msize too small: %d", file.client.Msize()) + } + size := len(buf) + if size > max { + size = max + } @@ func (file *Remote) WriteAt(data []byte, off int64) (int, error) { - size := min(len(data), file.maxBufSize()) + max := file.maxBufSize() + if max <= 0 { + return 0, util.Errorf("msize too small: %d", file.client.Msize()) + } + size := len(data) + if size > max { + size = max + } @@ func (file *Remote) Readdir() ([]DirEntry, error) { - return ReadDir(bufio.NewReaderSize(file, file.maxBufSize())) + max := file.maxBufSize() + if max <= 0 { + return nil, util.Errorf("msize too small: %d", file.client.Msize()) + } + return ReadDir(bufio.NewReaderSize(file, max)) }You may also want to validate negotiated
Msizeduring the client handshake soMsize <= IOHeaderSizefails fast.Also applies to: 283-296, 339-341
vendor/github.com/DeedleFake/p9/stat.go (1)
3-13:FileMode.Stringreturns a string backed by stack memory (unsafe use‑after‑return)
String()builds a local slice and then usesunsafe.String(unsafe.SliceData(buf), len(buf)). That string points at stack-allocated memory and can become invalid once the function returns, leading to undefined behavior.You can avoid this and drop
unsafeentirely:-import ( - "bytes" - "errors" - "io" - "os" - "time" - "unsafe" - - "github.com/DeedleFake/p9/internal/util" - "github.com/DeedleFake/p9/proto" -) +import ( + "bytes" + "errors" + "io" + "os" + "time" + + "github.com/DeedleFake/p9/internal/util" + "github.com/DeedleFake/p9/proto" +) @@ func (m FileMode) String() string { buf := []byte("----------") @@ - return unsafe.String(unsafe.SliceData(buf), len(buf)) + return string(buf) }This adds a tiny allocation for a 10-byte buffer but restores memory safety and removes the need for
unsafe.Also applies to: 135-153
🧹 Nitpick comments (2)
cmd/crc/cmd/daemon.go (1)
253-300: 9P server setup looks correct; consider propagating errors to errCh.The implementation properly:
- Gates on
EnableSharedDirsconfig (addressing the past review comment)- Captures server handles and defers
Stop()for graceful shutdown- Sets up both hvsock and TCP fallback
One optional improvement: the
WaitForError()goroutines only log errors. For consistency with other components (lines 178, 194, 210, 249), consider sending 9P server errors toerrChso the daemon exits on fatal 9P failures:go func() { if err := server9pHvsock.WaitForError(); err != nil { - logging.Errorf("9p server (hvsock) error: %v", err) + errCh <- errors.Wrap(err, "9p server (hvsock) failed") } }()vendor/github.com/DeedleFake/p9/proto/client.go (1)
90-97: Read errors are silently swallowed.When
c.p.Receivereturns an error that isn't EOF and context isn't cancelled, the loop continues without logging or propagating the error. This could mask protocol or connection issues.Consider logging transient errors or implementing a retry limit to avoid infinite silent retries on persistent failures. Since this is vendored code, file upstream if desired.
📜 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 (49)
Makefile(1 hunks)cmd/crc/cmd/daemon.go(5 hunks)cmd/crc/cmd/start.go(0 hunks)go.mod(2 hunks)packaging/windows/product.wxs.template(2 hunks)pkg/crc/config/settings.go(1 hunks)pkg/crc/constants/constants.go(1 hunks)pkg/crc/machine/driver.go(0 hunks)pkg/crc/machine/driver_darwin.go(0 hunks)pkg/crc/machine/driver_linux.go(0 hunks)pkg/crc/machine/driver_windows.go(0 hunks)pkg/crc/machine/libhvee/driver.go(1 hunks)pkg/crc/machine/libhvee/driver_test.go(1 hunks)pkg/crc/machine/libhvee/driver_windows.go(2 hunks)pkg/crc/machine/start.go(2 hunks)pkg/drivers/libhvee/libhvee_windows.go(0 hunks)pkg/drivers/libhvee/powershell_windows.go(0 hunks)pkg/fileserver/fs9p/server.go(1 hunks)pkg/fileserver/fs9p/server_fallback.go(1 hunks)pkg/fileserver/fs9p/server_windows.go(1 hunks)test/e2e/features/9pfs.feature(1 hunks)test/e2e/testsuite/testsuite.go(3 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 with no reviewable changes (7)
- cmd/crc/cmd/start.go
- pkg/crc/machine/driver_windows.go
- pkg/drivers/libhvee/libhvee_windows.go
- pkg/crc/machine/driver_linux.go
- pkg/drivers/libhvee/powershell_windows.go
- pkg/crc/machine/driver_darwin.go
- pkg/crc/machine/driver.go
✅ Files skipped from review due to trivial changes (2)
- Makefile
- vendor/github.com/DeedleFake/p9/LICENSE
🚧 Files skipped from review as they are similar to previous changes (14)
- pkg/fileserver/fs9p/server_fallback.go
- vendor/github.com/DeedleFake/p9/addr_other.go
- pkg/crc/constants/constants.go
- vendor/github.com/DeedleFake/p9/internal/util/util.go
- vendor/github.com/DeedleFake/p9/internal/debug/debug.go
- test/e2e/features/9pfs.feature
- pkg/crc/config/settings.go
- vendor/github.com/DeedleFake/p9/dir_linux.go
- vendor/github.com/DeedleFake/p9/encoding.go
- vendor/github.com/DeedleFake/p9/doc.go
- vendor/github.com/DeedleFake/p9/dir_windows.go
- vendor/github.com/DeedleFake/p9/internal/debug/nodebug.go
- vendor/github.com/DeedleFake/p9/p9.go
- go.mod
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: redbeam
Repo: crc-org/crc PR: 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
Repo: crc-org/crc PR: 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/server_windows.gocmd/crc/cmd/daemon.gotest/e2e/testsuite/testsuite.gopkg/fileserver/fs9p/server.govendor/github.com/DeedleFake/p9/fs.gopkg/crc/machine/start.govendor/github.com/DeedleFake/p9/msg.govendor/github.com/DeedleFake/p9/README.md
🧬 Code graph analysis (15)
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_windows.go (1)
pkg/fileserver/fs9p/server_fallback.go (1)
GetHvsockListener(13-15)
pkg/crc/machine/libhvee/driver_test.go (1)
pkg/crc/machine/libhvee/driver.go (1)
ConvertToUnixPath(10-25)
vendor/github.com/DeedleFake/p9/addr.go (2)
vendor/github.com/DeedleFake/p9/addr_other.go (1)
NamespaceDir(18-30)vendor/github.com/DeedleFake/p9/addr_unix.go (1)
NamespaceDir(18-30)
vendor/github.com/DeedleFake/p9/addr_unix.go (1)
vendor/github.com/DeedleFake/p9/addr_other.go (1)
NamespaceDir(18-30)
pkg/fileserver/fs9p/server.go (5)
vendor/github.com/DeedleFake/p9/fs.go (2)
FileSystem(21-34)FSConnHandler(162-167)vendor/github.com/DeedleFake/p9/dir.go (1)
Dir(16-16)vendor/github.com/DeedleFake/p9/proto/server.go (1)
Serve(17-42)vendor/github.com/DeedleFake/p9/proto/proto.go (1)
Proto(38-41)pkg/crc/constants/constants.go (1)
Plan9Msize(59-59)
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_plan9.go (4)
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/miekg/dns/types.go (2)
UID(1448-1451)GID(1456-1459)vendor/github.com/DeedleFake/p9/p9.go (2)
QID(42-46)Version(10-10)
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 (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/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/encoding.go (1)
WriteDir(33-42)
vendor/github.com/DeedleFake/p9/dir_darwin.go (2)
vendor/github.com/DeedleFake/p9/stat.go (3)
DirEntry(255-267)FileMode(23-23)ModeFromOS(43-78)vendor/github.com/DeedleFake/p9/p9.go (3)
QID(42-46)QIDType(49-49)Version(10-10)
vendor/github.com/DeedleFake/p9/remote.go (4)
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/msg.go (7)
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)
pkg/crc/machine/start.go (3)
pkg/os/exec.go (1)
RunPrivileged(48-59)pkg/crc/constants/constants.go (2)
Plan9HvsockPort(62-62)VSockGateway(42-42)pkg/crc/logging/logging.go (1)
Warnf(100-102)
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.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 (6)
OWRITE(27-27)ORDWR(28-28)OEXEC(29-29)OTRUNC(31-31)OCEXEC(32-32)ORCLOSE(33-33)
🪛 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). (15)
- GitHub Check: build-qe (darwin, arm64)
- GitHub Check: build-qe (linux, amd64)
- GitHub Check: build-qe (linux, arm64)
- GitHub Check: build-qe (windows, amd64)
- GitHub Check: build (macOS-15, 1.24)
- GitHub Check: build-qe (darwin, amd64)
- GitHub Check: build-installer (windows-2022, 1.24)
- GitHub Check: Run OKD bundle with crc (1.24)
- GitHub Check: build (windows-2022, 1.24)
- GitHub Check: build (ubuntu-latest, 1.24)
- GitHub Check: build (macOS-15, 1.24)
- GitHub Check: build (ubuntu-latest, 1.24)
- GitHub Check: verify-devcontainer
- GitHub Check: build (windows-2022, 1.24)
- GitHub Check: Konflux kflux-prd-rh02 / crc-binary-on-pull-request
🔇 Additional comments (19)
pkg/crc/machine/libhvee/driver_test.go (1)
9-12: LGTM! Consider expanding test coverage.The test correctly exercises the exported
ConvertToUnixPathfunction. The two test cases cover the primary use case (Windows drive-letter paths). You might consider adding edge cases (empty string, paths without drive letters, already-slash paths) in a follow-up.pkg/crc/machine/libhvee/driver.go (1)
9-25: Well-documented path conversion helper.The implementation correctly handles Windows-style paths with drive letters, converting them to the
/mnt/<drive>/...format expected by Podman. The reference to the Podman source is helpful for maintainability.Minor observation: The
runtime.GOOSbranch (lines 16-20) ensures tests can run on non-Windows platforms with backslash-containing test strings, sincefilepath.ToSlashon non-Windows would not convert backslashes. Good defensive coding.vendor/modules.txt (1)
13-18: LGTM! Vendor dependency addition is correct.The DeedleFake/p9 v0.7.1 package and its subpackages are properly declared with the explicit tag and Go version 1.24.0. This aligns with the PR's objective to add 9P file sharing support.
test/e2e/testsuite/testsuite.go (2)
604-615: LGTM! 9P test step registration is well-structured.The test steps properly validate 9P file sharing functionality by checking mount existence, filesystem type, and basic file/directory operations. The
libhvee.ConvertToUnixPathusage is appropriate per the earlier clarification that this function is now cross-platform.
1382-1492: LGTM! 9P test helpers provide comprehensive coverage.The helper functions systematically verify:
- Directory existence in VM (directoryExistInVM)
- Filesystem mount type verification (filesystemIsMounted)
- Directory listing (listingFilesInDirectoryShouldSucceed)
- CRUD operations on files (basicFileOperationsInDirectoryShouldSucceed)
- Directory operations (basicDirectoryOperationsInDirectoryShouldSucceed)
The implementations correctly use
util.SendCommandToVMand validate outputs appropriately.packaging/windows/product.wxs.template (1)
69-73: LGTM! Hvsock 9P registry entry is correctly configured.The new component registers the fs9p-hvsock service under Windows Virtualization Guest Communication Services with GUID
00009000-FACB-11E6-BD58-64006A7986D3. The structure follows the existing pattern for VsockRegistryEntry, and per the earlier discussion, the Win64 attribute is not required for this single-arch installer.vendor/github.com/DeedleFake/p9/dir_other.go (1)
1-15: LGTM!This fallback implementation for non-listed platforms correctly populates the available fields from
os.FileInfowhile leaving platform-specific fields (ATime, UID, GID, MUID) as zero values, which is consistent with the approach in other OS-specific variants.pkg/fileserver/fs9p/server_windows.go (1)
12-29: LGTM!Clean implementation with proper error wrapping. The use of
hvsock.GUIDWildcardforVMIDallows any VM to connect to this listener, which is appropriate for the CRC VM use case.cmd/crc/cmd/daemon.go (1)
182-182: LGTM!Good use of
net.JoinHostPortinstead of string formatting—this properly handles IPv6 addresses.vendor/github.com/DeedleFake/p9/dir_plan9.go (2)
10-31: LGTM!The
infoToEntryfunction correctly handles both cases: whensyscall.Diris available (populating ATime, UID, GID, MUID) and falling back to basic fields otherwise.
33-49:GetQIDinherits the path traversal vulnerability fromd.path().This method uses
d.path(p)which has the same path traversal issue flagged indir.go. The vulnerability will be resolved when the upstream fix for issue #82 is merged.Minor nit: The error message on line 41 could be clearer—"FileInfo was not Dir" may be confused with the directory type check. Consider: "FileInfo.Sys() was not *syscall.Dir".
vendor/github.com/DeedleFake/p9/dir_darwin.go (1)
46-62: LGTM!The
GetQIDimplementation correctly handles the Darwin-specificStat_ttype assertion and constructs a valid QID from the inode number.vendor/github.com/DeedleFake/p9/proto/encoding.go (1)
166-173: LGTM for string decoding.The
unsafe.String+unsafe.SliceDatapattern is idiomatic Go 1.20+ for zero-copy string creation from a byte slice.vendor/github.com/DeedleFake/p9/client.go (1)
45-47: LGTM!The
nextFIDimplementation correctly uses atomic operations to generate sequential FIDs in a thread-safe manner.vendor/github.com/DeedleFake/p9/proto/client.go (1)
165-208: LGTM!The
Sendmethod correctly handles tag acquisition, message dispatch, and response routing with proper cleanup on errors.pkg/fileserver/fs9p/server.go (3)
89-98: LGTM for Windows-specific error filtering.The filtering of "endpoint is in invalid state" errors is appropriate since 9P is Windows-only in this codebase, as clarified in prior review discussions.
77-87: LGTM with useful warning documented.The comment about client zombification on abrupt shutdown is helpful. The behavior is clearly documented.
34-58: LGTM!Path validation is thorough—checking for absolute path, existence, and directory type before constructing the server.
vendor/github.com/DeedleFake/p9/msg.go (1)
12-277: 9P message definitions and protocol registry look consistentThe message type constants,
protocoltype map, and the T*/R* structs (includingRerror,Rstat,Twstat) are internally consistent and match the expected 9P message set. The custom encode/decode paths forRstatandTwstatcorrectly pair withStat.size()and theLimitedReaderbounds.No changes needed here.
|
@redbeam we need to add a check to skip mounting shared directories when preset is microshift and the os is windows, wdyt? |
|
@redbeam could you please also rebase it on main again |
This enables communication through hvsocks instead of TCP sockets for 9P file sharing. TCP is still utilized, as a backup option.
After the addition of 9p file sharing for Windows, we are removing SMB file sharing support (#4768). Part of this effort is to remove at least some of the warnings reported by virustotal for the MSI: https://www.virustotal.com/gui/file/31b402dcc1da24265074a21a26018d6cde8eef0b63c77a18f89eb079b6556790
|
@anjannath I've addressed the microshift issue and rebased on the latest main. |
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (12)
pkg/crc/machine/libhvee/driver_windows.go (1)
23-29: Past review comment still applies: tag generation determinism.The concern about non-deterministic tag assignment based on iteration order has already been flagged in a previous review. If
machineConfig.SharedDirsorder changes, tag names will change, potentially causing mount failures. The suggestion to sort the directories before assigning tags remains valid.vendor/github.com/DeedleFake/p9/proto/encoding.go (2)
34-38:Read()can panic on non-pointer input.This issue was flagged in previous reviews. Per PR comments, consider filing an upstream PR to add a guard that validates
vis a non-nil pointer before callingdecode().
85-100: Missing bounds checks for slice and string lengths encoded as uint16.Non-byte slices and strings use uint16 length prefixes, which will silently wrap for lengths exceeding 65535. This was flagged in previous reviews.
vendor/github.com/DeedleFake/p9/remote.go (3)
183-185: Library code should not panic for invalid whence.As flagged in previous reviews,
panic(util.Errorf(...))should be replaced with returning an error. This prevents crashes from invalid input.
52-52: Unchecked type assertion can panic on protocol errors.
rsp.(*Rwalk)at line 52 will panic if the server returns an unexpected response type. This applies to similar assertions at lines 92, 117, 211, 265, and 327.
229-243: ReadAt loop may hang if maxBufSize() returns zero or negative.If
Msize() <= IOHeaderSize,maxBufSize()returns zero or negative, causingmin(len(buf), 0)to yield 0. The loopfor start := 0; start < len(buf); start += sizenever advances whensize == 0, resulting in an infinite loop.vendor/github.com/DeedleFake/p9/fs.go (1)
593-598: Type assertionrsp.(error)never matches; use*Rerror.
clunk()returns*Rerrorfor errors, not theerrorinterface. The check at line 596 will never be true, so clunk errors are silently ignored before the remove proceeds.Apply this fix:
- if _, ok := rsp.(error); ok { + if _, ok := rsp.(*Rerror); ok { return rsp }vendor/github.com/DeedleFake/p9/proto/proto.go (1)
89-93: Off-by-4 framing: LimitedReader should bound tosize-4.The 9P size field includes its own 4 bytes. After reading
size, the remaining frame issize-4bytes. UsingN: sizeallows reads to consume 4 bytes from the next frame on boundary-aligned payloads.+ if size < 4 { + return nil, NoTag, util.Errorf("receive: invalid frame size: %d", size) + } lr := &util.LimitedReader{ R: r, - N: size, + N: size - 4, E: ErrLargeMessage, }vendor/github.com/DeedleFake/p9/dir.go (3)
18-20: Critical: Path traversal via ".." allows filesystem escape.
filepath.Joinalone doesn't prevent..from escaping the root directory. For example,/share/../../etc/passwdresolves to/etc/passwd. This was reported upstream as issue #82.Consider applying a local patch until upstream fixes this, as this is a critical security vulnerability.
52-59: Partial ATime/MTime updates can corrupt timestamps.When only one of ATime or MTime is provided,
os.Chtimesis called with a zero-value for the other, potentially clobbering it. Either fetch the current value for the missing timestamp or reject partial updates.
69-75: Name change allows path traversal outside parent directory.
filepath.FromSlash(name)doesn't validate thatnameis a simple filename. A name like../../../etc/passwdwould move the file outside the intended directory.vendor/github.com/DeedleFake/p9/stat.go (1)
135-153: Memory safety issue withunsafe.Stringalready flagged—ensure upstream fix.This is a duplicate of a previous review comment. The
unsafe.Stringusage on line 152 creates a memory safety risk by returning a string that points to a stack-allocated slice buffer.As per PR comments, real issues in vendored code should be filed upstream so fixes propagate when merged. Please verify if an upstream issue or PR has been created for this fix.
🧹 Nitpick comments (3)
test/e2e/testsuite/testsuite.go (1)
1409-1468: Consider cleanup on partial failure in file operations test.If a step fails after file creation (e.g., write succeeds but read fails), the test file
story_9pfs_test_fileorstory_9pfs_test_file_renamedmay remain on the host. For test isolation, consider adding cleanup logic or documenting that test artifacts may persist on failure.func basicFileOperationsInDirectoryShouldSucceed(dirName string) error { filename := path.Join(dirName, "story_9pfs_test_file") + newFilename := path.Join(dirName, "story_9pfs_test_file_renamed") content := "test content" + + // Cleanup helper for partial failures + cleanup := func() { + _, _ = util.SendCommandToVM(fmt.Sprintf("rm -f %s %s", filename, newFilename)) + } + defer cleanup() // Create _, err := util.SendCommandToVM(fmt.Sprintf("> %s", filename))vendor/github.com/DeedleFake/p9/proto/encoding.go (1)
106-108: Error message logsreflect.Valuetype instead of the actual type.At line 107,
util.Errorf("invalid type: %T", v)will printreflect.Valuerather than the underlying type sincevis areflect.Value. Usev.Interface()orv.Type()to get a meaningful type name.The same issue exists at line 181.
- default: - e.err = util.Errorf("invalid type: %T", v) + default: + e.err = util.Errorf("invalid type: %v", v.Type())Also applies to line 181:
- default: - d.err = util.Errorf("invalid type: %T", v) + default: + d.err = util.Errorf("invalid type: %v", v.Type())vendor/github.com/DeedleFake/p9/proto/proto.go (1)
101-108: Dead code:errcheck at line 103 is unreachable.At line 103,
if err != nilchecks an error that would have caused an early return at line 98. When execution reaches line 102,erris alwaysnil, making lines 103-105 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) }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (48)
cmd/crc/cmd/daemon.go(5 hunks)cmd/crc/cmd/start.go(0 hunks)go.mod(2 hunks)packaging/windows/product.wxs.template(2 hunks)pkg/crc/config/settings.go(1 hunks)pkg/crc/constants/constants.go(1 hunks)pkg/crc/machine/driver.go(0 hunks)pkg/crc/machine/driver_darwin.go(0 hunks)pkg/crc/machine/driver_linux.go(0 hunks)pkg/crc/machine/driver_windows.go(0 hunks)pkg/crc/machine/libhvee/driver.go(1 hunks)pkg/crc/machine/libhvee/driver_test.go(1 hunks)pkg/crc/machine/libhvee/driver_windows.go(2 hunks)pkg/crc/machine/start.go(2 hunks)pkg/drivers/libhvee/libhvee_windows.go(0 hunks)pkg/drivers/libhvee/powershell_windows.go(0 hunks)pkg/fileserver/fs9p/server.go(1 hunks)pkg/fileserver/fs9p/server_fallback.go(1 hunks)pkg/fileserver/fs9p/server_windows.go(1 hunks)test/e2e/features/9pfs.feature(1 hunks)test/e2e/testsuite/testsuite.go(3 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 with no reviewable changes (7)
- pkg/crc/machine/driver_windows.go
- pkg/drivers/libhvee/powershell_windows.go
- cmd/crc/cmd/start.go
- pkg/drivers/libhvee/libhvee_windows.go
- pkg/crc/machine/driver_linux.go
- pkg/crc/machine/driver.go
- pkg/crc/machine/driver_darwin.go
✅ Files skipped from review due to trivial changes (1)
- pkg/fileserver/fs9p/server_fallback.go
🚧 Files skipped from review as they are similar to previous changes (21)
- vendor/github.com/DeedleFake/p9/dir_other.go
- vendor/github.com/DeedleFake/p9/addr_unix.go
- test/e2e/features/9pfs.feature
- pkg/fileserver/fs9p/server_windows.go
- pkg/crc/machine/libhvee/driver_test.go
- vendor/github.com/DeedleFake/p9/internal/debug/debug.go
- vendor/github.com/DeedleFake/p9/addr_other.go
- vendor/modules.txt
- vendor/github.com/DeedleFake/p9/proto/server.go
- vendor/github.com/DeedleFake/p9/encoding.go
- pkg/crc/config/settings.go
- vendor/github.com/DeedleFake/p9/proto/client.go
- vendor/github.com/DeedleFake/p9/dir_linux.go
- vendor/github.com/DeedleFake/p9/dir_darwin.go
- vendor/github.com/DeedleFake/p9/client.go
- pkg/crc/constants/constants.go
- vendor/github.com/DeedleFake/p9/LICENSE
- vendor/github.com/DeedleFake/p9/doc.go
- vendor/github.com/DeedleFake/p9/internal/util/util.go
- vendor/github.com/DeedleFake/p9/addr.go
- vendor/github.com/DeedleFake/p9/p9.go
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: redbeam
Repo: crc-org/crc PR: 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
Repo: crc-org/crc PR: 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:
go.modpkg/crc/machine/start.gotest/e2e/testsuite/testsuite.govendor/github.com/DeedleFake/p9/fs.gopkg/fileserver/fs9p/server.gocmd/crc/cmd/daemon.govendor/github.com/DeedleFake/p9/msg.govendor/github.com/DeedleFake/p9/README.md
🧬 Code graph analysis (11)
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/proto/encoding.go (1)
vendor/github.com/DeedleFake/p9/internal/util/util.go (1)
Errorf(44-52)
pkg/crc/machine/start.go (3)
pkg/os/exec.go (1)
RunPrivileged(48-59)pkg/crc/constants/constants.go (2)
Plan9HvsockPort(62-62)VSockGateway(42-42)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 (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/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 (6)
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 (2)
MessageHandler(135-139)ConnHandler(112-114)vendor/github.com/DeedleFake/p9/internal/debug/nodebug.go (1)
Log(6-6)vendor/github.com/DeedleFake/p9/msg.go (2)
Tversion(79-82)Tversion(84-84)vendor/github.com/DeedleFake/p9/encoding.go (1)
WriteDir(33-42)
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 (6)
OWRITE(27-27)ORDWR(28-28)OEXEC(29-29)OTRUNC(31-31)OCEXEC(32-32)ORCLOSE(33-33)
vendor/github.com/DeedleFake/p9/remote.go (5)
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)
pkg/crc/machine/libhvee/driver_windows.go (2)
vendor/github.com/crc-org/machine/libmachine/drivers/base.go (1)
SharedDir(27-35)pkg/crc/machine/libhvee/driver.go (1)
ConvertToUnixPath(10-25)
cmd/crc/cmd/daemon.go (5)
pkg/crc/config/settings.go (1)
EnableSharedDirs(33-33)pkg/fileserver/fs9p/server_windows.go (1)
GetHvsockListener(14-29)pkg/crc/constants/constants.go (2)
Plan9HvsockGUID(61-61)Plan9TcpPort(60-60)pkg/fileserver/fs9p/server.go (1)
New9pServer(34-58)pkg/crc/logging/logging.go (1)
Warnf(100-102)
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 (2)
FileMode(23-23)Stat(156-168)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)
🪛 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: verify-devcontainer
- GitHub Check: Run OKD bundle with crc (1.24)
- GitHub Check: build-qe (linux, amd64)
- GitHub Check: build (ubuntu-latest, 1.24)
- GitHub Check: build (macOS-15-intel, 1.24)
- GitHub Check: build (macOS-15, 1.24)
- GitHub Check: build-qe (linux, arm64)
- GitHub Check: build (windows-2022, 1.24)
- GitHub Check: build (macOS-15, 1.24)
- GitHub Check: build (ubuntu-22.04, 1.24)
- GitHub Check: build-qe (darwin, arm64)
- GitHub Check: build (macOS-26, 1.24)
- GitHub Check: build-qe (darwin, amd64)
- GitHub Check: build (ubuntu-latest, 1.24)
- GitHub Check: build-installer (windows-2022, 1.24)
- GitHub Check: build-qe (windows, amd64)
- GitHub Check: Konflux kflux-prd-rh02 / crc-binary-on-pull-request
🔇 Additional comments (19)
vendor/github.com/DeedleFake/p9/internal/debug/nodebug.go (1)
1-6: LGTM! Standard conditional compilation pattern.This vendored file correctly implements a no-op debug stub using Go's build constraint system. The implementation follows best practices: both new and legacy build tag syntax for backward compatibility, matching function signature with the real implementation in
debug.go, and appropriate no-op behavior when debugging is disabled.pkg/crc/machine/libhvee/driver.go (1)
10-25: LGTM! Path conversion logic is sound.The function correctly handles Windows-style path conversion to Unix format for podman compatibility. The OS-specific branching (filepath.ToSlash for Windows vs strings.ReplaceAll for non-Windows) is intentional and correct—the function may run on non-Windows platforms while still needing to convert Windows-style paths. The drive letter detection safely checks length before indexing and appropriately converts to the
/mnt/<drive>/format.pkg/crc/machine/libhvee/driver_windows.go (1)
25-28: LGTM! Migration to 9P correctly implemented.The configuration correctly transitions from SMB/CIFS to 9P:
- Type changed to "9p" (Line 28)
- Public
ConvertToUnixPathused for target path conversion (Line 26)- Username field removed (appropriate for 9P)
The changes align with the PR objective of migrating Windows file sharing from SMB to 9P.
pkg/crc/machine/start.go (2)
245-249: MicroShift guard is appropriateThe guard to skip 9P mounting for MicroShift bundles aligns with the PR objectives, which note that the microshift preset requires the 9pfs client in the bundle and will fail without it.
244-260: mount.Tag is not passed to 9pfs commands; verify if it should be passed as anameThe 9P protocol requires aname to specify the file tree to access when the server offers multiple exported file systems. The code at line 237 logs
mount.Tag, and at line 240,mount.Tagis correctly passed to the virtiofs mount command. However, the 9pfs commands at lines 254 and 257 do not includemount.Tag.Line 254 calls:
9pfs -V -p <port> 2 <mount.Target>
Line 257 calls:9pfs <VSockGateway> <mount.Target>Neither passes
mount.Tag. The 9pfuse helper accepts-a anameto specify the mount aname. Ifmount.Tagrepresents the aname or export identifier on the 9P server, it should be passed to the 9pfs command to identify which export to mount.go.mod (1)
7-7: Dependencies align with 9P implementation.The addition of
github.com/DeedleFake/p9and promotion ofgithub.com/linuxkit/virtsockto a direct dependency are appropriate for the 9P over hvsock implementation on Windows.Also applies to: 31-31
test/e2e/testsuite/testsuite.go (2)
604-616: 9P file sharing test steps properly integrated.The test step registrations cleanly integrate 9P file sharing verification into the scenario context, using
libhvee.ConvertToUnixPathfor cross-platform path conversion. The closure pattern for passingmountedHomeDirto step functions is appropriate.
1382-1398: VM helper functions are well-implemented.The
directoryExistInVM,filesystemIsMounted, andlistingFilesInDirectoryShouldSucceedfunctions provide clear error messages and appropriate checks for 9P mount verification.packaging/windows/product.wxs.template (1)
69-73: 9P hvsock registry entry correctly configured.The
Hvsock9pRegistryEntrycomponent properly registers the 9P hvsock service GUID (00009000-FACB-11E6-BD58-64006A7986D3) matchingconstants.Plan9HvsockGUID, and is correctly referenced inDefaultFeature. This follows the same pattern as the existingVsockRegistryEntry.Also applies to: 111-111
pkg/fileserver/fs9p/server.go (2)
61-75: Startup error check may race with server goroutine.The non-blocking
selectat lines 68-74 checks for an error immediately after spawning the goroutine, butproto.Servemay not have had a chance to fail yet. This means legitimate startup errors (e.g., bind failures) could be missed, with errors only surfacing later viaWaitForError().If
proto.Servefails quickly (e.g., listener already closed), the caller sees success fromStart()but an error later inWaitForError(). This might be acceptable for this use case, but consider documenting this behavior or adding a brief startup delay/handshake if synchronous error detection is important.
31-58: Server constructor with proper validation.
New9pServercorrectly validates that the exposed directory is an absolute path, exists, and is a directory. The buffered error channel (size 1) prevents goroutine leaks when no one reads fromErrChan.cmd/crc/cmd/daemon.go (1)
253-300: 9P server integration is well-structured.The implementation correctly:
- Gates on Windows platform and
EnableSharedDirsconfiguration- Sets up both hvsock (primary) and TCP (backup) 9P servers
- Uses proper error handling with immediate return on setup failures
- Defers
Stop()calls for graceful cleanup- Spawns goroutines for async error monitoring via
WaitForError()The dual-transport approach (hvsock + TCP fallback) provides good resilience for different VM connectivity scenarios.
vendor/github.com/DeedleFake/p9/README.md (1)
1-51: Vendored README provides context for the p9 library.This is standard vendored documentation from the upstream
github.com/DeedleFake/p9library. The static analysis warnings about markdown style are expected in third-party vendored content and should not be modified.vendor/github.com/DeedleFake/p9/dir_windows.go (1)
1-27: Vendored Windows-specific DirEntry conversion.This is vendored code from the upstream p9 library providing Windows-specific
os.FileInfotoDirEntryconversion with graceful fallback whenWin32FileAttributeDatais unavailable.vendor/github.com/DeedleFake/p9/dir_plan9.go (1)
1-49: Vendored Plan9-specific directory handling.This is vendored code from the upstream p9 library providing Plan9-native
infoToEntryandGetQIDimplementations. No modifications should be made to vendored code.vendor/github.com/DeedleFake/p9/fs.go (1)
169-186: LGTM: Safe string-to-bytes conversion using Go 1.20+ idiom.The
unsafe.Slice(unsafe.StringData(p), len(p))pattern at line 179 is the correct and safe way to get a byte slice view of a string in Go 1.20+. This avoids the allocation of[]byte(p)while being well-defined behavior, unlike the previously flagged*(*[]byte)(unsafe.Pointer(&p))pattern.vendor/github.com/DeedleFake/p9/dir.go (2)
239-260: LGTM: Flag mapping logic is straightforward.The
toOSFlagsfunction correctly maps 9P open modes to OS-level flags. The commented-out sections for OEXCL and OAPPEND suggest these may be added later.
146-180: LGTM: ReadOnlyFS wrapper correctly delegates while enforcing read-only semantics.The
ReadOnlyFSwrapper properly blocks write operations (WriteStat, Create, Remove) and restricts Open modes. ThepassQIDFShelper correctly preserves QIDFS capabilities when wrapping attachments.vendor/github.com/DeedleFake/p9/msg.go (1)
1-279: Vendored 9P protocol implementation looks correct.This vendored file implements the 9P2000 protocol message types and registry. The implementation correctly defines all standard message types, includes proper encoding/decoding with size bounds checking for Rstat/Twstat (lines 207-233, 240-276), and follows the 9P specification (including the intentional Terror slot skip on line 20).
|
/unhold |
|
@redbeam: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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. |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: anjannath The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
github-project-automation
Bot
moved this from Ready for review
to Done
in Project planning: crc
4 participants
Note
Together with this PR, we also need to merge
crc-org/snc#1193 to add the 9pfs client to the bundle.
This adds 9P file sharing support as discussed in #4858.
We are also removing SMB file sharing support for Windows in favor of 9P. Some of the work has already been done by @anjannath, and it is included here (#4620).
Fixes #4768
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.