Remove SMB file sharing for Windows #4879
Conversation
WalkthroughReplaces Windows SMB sharing with Plan 9 (9P) sharing: adds a vendored 9P library, implements a 9P fileserver and startup plumbing (including hvsock support), updates VM mount logic to 9P with TCP fallback, removes SMB/MSI/custom-action and password plumbing, and adds Plan9 constants. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant CLI as crc start
participant Daemon as crc daemon (host)
participant Fileserver as fs9p
participant VM as VM (guest)
User->>CLI: run start
CLI->>Daemon: configureSharedDirs (type=9p)
Daemon->>Fileserver: StartHvsockShares(HvsockMount9p(home,HvsockGUID))
alt hvsock succeeds
Fileserver->>Daemon: hvsock listener created
Daemon->>Fileserver: StartShares(Mount9p{Listener:hvsock,Path:home})
Fileserver-->>Daemon: started (async)
VM->>Fileserver: mount 9p over hvsock (port Plan9HvsockPort)
else hvsock fails
Daemon->>Fileserver: listen TCP on GatewayIP:Plan9TcpPort
Fileserver->>Daemon: listener bound
Daemon->>Fileserver: StartShares(Mount9p{Listener:tcp,Path:home})
Fileserver-->>Daemon: started (async)
VM->>Fileserver: mount 9p TCP (GatewayIP:Plan9TcpPort)
end
Fileserver-->>VM: expose host directory over 9P
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Assessment against linked issues
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 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.
| // 9p home directory sharing | ||
| listener9p, err := vn.Listen("tcp", net.JoinHostPort(configuration.GatewayIP, fmt.Sprintf("%d", constants.Plan9Port))) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| if _, err := fs9p.StartShares([]fs9p.Mount9p{{Listener: listener9p, Path: constants.GetHomeDir()}}); err != nil { | ||
| return err | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Don’t drop server handles; add graceful shutdown for 9P servers
You start 9P shares but discard the returned servers, so they can’t be stopped on shutdown. Capture them and defer Stop() to avoid leaking goroutines and to ensure clean teardown.
Apply this diff:
// 9p home directory sharing
listener9p, err := vn.Listen("tcp", net.JoinHostPort(configuration.GatewayIP, fmt.Sprintf("%d", constants.Plan9Port)))
if err != nil {
return err
}
-if _, err := fs9p.StartShares([]fs9p.Mount9p{{Listener: listener9p, Path: constants.GetHomeDir()}}); err != nil {
- return err
-}
+servers9p, err := fs9p.StartShares([]fs9p.Mount9p{{Listener: listener9p, Path: constants.GetHomeDir()}})
+if err != nil {
+ return err
+}
+defer func() {
+ for _, s := range servers9p {
+ if err := s.Stop(); err != nil {
+ logging.Errorf("Error stopping 9p server: %v", err)
+ }
+ }
+}()Optional: If you want the daemon to exit on 9P server failure (not just log), consider plumbing fs9p server errors into errCh (requires fs9p.StartShares to accept a callback or return error channels).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // 9p home directory sharing | |
| listener9p, err := vn.Listen("tcp", net.JoinHostPort(configuration.GatewayIP, fmt.Sprintf("%d", constants.Plan9Port))) | |
| if err != nil { | |
| return err | |
| } | |
| if _, err := fs9p.StartShares([]fs9p.Mount9p{{Listener: listener9p, Path: constants.GetHomeDir()}}); err != nil { | |
| return err | |
| } | |
| // 9p home directory sharing | |
| listener9p, err := vn.Listen("tcp", net.JoinHostPort(configuration.GatewayIP, fmt.Sprintf("%d", constants.Plan9Port))) | |
| if err != nil { | |
| return err | |
| } | |
| servers9p, err := fs9p.StartShares([]fs9p.Mount9p{{Listener: listener9p, Path: constants.GetHomeDir()}}) | |
| if err != nil { | |
| return err | |
| } | |
| defer func() { | |
| for _, s := range servers9p { | |
| if err := s.Stop(); err != nil { | |
| logging.Errorf("Error stopping 9p server: %v", err) | |
| } | |
| } | |
| }() |
🤖 Prompt for AI Agents
In cmd/crc/cmd/daemon.go around lines 252 to 260, the code starts 9P shares but
discards the returned server handles; capture the returned servers from
fs9p.StartShares into a variable, check the error as now, and immediately defer
a cleanup loop that calls Stop() (or Close()) on each returned server to ensure
graceful shutdown and avoid leaking goroutines; if there’s an existing errCh for
daemon components, optionally wire server errors into it (or modify StartShares
to accept a callback/return error channels) so 9P failures can cause daemon
shutdown instead of only logging.
| @@ -4,6 +4,7 @@ go 1.23.0 | |||
|
|
|||
| require ( | |||
| github.com/AlecAivazis/survey/v2 v2.3.7 | |||
| github.com/DeedleFake/p9 v0.6.12 | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Add p9 dependency: OK. Please double-check any lingering SMB/CIFS references.
The module addition aligns with the move to 9P. As a precaution for this PR’s objective (remove SMB on Windows), scan the tree for any remaining SMB/CIFS usage or password plumbing.
Run this script to verify no SMB/CIFS remnants remain and that 9p is wired on Windows:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Expect: no matches (outside vendor)
rg -n --hidden --glob '!vendor/**' -e '\bSMB\b' -e '\bsmb\b' -e '\bcifs\b' -e 'mount\.cifs' -e 'SharedDirPassword' -e 'SharedDirUsername' -e 'net use' -e 'MSI.*custom.*action'
# Expect: matches in Windows driver code
rg -n --glob '!vendor/**' -e 'Type:\s*"9p"' pkg/crc/machineLength of output: 820
Add p9 dependency: OK — remove leftover SMB/CIFS credential plumbing
9p is wired on Windows, but the repo still contains SharedDir credential fields/uses; these need to be removed or refactored.
Files to fix:
- pkg/crc/machine/start.go:317-318 — SharedDirPassword / SharedDirUsername passed through
- pkg/crc/machine/types/types.go:37-38 — SharedDirPassword / SharedDirUsername fields
- pkg/crc/machine/config/config.go:24-25 — SharedDirPassword / SharedDirUsername config fields
- pkg/crc/config/settings.go:34 — SharedDirPassword setting (sensitive)
- pkg/crc/machine/libhvee/driver_windows.go:44 — Type: "9p" (expected; confirms 9p is in use)
Action: remove or replace SMB/CIFS credential plumbing (structs, config, callers, and settings) or explicitly document why they must remain. Re-run the search after changes to confirm no SMB/CIFS remnants remain.
🤖 Prompt for AI Agents
In go.mod line 7 and across the codebase (specifically pkg/crc/machine/start.go
lines ~317-318, pkg/crc/machine/types/types.go lines ~37-38,
pkg/crc/machine/config/config.go lines ~24-25, pkg/crc/config/settings.go line
~34, and pkg/crc/machine/libhvee/driver_windows.go line ~44) there are leftover
SharedDirUsername/SharedDirPassword SMB/CIFS credential fields and plumbing that
are no longer needed because 9p is used; remove the SharedDirUsername and
SharedDirPassword fields from structs and config, delete or refactor any callers
that pass these credentials (e.g., remove arguments, calls, and validation
related to them), remove the sensitive setting from settings.go (or mark
deprecated with migration if you must preserve data), and update
driver_windows.go to rely on 9p-only configuration; after making these
removals/refactors, run a repository-wide search to ensure no remaining SMB/CIFS
credential references exist and update any comments/docs to reflect the removal
or provide a short note documenting why SMB fields were removed.
| for i, dir := range machineConfig.SharedDirs { | ||
| sharedDir := drivers.SharedDir{ | ||
| Source: dir, | ||
| Target: convertToUnixPath(dir), | ||
| Tag: "crc-dir0", // smb share 'crc-dir0' is created in the msi | ||
| Type: "cifs", | ||
| Username: machineConfig.SharedDirUsername, | ||
| Source: dir, | ||
| Target: convertToUnixPath(dir), | ||
| Tag: fmt.Sprintf("dir%d", i), | ||
| Type: "9p", | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
9P migration: ensure tag/name symmetry with the 9P server and make tag generation deterministic.
- The Tag "dir%d" must match the server’s share names (9P aname). If server-side naming diverges, mounts will fail. Recommend centralizing the tag format or deriving it from a shared helper/constant to avoid drift.
- Consider sorting the list before assigning tags to keep mapping stable across runs/config reloads. Otherwise, reordering SharedDirs can change tags and surprise the guest mount logic.
Apply this refactor to stabilize ordering:
@@
-import (
- "fmt"
+import (
+ "fmt"
+ "sort"
@@
func configureShareDirs(machineConfig config.MachineConfig) []drivers.SharedDir {
var sharedDirs []drivers.SharedDir
- 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",
}Optionally, define a shared constant/func for the tag format (e.g., constants.Plan9ShareTagFmt = "dir%d") and use it both here and in the fileserver to prevent divergence. I can draft that change if you want.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for i, dir := range machineConfig.SharedDirs { | |
| sharedDir := drivers.SharedDir{ | |
| Source: dir, | |
| Target: convertToUnixPath(dir), | |
| Tag: "crc-dir0", // smb share 'crc-dir0' is created in the msi | |
| Type: "cifs", | |
| Username: machineConfig.SharedDirUsername, | |
| Source: dir, | |
| Target: convertToUnixPath(dir), | |
| Tag: fmt.Sprintf("dir%d", i), | |
| Type: "9p", | |
| } | |
| import ( | |
| "fmt" | |
| "sort" | |
| ) | |
| func configureShareDirs(machineConfig config.MachineConfig) []drivers.SharedDir { | |
| var sharedDirs []drivers.SharedDir | |
| 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", | |
| } | |
| sharedDirs = append(sharedDirs, sharedDir) | |
| } | |
| return sharedDirs | |
| } |
🤖 Prompt for AI Agents
In pkg/crc/machine/libhvee/driver_windows.go around lines 39 to 45, the current
Tag generation uses the unstable iteration index ("dir%d") which can diverge
from the 9P server's share names and be non-deterministic across runs; sort
machineConfig.SharedDirs (or a copy) deterministically (e.g., lexicographically
by Source) before assigning tags, and replace the inline "dir%d" format with a
shared constant or helper function (e.g., Plan9ShareTagFmt or
MakePlan9Tag(sourceOrIndex)) that the fileserver also uses so tag/aname symmetry
is guaranteed. Ensure you do not mutate the original slice unless intended (use
a sorted copy) and keep tag generation deterministic and centralized.
| case "9p": | ||
| // change owner to core user to allow mounting to it as a non-root user | ||
| if _, _, err := sshRunner.RunPrivileged("Changing owner of mount directory", "chown core:core", mount.Target); err != nil { | ||
| return err | ||
| } | ||
| if _, _, err := sshRunner.Run("9pfs 192.168.127.1", mount.Target); err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
9P mount: missing options, not privileged, and doesn’t use tag/aname
- Mounting typically requires root; using sshRunner.Run (non-privileged) will likely fail.
- The command lacks essential 9P options (trans=tcp, port, version, msize, cache).
- The share/tag (aname) is not passed, so the server can't resolve which export to mount.
- Using a non-standard “9pfs” helper is brittle; prefer explicit mount invocation.
Apply this diff to use a privileged, explicit 9P mount with tag and CRC constants:
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.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
+ }Follow-ups to consider:
- Idempotency: skip if already mounted (mountpoint -q). I can propose a patch if you want it built-in.
- If SELinux confines containers, verify if additional labeling is needed for the 9p mount; 9p often doesn't support SELinux contexts like virtiofs does. We may need a chcon workaround or document the limitation.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| case "9p": | |
| // change owner to core user to allow mounting to it as a non-root user | |
| if _, _, err := sshRunner.RunPrivileged("Changing owner of mount directory", "chown core:core", mount.Target); err != nil { | |
| return err | |
| } | |
| if _, _, err := sshRunner.Run("9pfs 192.168.127.1", mount.Target); err != nil { | |
| return err | |
| } | |
| case "9p": | |
| // change owner to core user to allow 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.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 | |
| } |
pkg/fileserver/fs9p/server.go
Outdated
| errChan := make(chan error) | ||
|
|
||
| fs := p9.FileSystem(p9.Dir(exposeDir)) | ||
|
|
||
| go func() { | ||
| errChan <- proto.Serve(listener, p9.Proto(), p9.FSConnHandler(fs, constants.Plan9Msize)) | ||
| close(errChan) | ||
| }() |
There was a problem hiding this comment.
Prevent goroutine leaks and treat net.ErrClosed as a normal shutdown
- The server goroutine writes exactly one error to ErrChan when proto.Serve returns (typically net.ErrClosed after Stop). If no one is receiving (WaitForError never called), the goroutine can block forever on the send. Also, WaitForError will surface net.ErrClosed as an error even on a clean Stop.
- Make ErrChan buffered (size 1) and ignore net.ErrClosed in WaitForError.
Apply this diff:
@@
- 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 nobody is waiting, the server goroutine can still deliver its terminal error.
+ // Treat net.ErrClosed (listener closed due to Stop) as a clean shutdown.
+ if err, ok := <-s.ErrChan; ok {
+ if errors.Is(err, net.ErrClosed) {
+ return nil
+ }
+ return err
+ }
+ return nil
+}Also add the import at the top:
@@
-import (
+import (
+ "errors"
"fmt"
"net"
"os"
"path/filepath"Also applies to: 76-80
🤖 Prompt for AI Agents
In pkg/fileserver/fs9p/server.go around lines 39-46 (and similarly at 76-80),
the goroutine sending the Serve error can block if nobody receives and
WaitForError treats net.ErrClosed as an error; change errChan to a buffered
channel of size 1 to avoid goroutine leaks, update WaitForError to treat
net.ErrClosed as a normal shutdown (ignore it and return nil), and add the
necessary import for the net package at the top of the file so you can compare
errors against net.ErrClosed.
| if (msize > 0) && (size > msize) { | ||
| return nil, NoTag, util.Errorf("receive: %w", ErrLargeMessage) | ||
| } | ||
|
|
||
| lr := &util.LimitedReader{ | ||
| R: r, | ||
| N: size, | ||
| E: ErrLargeMessage, | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Off-by-4 framing bug: LimitedReader should bound to size-4 (size includes itself).
9P’s size field includes the 4 bytes of the size itself. After reading size, the remaining frame length is size-4. Using N: size allows reads to leak 4 bytes into the next frame on exact-boundary payloads. This breaks message framing and can cause hard-to-debug, nondeterministic errors.
Apply this diff to bound reads correctly and add a guard for undersized frames:
- 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,
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (msize > 0) && (size > msize) { | |
| return nil, NoTag, util.Errorf("receive: %w", ErrLargeMessage) | |
| } | |
| lr := &util.LimitedReader{ | |
| R: r, | |
| N: size, | |
| E: ErrLargeMessage, | |
| } | |
| if (msize > 0) && (size > msize) { | |
| return nil, NoTag, util.Errorf("receive: %w", ErrLargeMessage) | |
| } | |
| if size < 4 { | |
| return nil, NoTag, util.Errorf("receive: invalid frame size: %d", size) | |
| } | |
| lr := &util.LimitedReader{ | |
| R: r, | |
| N: size - 4, | |
| E: ErrLargeMessage, | |
| } |
🤖 Prompt for AI Agents
In vendor/github.com/DeedleFake/p9/proto/proto.go around lines 85 to 94, the
LimitedReader is currently bounded with N: size which is off-by-4 because the 9P
size field includes its own 4 bytes; change the reader bound to N: size-4 and
add a guard that returns an error if size < 4 (undersized frame) before
constructing the LimitedReader so we never allow negative/invalid bounds or leak
4 bytes into the next frame.
| import ( | ||
| "io" | ||
| "log" | ||
| "net" | ||
| "sync" | ||
| ) | ||
|
|
There was a problem hiding this comment.
Break on non-EOF read errors and don’t dispatch a handler on failed Receive
Currently, after a Receive error, the loop logs and then proceeds to call the handler with a garbage message/tag, which can panic or send invalid responses. Also, closed connections (e.g., net.ErrClosed) should terminate the loop, not busy-loop and spam logs.
Apply this diff:
@@
-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.
+ if err == io.EOF || errors.Is(err, net.ErrClosed) {
+ return
+ }
+ // For other errors, log and continue to try receiving again.
+ log.Printf("Error reading message: %v", err)
+ continue
}
mode(func() {
rmsg := handler.HandleMessage(tmsg)Also applies to: 71-79, 79-99
🤖 Prompt for AI Agents
In vendor/github.com/DeedleFake/p9/proto/server.go around lines 3-9, 71-79 and
79-99, the loop continues after a Receive error and calls the handler with
invalid data, and it busy-loops on closed connections; update the
receive-and-dispatch loop to: check the error returned by Receive and if err !=
nil then if errors.Is(err, io.EOF) or errors.Is(err, net.ErrClosed) break the
loop (cleanly close/return), otherwise log the error and break (do not call the
handler); only call the handler when Receive succeeds (no error) and ensure you
don’t use stale/garbage msg/tag after a failed Receive.
| w := []string{path.Clean(p)} | ||
| if w[0] != "/" { | ||
| w = strings.Split(w[0], "/") | ||
| } | ||
| if (len(w) == 1) && (w[0] == ".") { | ||
| w = nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Absolute path handling in walk() sends invalid Wname (empty element or “/”).
path.Clean("/a/b") yields "/a/b"; the current code splits it into ["", "a", "b"] due to the leading slash, causing an empty name element in Twalk. For "/", it sends Wname = ["/"], which is also invalid. Twalk Wname must be path elements without empty strings or slashes.
- 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 = nil
+ } else {
+ w = strings.Split(p, "/")
+ }
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 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 = nil | |
| } else { | |
| w = strings.Split(p, "/") | |
| } | |
| } |
🤖 Prompt for AI Agents
In vendor/github.com/DeedleFake/p9/remote.go around lines 37 to 43, the code
builds Wname incorrectly for absolute paths (leading "/" produces empty first
element and "/" alone yields invalid name); update the logic to Clean the path,
then split on "/" but remove any empty elements and any elements equal to "/" so
Wname contains only valid path components, and treat the root path (no
components) as nil/empty Wname for Twalk; ensure you handle both relative and
absolute inputs consistently by trimming a leading slash before splitting or by
filtering out empty tokens after Split.
| panic(util.Errorf("Invalid whence: %v", whence)) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Don’t panic on invalid whence; return an error.
Library code should not panic for invalid input. Return the current position and a descriptive error instead.
- panic(util.Errorf("Invalid whence: %v", whence))
+ return int64(file.pos), util.Errorf("invalid whence: %v", whence)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| panic(util.Errorf("Invalid whence: %v", whence)) | |
| } | |
| return int64(file.pos), util.Errorf("invalid whence: %v", whence) | |
| } |
🤖 Prompt for AI Agents
In vendor/github.com/DeedleFake/p9/remote.go around lines 184-185, replace the
panic call for an invalid whence with returning the current position and an
error; specifically, remove panic(util.Errorf(...)) and instead return the
current offset/position variable (the value the function would report as the
current file position) along with util.Errorf("invalid whence: %v", whence) so
callers receive an error instead of the process aborting.
| func (file *Remote) maxBufSize() int { | ||
| return int(file.client.Msize() - IOHeaderSize) | ||
| } | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Guard against negative/invalid max buffer size.
If Msize() is misnegotiated or too small, int(file.client.Msize() - IOHeaderSize) can go negative, leading to slicing errors or infinite loops in chunked I/O. Return 0 in such cases.
-func (file *Remote) maxBufSize() int {
- return int(file.client.Msize() - IOHeaderSize)
-}
+func (file *Remote) maxBufSize() int {
+ m := int(file.client.Msize())
+ if m <= IOHeaderSize {
+ return 0
+ }
+ return m - IOHeaderSize
+}Please verify negotiated msize is always > IOHeaderSize on all supported backends.
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Searching for IOHeaderSize ==="
rg -n --hidden --no-ignore -S "IOHeaderSize" || true
echo
echo "=== Showing vendor/github.com/DeedleFake/p9/remote.go (first 300 lines) ==="
sed -n '1,300p' vendor/github.com/DeedleFake/p9/remote.go || true
echo
echo "=== Showing vendor/github.com/DeedleFake/p9/remote.go (around reported lines 180-220) ==="
sed -n '160,220p' vendor/github.com/DeedleFake/p9/remote.go || true
echo
echo "=== Searching for maxBufSize usages ==="
rg -n --hidden --no-ignore -S "maxBufSize" || true
echo
echo "=== Searching for Msize definitions/usages in vendor ==="
rg -n --hidden --no-ignore -S "Msize" vendor || true
echo
echo "=== Searching for type of client in repo (look for struct/interface fields named client) ==="
rg -n --hidden --no-ignore -S "client" || true
echo
echo "=== Grep for 'type Remote' to show its definition ==="
rg -n --hidden --no-ignore -S "type Remote" || true
sed -n '1,240p' vendor/github.com/DeedleFake/p9/remote.go | sed -n '1,120p' || trueLength of output: 273701
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== vendor/github.com/DeedleFake/p9/p9.go ==="
sed -n '1,240p' vendor/github.com/DeedleFake/p9/p9.go || true
echo
echo "=== vendor/github.com/DeedleFake/p9/proto/client.go ==="
sed -n '1,240p' vendor/github.com/DeedleFake/p9/proto/client.go || true
echo
echo "=== vendor/github.com/DeedleFake/p9/client.go ==="
sed -n '1,240p' vendor/github.com/DeedleFake/p9/client.go || true
echo
echo "=== vendor/github.com/DeedleFake/p9/fs.go (around msize logic) ==="
sed -n '1,320p' vendor/github.com/DeedleFake/p9/fs.go || true
echo
echo "=== Search for SetMsize or SetMsize usages ==="
rg -n --hidden --no-ignore -S "SetMsize|Set Msize|SetMsize" vendor/github.com/DeedleFake/p9 || true
echo
echo "=== Show vendor/github.com/DeedleFake/p9/proto/server.go (relevant Msize negotiation) ==="
sed -n '1,240p' vendor/github.com/DeedleFake/p9/proto/server.go || trueLength of output: 21739
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== List proto files ==="
ls -la vendor/github.com/DeedleFake/p9/proto || true
echo
echo "=== Show vendor/github.com/DeedleFake/p9/proto/proto.go ==="
sed -n '1,240p' vendor/github.com/DeedleFake/p9/proto/proto.go || true
echo
echo "=== Show vendor/github.com/DeedleFake/p9/proto/msg.go ==="
sed -n '1,300p' vendor/github.com/DeedleFake/p9/proto/msg.go || true
echo
echo "=== Search for Receive and Send implementations in proto ==="
rg -n --hidden --no-ignore -S "func .*Receive|func .*Send" vendor/github.com/DeedleFake/p9/proto || trueLength of output: 5465
Guard negotiated msize and avoid underflow / infinite-loop
Briefly: IOHeaderSize is 24 and Client.Msize() is a uint32. The current expression int(file.client.Msize() - IOHeaderSize) can underflow/wrap when the negotiated msize is <= IOHeaderSize. Returning 0 (as in the original suggestion) would cause an infinite loop in ReadAt/WriteAt (they do "start += size"). Fixes needed.
Files to update:
- vendor/github.com/DeedleFake/p9/remote.go — maxBufSize()
- vendor/github.com/DeedleFake/p9/client.go — Handshake() (validate negotiated msize)
Suggested changes (replace the previous diff — do NOT return 0; either validate at handshake or clamp to a safe positive chunk):
--- a/vendor/github.com/DeedleFake/p9/remote.go
+++ b/vendor/github.com/DeedleFake/p9/remote.go
@@
func (file *Remote) maxBufSize() int {
- return int(file.client.Msize() - IOHeaderSize)
+ m := file.client.Msize()
+ if m <= uint32(IOHeaderSize) {
+ // negotiated msize too small / invalid — avoid underflow or returning 0
+ // (returning 0 would make ReadAt/WriteAt loop forever). Return 1
+ // so callers make forward progress; callers should still treat a
+ // tiny negotiated msize as an error (see Handshake validation below).
+ return 1
+ }
+ return int(m - uint32(IOHeaderSize))
}Optional but recommended: fail the handshake early so an invalid/too-small msize is rejected instead of working around it:
--- a/vendor/github.com/DeedleFake/p9/client.go
+++ b/vendor/github.com/DeedleFake/p9/client.go
@@
-import (
- "errors"
- "net"
- "sync/atomic"
-
- "github.com/DeedleFake/p9/proto"
-)
+import (
+ "errors"
+ "fmt"
+ "net"
+ "sync/atomic"
+
+ "github.com/DeedleFake/p9/proto"
+)
@@
version := rsp.(*Rversion)
if version.Version != Version {
return 0, ErrUnsupportedVersion
}
+
+ // Defensive: negotiated msize must be larger than the 9P IO header.
+ if version.Msize <= uint32(IOHeaderSize) {
+ return 0, fmt.Errorf("negotiated msize too small: %d", version.Msize)
+ }
c.SetMsize(version.Msize)
return version.Msize, nil
}Reasoning: underflow/wrap from unsigned arithmetic or int conversion can produce huge or negative values (on 32-bit), and returning 0 will break the chunking loops. Clamping to a safe positive value prevents infinite loops; failing the handshake is preferable so callers don't run with an unusable msize.
Please implement one or both of the above changes and verify that all supported server backends never negotiate Msize <= IOHeaderSize.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (file *Remote) maxBufSize() int { | |
| return int(file.client.Msize() - IOHeaderSize) | |
| } | |
| func (file *Remote) maxBufSize() int { | |
| m := file.client.Msize() | |
| if m <= uint32(IOHeaderSize) { | |
| // negotiated msize too small / invalid — avoid underflow or returning 0 | |
| // (returning 0 would make ReadAt/WriteAt loop forever). Return 1 | |
| // so callers make forward progress; callers should still treat a | |
| // tiny negotiated msize as an error (see Handshake validation below). | |
| return 1 | |
| } | |
| return int(m - uint32(IOHeaderSize)) | |
| } |
🤖 Prompt for AI Agents
In vendor/github.com/DeedleFake/p9/remote.go around lines 198-201, avoid
unsigned underflow by not computing int(file.client.Msize() - IOHeaderSize)
directly; instead compute a safe positive chunk size (e.g. if
file.client.Msize() > IOHeaderSize return int(file.client.Msize() -
IOHeaderSize) else return a small positive minimum) or better yet validate
earlier. In vendor/github.com/DeedleFake/p9/client.go Handshake(), validate the
negotiated Msize immediately and return an error if Msize <= IOHeaderSize
(reject the handshake) so callers never run with an unusable msize; if you
choose to clamp in maxBufSize, ensure it never returns 0 and cannot trigger
infinite loops in ReadAt/WriteAt.
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. |
this will be moved to the 'crc setup' command as we should only enable file sharing and create the smb share for home dir when users have set the config option 'enable-shared-dirs' to 'true' this should remove at least of the warnings reported by virustotal for the MSI: https://www.virustotal.com/gui/file/31b402dcc1da24265074a21a26018d6cde8eef0b63c77a18f89eb079b6556790
After the addition of 9p file sharing for Windows, we are removing SMB file sharing support (#4768).
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.
| <Component Id="Hvsock9pRegistryEntry" Guid="*"> | ||
| <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> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Write 9P HVSOCK registry to 64‑bit hive (avoid WOW6432 redirection).
Without marking the component as 64‑bit, the key may land under Wow6432Node and not be honored by Guest Communication Services.
Apply this diff:
- <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>Optional (recommended): also mark VsockRegistryEntry as Win64 for consistency:
- <Component Id="VsockRegistryEntry" Guid="*">
+ <Component Id="VsockRegistryEntry" Guid="*" Win64="yes">🤖 Prompt for AI Agents
In packaging/windows/product.wxs.template around lines 69 to 73, the
Hvsock9pRegistryEntry component currently writes to the registry without
specifying 64-bit, which can cause the key to be redirected under Wow6432Node;
update the Component element to include Win64="yes" so the registry key is
written to the 64-bit hive, and for consistency also set Win64="yes" on the
VsockRegistryEntry component.
| import ( | ||
| "fmt" | ||
|
|
||
| "github.com/linuxkit/virtsock/pkg/hvsock" | ||
| "github.com/sirupsen/logrus" | ||
| ) |
There was a problem hiding this comment.
Close pre-created listeners on early errors to avoid handle leaks
If any GUID parse or hvsock.Listen fails mid-loop, previously created listeners are leaked because StartShares is never called. Collect and close them on failure.
import (
"fmt"
+ "net"
"github.com/linuxkit/virtsock/pkg/hvsock"
"github.com/sirupsen/logrus"
)
@@
-func StartHvsockShares(mounts []HvsockMount9p) ([]*Server, error) {
- mounts9p := []Mount9p{}
+func StartHvsockShares(mounts []HvsockMount9p) ([]*Server, error) {
+ mounts9p := make([]Mount9p, 0, len(mounts))
+ created := make([]net.Listener, 0, len(mounts))
for _, mount := range mounts {
service, err := hvsock.GUIDFromString(mount.HvsockGUID)
if err != nil {
return nil, fmt.Errorf("parsing hvsock guid %s: %w", mount.HvsockGUID, err)
}
@@
listener, err := hvsock.Listen(hvsock.Addr{
VMID: hvsock.GUIDWildcard,
ServiceID: service,
})
if err != nil {
- return nil, fmt.Errorf("retrieving listener for hvsock %s: %w", mount.HvsockGUID, err)
+ for _, l := range created {
+ _ = l.Close()
+ }
+ return nil, fmt.Errorf("creating listener for hvsock %s: %w", mount.HvsockGUID, err)
}
logrus.Debugf("Going to serve directory %s on hvsock %s", mount.Path, mount.HvsockGUID)
- mounts9p = append(mounts9p, Mount9p{Path: mount.Path, Listener: listener})
+ mounts9p = append(mounts9p, Mount9p{Path: mount.Path, Listener: listener})
+ created = append(created, listener)
}
return StartShares(mounts9p)
}Also applies to: 12-30
🤖 Prompt for AI Agents
In pkg/fileserver/fs9p/shares_windows.go around lines 3 to 8 (and similarly 12
to 30), the function that creates hvsock listeners can fail mid-loop and leak
previously created listeners; update the code to track any successfully created
listeners in a slice (or similar), and if any subsequent GUID parse or
hvsock.Listen call fails, iterate that slice and close each listener before
returning the error; alternatively, allocate the slice before the loop and use a
deferred cleanup function that closes all entries if the function exits with an
error, ensuring StartShares is only called when no errors occurred.
| size := len(data) | ||
| if size > file.maxBufSize() { | ||
| size = file.maxBufSize() | ||
| } | ||
|
|
There was a problem hiding this comment.
Mirror the chunk-size guard in WriteAt.
- size := len(data)
- if size > file.maxBufSize() {
- size = file.maxBufSize()
- }
+ size := len(data)
+ max := file.maxBufSize()
+ if max <= 0 {
+ return 0, util.Errorf("msize too small: %d", file.client.Msize())
+ }
+ if size > max {
+ size = max
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| size := len(data) | |
| if size > file.maxBufSize() { | |
| size = file.maxBufSize() | |
| } | |
| size := len(data) | |
| max := file.maxBufSize() | |
| if max <= 0 { | |
| return 0, util.Errorf("msize too small: %d", file.client.Msize()) | |
| } | |
| if size > max { | |
| size = max | |
| } |
🤖 Prompt for AI Agents
In vendor/github.com/DeedleFake/p9/remote.go around lines 289 to 293, the Read
chunk-size handling only sets size := len(data) and caps it to
file.maxBufSize(), but it needs to mirror the exact guard logic used in WriteAt:
compute the chunk size the same way WriteAt does (respecting maxBufSize and any
remaining-file or offset boundaries), and replace the current simple cap with
that same calculation so reads use identical chunk limits as writes.
|
@redbeam: The following tests failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
Fixes #4768
We are 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).
Summary by CodeRabbit
New Features
Refactor
Chores