FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

fix(netbsd): add NetBSD support to daemon IPC and runtime by cmeerw · Pull Request #1342 · DeusData/codebase-memory-mcp · GitHub

fix(netbsd): add NetBSD support to daemon IPC and runtime - #1342

Merged
DeusData merged 1 commit into
DeusData:mainfrom
cmeerw:fix/netbsd-daemon-support
Aug 28, 2026
Merged

fix(netbsd): add NetBSD support to daemon IPC and runtime#1342
DeusData merged 1 commit into
DeusData:mainfrom
cmeerw:fix/netbsd-daemon-support

Conversation

cmeerw commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Extend Linux codepaths in runtime.c to also cover NetBSD, and add NetBSD-specific peer PID retrieval via LOCAL_PEEREID in ipc.c.

cmeerw requested a review from DeusData as a code owner July 29, 2026 21:45

Copy link
Copy Markdown
Owner

Thank you for this — it is a careful, minimal port, and I want to tell you plainly where it stands rather than leaving it silent.

The code review came back clean, and I checked the part that would worry me most. The runtime.c changes are purely || defined(__NetBSD__) added to conditionals that already existed, so they are preprocessor-inert on Linux, macOS and Windows. The new ipc.c branch is guarded on SOL_LOCAL && LOCAL_PEEREID, which are NetBSD-only, and it sits after the Linux SO_PEERCRED and macOS LOCAL_PEERPID branches — so it is unreachable on the three platforms we ship. I verified that specifically, because a widened #ifdef quietly capturing an existing platform is the failure mode that would make a 16-line port dangerous. It does not happen here.

The NetBSD peer-credential logic itself reads correctly — unp_pid with a length check, failing closed to 0 rather than open.

What is holding it is a policy question, not your code. Merging means the project claims NetBSD support, and we have no NetBSD CI leg — so this path would be contributor-verified only, indefinitely. That is a maintainer decision about which platforms we are willing to stand behind without automated coverage, and I have put it in front of him rather than deciding it myself. I would rather give you a real answer than a quiet merge that leaves you as the sole guarantor of a platform forever.

Two things that would help whichever way it goes:

  • A line in the docs stating NetBSD is best-effort and untested by CI would make the commitment honest and explicit.
  • Worth noting in that same place: the shared path relies on /proc/<pid>/exe, which needs procfs mounted and that is not a NetBSD default. It degrades fail-closed — acquire simply returns false — so it is not unsafe, but a NetBSD user hitting it would otherwise have no idea why.

One practical heads-up: #1138 also edits the same #elif defined(__linux__) chains in runtime.c for a FreeBSD port, so whichever of the two moves first will conflict the other. Not something you need to act on now.

Thanks for the patience, and for keeping the change as small as it is — that is a large part of why the review was straightforward.

DeusData added enhancement New feature or request priority/backlog Valuable contribution, lower scheduling urgency; review when maintainer capacity opens. labels Aug 3, 2026
DeusData added this to the 0.9.2-rc milestone Aug 3, 2026

Copy link
Copy Markdown
Owner

