fix(netbsd): add NetBSD support to daemon IPC and runtime - #1342
Conversation
|
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 The NetBSD peer-credential logic itself reads correctly — 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 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. |
|
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. |
f140939 to
24f6819
Compare
|
Both code asks are addressed — you replaced One correctness defect before merge, and no CI leg can catch it. In int mib[4] = {CTL_KERN, KERN_PROC_ARGS, -1, KERN_PROC_PATHNAME};On NetBSD Worth noting the same One scope note, not a blocker. The new On the honest-limits documentation note I asked for: I checked, and no such note exists for FreeBSD either — 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>
24f6819 to
3e1703d
Compare
|
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 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. |
…istic lock_registry_absolute_deadline_survives_repeated_wakes has failed on the test-unix (macos-15-intel) leg of three unrelated pull requests in one week -- #1342 (08-28), #1811 (09-02) and #1819 (09-03, run 33799475629, job 100823626965) -- always with the same signature: FAIL tests/test_lock_registry.c:1153: ASSERT(tail_queued) 7648 passed / 1 failed The registry is not racy. cbm_lock_registry_acquire enqueues the waiter synchronously under the registry mutex before any wait, so waiter_count and the attempting count are exact. The defect is in the fixture: it raced two wall-clock windows against each other, both anchored to a timestamp the observer thread took before the tail thread had even been scheduled. deadline_start = cbm_now_ms(); tail.deadline_ms = deadline_start + 200; /* the tail's acquire deadline */ queue_deadline = deadline_start + 100; /* the observer's budget */ Because both windows start before the tail runs, a loaded runner breaks the fixture two different ways: * the tail is scheduled inside its deadline but after the observer's 100 ms budget has expired -- the queued state existed and was simply no longer being looked at; or * the tail is scheduled more than 200 ms late, in which case its deadline has already passed when it finally calls acquire, the pre-registration deadline check in lock_registry_acquire_internal returns BUSY immediately, and the tail never enqueues at all -- the asserted state can then never occur, however long the observer waits. lock_registry is in the parallel wave of run-tests-parallel.sh, not the serial tail, so on a small CI runner it competes with a full wave of sanitized suites -- exactly the scheduling delay both paths need. Widening the observer's budget would only paper over the first path, and the state it waits for is transient by construction: it exists only between the tail's enqueue and the tail's own deadline. So the fixture is rebuilt to observe states that cannot evaporate. * The tail anchors its absolute deadline itself, in its own thread, immediately before the acquire it bounds. Scheduling delay can no longer consume the deadline before the call starts, so the enqueue is unconditional, and elapsed is measured from the tail's own anchor -- it now times the registry instead of timing the scheduler. * Enqueue is exposed as a monotonic counter, cbm_lock_registry_waiter_enqueue_count_for_test, next to the existing test_condition_wait_calls counter it is modelled on. Because the count only ever grows, the observer reads it once after the tail has returned rather than trying to catch a live queue depth: the polling loop, and with it the window, is gone. * The fixture's remaining 500 ms and 600 ms budgets become the file's LOCK_REGISTRY_TEST_TIMEOUT_MS backstop, and each loop exits on the state it waits for instead of on the clock, so the backstop only fires when the product is actually broken. The contract is unchanged: the tail must still return at its absolute deadline (150 <= elapsed < 350 ms for a 200 ms deadline) despite ~40 unrelated cancel broadcasts, still with BUSY and no lease, with the head still holding the attempt. The broadcast loop now starts immediately after the tail is released, so it overlaps the tail's wait at least as much as it did before. Verification, all on macOS arm64 with the sanitized runner: * The mechanism was reproduced locally by delaying only the tail thread after its start gate, with production untouched. A 120 ms delay (the state exists, outside the observer's budget) and a 250 ms delay (the tail never enqueues) each produced exactly one failure, ASSERT(tail_queued) -- the CI signature. * After the rebuild the same injections are green at 120, 250, 400 and 900 ms: an arbitrary scheduling delay no longer decides the verdict. * lock_registry 30/30 green plain and 30/30 green under four CPU hogs; 16/16 inside the saturated 18-job parallel wave. * private_file_lock, lock_registry, daemon, project_lock and the daemon_* suites: 222 passed, 1 skipped. Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
Extend Linux codepaths in runtime.c to also cover NetBSD, and add NetBSD-specific peer PID retrieval via LOCAL_PEEREID in ipc.c.