| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
|
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:
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. |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
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>
|
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. |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
Extend Linux codepaths in runtime.c to also cover NetBSD, and add NetBSD-specific peer PID retrieval via LOCAL_PEEREID in ipc.c.