Decision on the BSD pair: FreeBSD (#1467) just merged, and we want NetBSD too — with one fix first. Your NetBSD branch reuses the Linux /proc//exe path, but NetBSD's procfs is an optional mount and 'exe' is its Linux-compat name, so on a default install the identity check would silently fail — the exact class of quiet breakage the daemon code avoids elsewhere. The native idiom would be sysctl KERN_PROC_PATHNAME (see how #1467 did FreeBSD, including the O_NOFOLLOW/fstat double-check). You'll also need a small rebase over #1467's guard renames (runtime_linux_stat_same_image → runtime_posix_stat_same_image). With those two, this merges — with the same honest-limits note FreeBSD carries: no NetBSD CI leg, community-verified path. Thank you for bringing the platform this far.

cmeerw force-pushed the fix/netbsd-daemon-support branch from f140939 to 24f6819 Compare August 18, 2026 20:42

Copy link
Copy Markdown
Owner

Both code asks are addressed — you replaced /proc/<pid>/exe with the native sysctl KERN_PROC_PATHNAME idiom and rebased over #1467's runtime_posix_stat_same_image rename, and all 34 checks are green. Sorry for the three-day silence after you did that.

One correctness defect before merge, and no CI leg can catch it.

In runtime_process_image_reference_acquire, the NetBSD MIB is built with the PID slot hardcoded:

int mib[4] = {CTL_KERN, KERN_PROC_ARGS, -1, KERN_PROC_PATHNAME};

On NetBSD -1 in that slot means the calling process. But this function's contract is to resolve the image of the peer passed in as process_id — the FreeBSD arm immediately beside it does exactly that (int pid = (int)process_id; then {CTL_KERN, KERN_PROC, KERN_PROC_PATHNAME, pid}). As written, the NetBSD arm resolves the daemon's own executable, and runtime_process_image_reference_matches_process then compares that image against itself — so the peer-image identity check succeeds unconditionally on NetBSD. process_id also goes unused in that branch, which may trip an unused-parameter warning.

Worth noting the same -1 is correct in cli.c's cbm_detect_self_path, where self genuinely is the target. The pattern is right in one place and wrong in the other, which is exactly how this survives review.

One scope note, not a blocker. The new ipc.c branch is guarded by #elif defined(SOL_LOCAL) && defined(LOCAL_PEEREID) rather than by __NetBSD__. Linux takes the earlier SO_PEERCRED arm, macOS takes LOCAL_PEERPID, and Windows compiles a different function entirely — so none of the shipped platforms are affected. But OpenBSD also defines those macros and struct unpcbid, so it would newly reach this branch and start returning a real peer PID. Probably fine, arguably desirable, but broader than the title implies and worth a word in the commit message.

On the honest-limits documentation note I asked for: I checked, and no such note exists for FreeBSD either — docs/, README.md and CONTRIBUTING.md mention neither BSD. So that was me asking you to establish a convention that does not exist yet. Skip it; if we want a best-effort platforms note it should land once, covering both, and that is our job rather than yours.

Fix the MIB and I will merge.

Extend Linux/FreeBSD codepaths in runtime.c to also cover NetBSD, and add
NetBSD-specific peer PID retrieval via LOCAL_PEEREID in ipc.c.

Signed-off-by: Christof Meerwald <cmeerw@cmeerw.org>
cmeerw force-pushed the fix/netbsd-daemon-support branch from 24f6819 to 3e1703d Compare August 22, 2026 13:09
DeusData merged commit f899770 into DeusData:main Aug 28, 2026
62 of 65 checks passed

Copy link
Copy Markdown
Owner

Merged in f899770. Thank you, @cmeerw — NetBSD support lands with every review condition met, and met precisely.

The MIB fix was verified hunk-by-hunk before merge: the self-path in cli.c keeps -1, the peer-identity check in runtime.c takes the real peer pid, and the LOCAL_PEEREID branch fails closed on any getsockopt anomaly — exactly the split the 21 August review asked for. Every hunk is preprocessor-gated; the preprocessed source on the shipped platforms is byte-identical to main, which is what let this merge confidently without a NetBSD CI leg.

The two CI reds on the way in were both documented flake classes on our attribution ledger (a wall-clock-windowed lock_registry case and the Windows daemon-stability cluster), unrelated to a provably-inert diff, and both cleared on rerun.

Same honest-limits framing as FreeBSD's runtime support: the NetBSD paths are community-verified — your verification — with no CI leg, by explicit decision. The best-effort-platforms docs note covering both BSDs is on our side and will land once, separately. The OpenBSD reachability note stays waived per the review.

A model platform contribution: native sysctl over procfs when asked, a clean rebase over the identity rename, and fail-closed semantics throughout.

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request priority/backlog Valuable contribution, lower scheduling urgency; review when maintainer capacity opens.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants


Back | FazBrowse Home | New Git URL