mirror of
https://github.com/devmobasa/wayscriber.git
synced 2026-11-01 07:36:42 +01:00
refactor: finish moving kernel calls off raw libc
Detach from the terminal with rustix and std, and drop the remaining unsafe process, poll, and shutdown calls from the tests through small shared helpers. Prune the rustix features nothing ended up using, and record which call sites stay on libc and why in the agent guidance.
This commit is contained in:
1 parent
82a8d5e9c6
commit
66e9259f6f
9 files changed
+80
-104
No files matched your search
@@ -40,6 +40,7 @@
|
||||
- Never reach inside a poisoned lock. Use `expect` when the critical section cannot panic, or a fallback that leaves the data alone. Tests that serialize on process-wide state use `test_env::lock()` or a `test_env::Lease`.
|
||||
- `catch_unwind` is reserved for the reviewed worker-isolation sites that carry an `#[expect]` reason. Tests observe panics through a joined scoped thread.
|
||||
- Getters are `foo()`/`foo_mut()`, a fallible constructor is `try_new`, and type lifetimes have descriptive names. Every `unsafe` block has a `// SAFETY:` comment that says why it is sound.
|
||||
- Make kernel calls through `std` or `rustix`, which need no `unsafe`, and rename on import when a module name clashes with `std` (`use rustix::fs as rustix_fs`). Use raw `libc` calls only where neither fits: signal handlers, the post-`clone` broker stub, `fail_stop`, the zero-signal pidfd check, the inotify header parser, `dup2` onto a fixed descriptor number, and `localtime_r`. `libc` constants stay fine.
|
||||
- Convert numbers losslessly with `From` (`f64::from(width)`) wherever it can be called; `const` initializers keep `as`. Take the middle of two values with `midpoint` (`f64::midpoint(a, b)`), which cannot overflow.
|
||||
- Inline named format arguments: `format!("{name}: {err}")`.
|
||||
- Use `map_or`, `map_or_else`, `is_some_and`, or `is_ok_and` instead of `map(..).unwrap_or(..)`. Use `let ... else` for a `match` that only unwraps or leaves, and `if let ... else` for a `match` with one pattern and a fallback.
|
||||
|
||||
+1
-1
@@ -94,7 +94,7 @@ serde_ignored = "0.1"
|
||||
schemars = { version = "1.2", features = ["derive"], optional = true }
|
||||
libc = "0.2"
|
||||
# Safe wrappers for the kernel interfaces that the std library does not cover.
|
||||
rustix = { version = "1", features = ["event", "fs", "net", "pipe", "process", "stdio", "termios", "thread", "time"] }
|
||||
rustix = { version = "1", features = ["event", "fs", "net", "pipe", "process", "thread", "time"] }
|
||||
# Kernel random bytes for portal handle tokens and daemon protocol identifiers.
|
||||
getrandom = { version = "0.4", features = ["std"] }
|
||||
# Securely created, self-deleting temporary files for the OCR engine handoff.
|
||||
|
||||
+15
-9
@@ -22,10 +22,14 @@ use crate::{
|
||||
};
|
||||
use anyhow::Context;
|
||||
use env::env_flag_enabled;
|
||||
#[cfg(unix)]
|
||||
use rustix::process as rustix_process;
|
||||
use session::run_session_cli_commands;
|
||||
use std::ffi::OsString;
|
||||
use std::fs::{File, OpenOptions};
|
||||
use std::io::ErrorKind;
|
||||
#[cfg(unix)]
|
||||
use std::io::IsTerminal;
|
||||
use std::path::{Path, PathBuf};
|
||||
use usage::{log_overlay_controls, print_usage};
|
||||
|
||||
@@ -170,16 +174,17 @@ fn run_update_check() -> anyhow::Result<()> {
|
||||
#[cfg(unix)]
|
||||
fn detach_from_tty() {
|
||||
// Start a new session to drop the controlling terminal (prevents stuck shells).
|
||||
// SAFETY: setsid takes no arguments; a failure only leaves the session as is.
|
||||
unsafe {
|
||||
let _ = libc::setsid();
|
||||
}
|
||||
// A failure only leaves the session as is.
|
||||
let _ = rustix_process::setsid();
|
||||
// Point stdio that still refers to a TTY at /dev/null. Closing it instead
|
||||
// frees the descriptor number for the next open, and the logger's stderr
|
||||
// writes would then land in whatever file that open returned.
|
||||
for fd in [libc::STDIN_FILENO, libc::STDOUT_FILENO, libc::STDERR_FILENO] {
|
||||
// SAFETY: isatty only inspects the descriptor number it is given.
|
||||
let is_tty = unsafe { libc::isatty(fd) } == 1;
|
||||
let stdio = [
|
||||
(libc::STDIN_FILENO, io::stdin().is_terminal()),
|
||||
(libc::STDOUT_FILENO, io::stdout().is_terminal()),
|
||||
(libc::STDERR_FILENO, io::stderr().is_terminal()),
|
||||
];
|
||||
for (fd, is_tty) in stdio {
|
||||
if is_tty && let Err(error) = redirect_to_dev_null(fd) {
|
||||
log::warn!("Failed to detach descriptor {fd} from the terminal: {error}");
|
||||
}
|
||||
@@ -414,6 +419,8 @@ mod tests {
|
||||
use std::io::Write;
|
||||
use std::os::fd::AsRawFd;
|
||||
|
||||
use rustix::io as rustix_io;
|
||||
|
||||
let temp = test_temp::tempdir().unwrap();
|
||||
let path = temp.path().join("was-a-terminal");
|
||||
let mut file = File::create(&path).unwrap();
|
||||
@@ -422,8 +429,7 @@ mod tests {
|
||||
redirect_to_dev_null(file.as_raw_fd()).unwrap();
|
||||
file.write_all(b"after").unwrap();
|
||||
|
||||
// SAFETY: F_GETFD only inspects the descriptor the file still owns.
|
||||
assert!(unsafe { libc::fcntl(file.as_raw_fd(), libc::F_GETFD) } >= 0);
|
||||
rustix_io::fcntl_getfd(&file).unwrap();
|
||||
assert_eq!(fs::read(&path).unwrap(), b"before ");
|
||||
}
|
||||
|
||||
|
||||
@@ -1,4 +1,3 @@
|
||||
use std::os::fd::AsRawFd;
|
||||
use std::path::PathBuf;
|
||||
use std::sync::atomic::{AtomicUsize, Ordering};
|
||||
use std::sync::{Arc, Mutex, mpsc};
|
||||
@@ -163,23 +162,19 @@ async fn completion_wakes_an_already_blocked_runtime_poll_after_publication() {
|
||||
.unwrap();
|
||||
started.notified().await;
|
||||
|
||||
let wake_fd = runtime_wake.poll_fd().as_raw_fd();
|
||||
let (polling_tx, polling_rx) = mpsc::sync_channel(0);
|
||||
let poller = thread::spawn(move || {
|
||||
let mut pollfd = libc::pollfd {
|
||||
fd: wake_fd,
|
||||
events: libc::POLLIN,
|
||||
revents: 0,
|
||||
};
|
||||
polling_tx.send(()).unwrap();
|
||||
// SAFETY: the runtime wake source outlives this bounded poll and owns
|
||||
// the descriptor for the entire test.
|
||||
assert_eq!(unsafe { libc::poll(&raw mut pollfd, 1, 1_000) }, 1);
|
||||
assert_ne!(pollfd.revents & libc::POLLIN, 0);
|
||||
assert!(
|
||||
runtime_wake
|
||||
.is_readable_within(Duration::from_secs(1))
|
||||
.unwrap()
|
||||
);
|
||||
runtime_wake
|
||||
});
|
||||
polling_rx.recv().unwrap();
|
||||
release.notify_one();
|
||||
poller.join().unwrap();
|
||||
let runtime_wake = poller.join().unwrap();
|
||||
runtime_wake.drain().unwrap();
|
||||
|
||||
assert!(matches!(
|
||||
|
||||
@@ -1,13 +1,22 @@
|
||||
use std::os::fd::AsRawFd;
|
||||
use std::os::fd::{AsFd, BorrowedFd};
|
||||
use std::process::Command;
|
||||
use std::time::{Duration, Instant};
|
||||
use std::{env, process};
|
||||
|
||||
use rustix::event::{self, PollFd, PollFlags, Timespec};
|
||||
|
||||
use crate::tray_action::TrayAction;
|
||||
use crate::{env_vars, test_env, test_temp};
|
||||
|
||||
use super::*;
|
||||
|
||||
fn is_readable(fd: BorrowedFd<'_>, timeout: Duration) -> bool {
|
||||
let mut pollfds = [PollFd::from_borrowed_fd(fd, PollFlags::IN)];
|
||||
let timeout = Timespec::try_from(timeout).unwrap();
|
||||
event::poll(&mut pollfds, Some(&timeout)).unwrap() == 1
|
||||
&& pollfds[0].revents().contains(PollFlags::IN)
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn production_mode_is_closed_to_published_v2_after_cutover() {
|
||||
assert_eq!(
|
||||
@@ -71,15 +80,7 @@ fn boottime_deadline_source_arms_absolutely_and_drains_once() {
|
||||
.unwrap();
|
||||
source.arm(deadline).unwrap();
|
||||
|
||||
let mut poll_fd = libc::pollfd {
|
||||
fd: source.poll_fd().as_raw_fd(),
|
||||
events: libc::POLLIN,
|
||||
revents: 0,
|
||||
};
|
||||
// SAFETY: poll_fd remains valid for this bounded poll.
|
||||
let ready = unsafe { libc::poll(&raw mut poll_fd, 1, 500) };
|
||||
assert_eq!(ready, 1);
|
||||
assert_ne!(poll_fd.revents & libc::POLLIN, 0);
|
||||
assert!(is_readable(source.poll_fd(), Duration::from_millis(500)));
|
||||
assert!(source.drain().unwrap());
|
||||
assert!(!source.drain().unwrap());
|
||||
source.disarm().unwrap();
|
||||
@@ -128,14 +129,7 @@ fn pidfd_wait_times_out_while_alive_and_returns_once_exited() {
|
||||
#[test]
|
||||
fn self_pidfd_is_not_readable_while_process_is_alive() {
|
||||
let pidfd = linux::open_self_pidfd().unwrap();
|
||||
let mut poll_fd = libc::pollfd {
|
||||
fd: pidfd.as_raw_fd(),
|
||||
events: libc::POLLIN,
|
||||
revents: 0,
|
||||
};
|
||||
// SAFETY: poll_fd remains valid for the zero-timeout observation.
|
||||
let ready = unsafe { libc::poll(&raw mut poll_fd, 1, 0) };
|
||||
assert_eq!(ready, 0);
|
||||
assert!(!is_readable(pidfd.as_fd(), Duration::ZERO));
|
||||
}
|
||||
|
||||
#[test]
|
||||
@@ -189,13 +183,7 @@ fn command_queue_watch_reports_atomic_publication_and_rejects_replacement() {
|
||||
)
|
||||
.unwrap();
|
||||
|
||||
let mut poll_fd = libc::pollfd {
|
||||
fd: watcher.poll_fd().as_raw_fd(),
|
||||
events: libc::POLLIN,
|
||||
revents: 0,
|
||||
};
|
||||
// SAFETY: watcher owns the descriptor throughout the bounded poll.
|
||||
assert_eq!(unsafe { libc::poll(&raw mut poll_fd, 1, 500) }, 1);
|
||||
assert!(is_readable(watcher.poll_fd(), Duration::from_millis(500)));
|
||||
assert!(watcher.drain().unwrap().scan_pending);
|
||||
|
||||
fs::rename(&queue, temp.path().join("old-queue")).unwrap();
|
||||
|
||||
@@ -633,7 +633,6 @@ mod tests {
|
||||
|
||||
drop(ownership);
|
||||
|
||||
// SAFETY: signal zero only probes the test-owned provider after teardown.
|
||||
assert_ne!(unsafe { libc::kill(pid, 0) }, 0);
|
||||
assert!(rustix_process::test_kill_process(Pid::from_raw(pid).unwrap()).is_err());
|
||||
}
|
||||
}
|
||||
+33
-50
@@ -1,5 +1,5 @@
|
||||
use std::ffi::OsStr;
|
||||
use std::os::fd::{AsFd, AsRawFd};
|
||||
use std::os::fd::AsFd;
|
||||
use std::os::unix::process::CommandExt;
|
||||
use std::path::Path;
|
||||
use std::process::Command;
|
||||
@@ -7,10 +7,23 @@ use std::sync::mpsc;
|
||||
use std::time::{Duration, Instant};
|
||||
use std::{env, fs, iter, thread};
|
||||
|
||||
use rustix::net::{self, Shutdown};
|
||||
use rustix::process::{self as rustix_process, Pid, Signal};
|
||||
|
||||
use crate::daemon::protocol_v2;
|
||||
use crate::test_fake_helper::Role;
|
||||
use crate::{env_vars, process_broker, test_env, test_fake_helper, test_temp};
|
||||
|
||||
fn process_exists(pid: libc::pid_t) -> bool {
|
||||
Pid::from_raw(pid).is_some_and(|pid| rustix_process::test_kill_process(pid).is_ok())
|
||||
}
|
||||
|
||||
fn kill_for_cleanup(pid: libc::pid_t) {
|
||||
if let Some(pid) = Pid::from_raw(pid) {
|
||||
let _ = rustix_process::kill_process(pid, Signal::KILL);
|
||||
}
|
||||
}
|
||||
|
||||
use super::*;
|
||||
|
||||
fn wire_arguments(arguments: &[&str]) -> Vec<super::wire::OsWire> {
|
||||
@@ -28,10 +41,7 @@ fn release_test_provider(release_path: &Path, proof_path: &Path, provider_pid: i
|
||||
return;
|
||||
}
|
||||
if Instant::now() >= deadline {
|
||||
// SAFETY: cleanup keeps a failing regression test from leaking its provider.
|
||||
unsafe {
|
||||
libc::kill(provider_pid, libc::SIGKILL);
|
||||
}
|
||||
kill_for_cleanup(provider_pid);
|
||||
panic!("successful provider could not act after normal broker shutdown");
|
||||
}
|
||||
thread::sleep(Duration::from_millis(5));
|
||||
@@ -540,8 +550,7 @@ fn process_group_guard_cleans_up_before_ownership_transfer() {
|
||||
|
||||
drop(child);
|
||||
|
||||
// SAFETY: signal zero only probes the test-owned helper PID after guard cleanup.
|
||||
assert_ne!(unsafe { libc::kill(pid, 0) }, 0);
|
||||
assert!(!process_exists(pid));
|
||||
}
|
||||
|
||||
#[test]
|
||||
@@ -762,8 +771,7 @@ fn broker_guard_preempts_an_active_operation_and_kills_its_group() {
|
||||
"broker shutdown waited for the active operation"
|
||||
);
|
||||
assert!(run.join().unwrap().is_err());
|
||||
// SAFETY: signal zero only probes the test-owned helper PID.
|
||||
assert_ne!(unsafe { libc::kill(helper_pid, 0) }, 0);
|
||||
assert!(!process_exists(helper_pid));
|
||||
}
|
||||
|
||||
#[test]
|
||||
@@ -891,10 +899,7 @@ fn a_descendant_outside_the_helper_group_cannot_hold_the_broker_on_its_pipes() {
|
||||
.ok()
|
||||
.and_then(|pid| pid.trim().parse::<libc::pid_t>().ok())
|
||||
{
|
||||
// SAFETY: cleanup of the descendant this test started in its own session.
|
||||
unsafe {
|
||||
libc::kill(pid, libc::SIGKILL);
|
||||
}
|
||||
kill_for_cleanup(pid);
|
||||
}
|
||||
|
||||
let error = result.expect_err("output still held open must not read as complete");
|
||||
@@ -950,8 +955,7 @@ fn normal_broker_shutdown_releases_successful_provider_descendant() {
|
||||
.trim()
|
||||
.parse::<i32>()
|
||||
.unwrap();
|
||||
// SAFETY: signal zero only checks the test-owned provider.
|
||||
assert_eq!(unsafe { libc::kill(provider_pid, 0) }, 0);
|
||||
assert!(process_exists(provider_pid));
|
||||
|
||||
drop(guard);
|
||||
|
||||
@@ -984,28 +988,19 @@ fn shutdown_channel_peer_loss_kills_retained_provider() {
|
||||
.trim()
|
||||
.parse::<i32>()
|
||||
.unwrap();
|
||||
// SAFETY: signal zero only checks the test-owned provider.
|
||||
assert_eq!(unsafe { libc::kill(provider_pid, 0) }, 0);
|
||||
assert!(process_exists(provider_pid));
|
||||
|
||||
// Simulate abrupt parent loss without writing the graceful-shutdown packet.
|
||||
assert_eq!(
|
||||
// SAFETY: the descriptor belongs to this test's live broker guard.
|
||||
unsafe { libc::shutdown(guard.broker().inner.shutdown.as_raw_fd(), libc::SHUT_RDWR) },
|
||||
0
|
||||
);
|
||||
net::shutdown(&guard.broker().inner.shutdown, Shutdown::Both).unwrap();
|
||||
drop(guard);
|
||||
|
||||
let deadline = Instant::now() + Duration::from_secs(1);
|
||||
loop {
|
||||
// SAFETY: signal zero only probes the recorded test provider PID.
|
||||
if unsafe { libc::kill(provider_pid, 0) } != 0 {
|
||||
if !process_exists(provider_pid) {
|
||||
break;
|
||||
}
|
||||
if Instant::now() >= deadline {
|
||||
// SAFETY: cleanup keeps a failing regression test from leaking its provider.
|
||||
unsafe {
|
||||
libc::kill(provider_pid, libc::SIGKILL);
|
||||
}
|
||||
kill_for_cleanup(provider_pid);
|
||||
panic!("provider survived abnormal broker channel loss");
|
||||
}
|
||||
thread::sleep(Duration::from_millis(5));
|
||||
@@ -1069,22 +1064,17 @@ fn retained_publication_replacement_disposes_the_previous_provider() {
|
||||
.unwrap();
|
||||
let deadline = Instant::now() + Duration::from_secs(1);
|
||||
loop {
|
||||
// SAFETY: signal zero only probes the recorded test provider PID.
|
||||
if unsafe { libc::kill(first_pid, 0) } != 0 {
|
||||
if !process_exists(first_pid) {
|
||||
break;
|
||||
}
|
||||
if Instant::now() >= deadline {
|
||||
// SAFETY: cleanup keeps a failing regression test from leaking its helpers.
|
||||
unsafe {
|
||||
libc::kill(first_pid, libc::SIGKILL);
|
||||
libc::kill(second_pid, libc::SIGKILL);
|
||||
}
|
||||
kill_for_cleanup(first_pid);
|
||||
kill_for_cleanup(second_pid);
|
||||
panic!("replaced publication provider remained alive");
|
||||
}
|
||||
thread::sleep(Duration::from_millis(5));
|
||||
}
|
||||
// SAFETY: signal zero only checks the current test-owned provider.
|
||||
assert_eq!(unsafe { libc::kill(second_pid, 0) }, 0);
|
||||
assert!(process_exists(second_pid));
|
||||
|
||||
drop(guard);
|
||||
release_test_provider(&second_release_path, &second_proof_path, second_pid);
|
||||
@@ -1146,20 +1136,15 @@ fn failed_publication_replacement_preserves_the_current_provider() {
|
||||
.trim()
|
||||
.parse::<i32>()
|
||||
.unwrap();
|
||||
// SAFETY: signal zero only checks the current test-owned provider.
|
||||
assert_eq!(unsafe { libc::kill(current_pid, 0) }, 0);
|
||||
assert!(process_exists(current_pid));
|
||||
let deadline = Instant::now() + Duration::from_secs(1);
|
||||
loop {
|
||||
// SAFETY: signal zero only probes the failed test provider PID.
|
||||
if unsafe { libc::kill(failed_pid, 0) } != 0 {
|
||||
if !process_exists(failed_pid) {
|
||||
break;
|
||||
}
|
||||
if Instant::now() >= deadline {
|
||||
// SAFETY: cleanup keeps a failing regression test from leaking its helpers.
|
||||
unsafe {
|
||||
libc::kill(current_pid, libc::SIGKILL);
|
||||
libc::kill(failed_pid, libc::SIGKILL);
|
||||
}
|
||||
kill_for_cleanup(current_pid);
|
||||
kill_for_cleanup(failed_pid);
|
||||
panic!("failed replacement provider survived cleanup");
|
||||
}
|
||||
thread::sleep(Duration::from_millis(5));
|
||||
@@ -1205,8 +1190,7 @@ fn retained_publication_kills_failed_or_input_stalled_provider_groups() {
|
||||
}
|
||||
let deadline = Instant::now() + Duration::from_secs(1);
|
||||
loop {
|
||||
// SAFETY: signal zero only probes the recorded test child PID.
|
||||
if unsafe { libc::kill(provider_pid, 0) } != 0 {
|
||||
if !process_exists(provider_pid) {
|
||||
break;
|
||||
}
|
||||
assert!(
|
||||
@@ -1307,8 +1291,7 @@ fn broker_shutdown_preempts_retained_publication_stdin_writer() {
|
||||
release_test_provider(¤t_release_path, ¤t_proof_path, current_pid);
|
||||
let deadline = Instant::now() + Duration::from_secs(1);
|
||||
loop {
|
||||
// SAFETY: signal zero only probes the recorded test child PID.
|
||||
if unsafe { libc::kill(provider_pid, 0) } != 0 {
|
||||
if !process_exists(provider_pid) {
|
||||
break;
|
||||
}
|
||||
assert!(
|
||||
|
||||
@@ -15,6 +15,8 @@ use std::os::unix::fs as unix_fs;
|
||||
use std::path::{Path, PathBuf};
|
||||
use std::{env, fs, io, process};
|
||||
|
||||
use rustix::process as rustix_process;
|
||||
|
||||
use anyhow::{Context, Result};
|
||||
|
||||
use crate::durable_io;
|
||||
@@ -124,8 +126,7 @@ fn play(arguments: &[OsString]) -> Result<()> {
|
||||
let report: Vec<u8> = match role {
|
||||
Role::Exit => return Ok(()),
|
||||
Role::ReportProcessGroup => {
|
||||
// SAFETY: getpgrp has no preconditions and cannot fail.
|
||||
let leader = unsafe { libc::getpgrp() } == process::id() as libc::pid_t;
|
||||
let leader = rustix_process::getpgrp().as_raw_pid() == process::id() as libc::pid_t;
|
||||
if leader {
|
||||
"process-group-leader"
|
||||
} else {
|
||||
|
||||
@@ -7,6 +7,9 @@ use std::sync::{Arc, Mutex};
|
||||
use std::thread::{self, JoinHandle};
|
||||
use std::{fmt, io};
|
||||
|
||||
#[cfg(test)]
|
||||
use rustix::thread as rustix_thread;
|
||||
|
||||
use super::{
|
||||
SIGNAL_WRITE_FD, begin_listener_teardown, clear_registered_signals, create_pipe,
|
||||
dispatch_pending_signals, finish_listener_teardown, install_handler, publish_listener_active,
|
||||
@@ -259,9 +262,9 @@ impl Drop for SignalListener {
|
||||
fn listener_thread(read_fd: RawFd, shared: &ListenerShared) {
|
||||
#[cfg(test)]
|
||||
{
|
||||
// SAFETY: gettid has no preconditions and is used only by tests to
|
||||
// observe that this exact thread reached its blocking read.
|
||||
let tid = unsafe { libc::syscall(libc::SYS_gettid) as libc::pid_t };
|
||||
// Used only by tests to observe that this exact thread reached its
|
||||
// blocking read.
|
||||
let tid = rustix_thread::gettid().as_raw_pid();
|
||||
shared.thread_tid.store(tid, Ordering::Release);
|
||||
}
|
||||
|
||||
|
||||
Reference in new issue
Block a user