From ef3923c992e343ce9bda3307b2167059c244223f Mon Sep 17 00:00:00 2001 From: Johannes Altmanninger Date: Sun, 16 Nov 2025 13:50:55 +0100 Subject: [PATCH] Clean up cfg() definitions Sort. Use idiomatic names. Use cfg_if. --- build.rs | 39 +++++++++--------- src/exec.rs | 10 ++--- src/fd_monitor.rs | 94 +++++++++++++++++++++++++------------------- src/fds.rs | 44 +++++++++++---------- src/fork_exec/mod.rs | 2 +- src/locale.rs | 4 +- src/proc.rs | 20 ++++++---- 7 files changed, 115 insertions(+), 98 deletions(-) diff --git a/build.rs b/build.rs index c0c0365c4..a0339a552 100644 --- a/build.rs +++ b/build.rs @@ -73,37 +73,34 @@ fn main() { /// `Cargo.toml`) behind a feature we just enabled. /// /// [0]: https://github.com/rust-lang/cargo/issues/5499 -#[rustfmt::skip] fn detect_cfgs(target: &mut Target) { for (name, handler) in [ - // Ignore the first entry, it just sets up the type inference. Model new entries after the - // second line. + // Ignore the first entry, it just sets up the type inference. ("", &(|_: &Target| false) as &dyn Fn(&Target) -> bool), ("apple", &detect_apple), ("bsd", &detect_bsd), - ("using_cmake", &|_| option_env!("FISH_CMAKE_BINARY_DIR").is_some()), - ("use_prebuilt_docs", &|_| env_var("FISH_USE_PREBUILT_DOCS").is_some_and(|v| v == "TRUE") ), ("cygwin", &detect_cygwin), - ("small_main_stack", &has_small_stack), - // See if libc supports the thread-safe localeconv_l(3) alternative to localeconv(3). - ("localeconv_l", &|target| { - target.has_symbol("localeconv_l") - }), - ("FISH_USE_POSIX_SPAWN", &|target| { - target.has_header("spawn.h") - }), - ("HAVE_PIPE2", &|target| { - target.has_symbol("pipe2") - }), - ("HAVE_EVENTFD", &|target| { + ("have_eventfd", &|target| { // FIXME: NetBSD 10 has eventfd, but the libc crate does not expose it. if cfg!(target_os = "netbsd") { - false - } else { - target.has_header("sys/eventfd.h") + false + } else { + target.has_header("sys/eventfd.h") } }), - ("HAVE_WAITSTATUS_SIGNAL_RET", &|target| { + ("have_localeconv_l", &|target| { + target.has_symbol("localeconv_l") + }), + ("have_pipe2", &|target| target.has_symbol("pipe2")), + ("have_posix_spawn", &|target| target.has_header("spawn.h")), + ("small_main_stack", &has_small_stack), + ("use_prebuilt_docs", &|_| { + env_var("FISH_USE_PREBUILT_DOCS").is_some_and(|v| v == "TRUE") + }), + ("using_cmake", &|_| { + option_env!("FISH_CMAKE_BINARY_DIR").is_some() + }), + ("waitstatus_signal_ret", &|target| { target.r#if("WEXITSTATUS(0x007f) == 0x7f", &["sys/wait.h"]) }), ] { diff --git a/src/exec.rs b/src/exec.rs index b085c04ed..1c44e5ff1 100644 --- a/src/exec.rs +++ b/src/exec.rs @@ -12,7 +12,7 @@ write_loop, }; use crate::env::{EnvMode, EnvStack, Environment, READ_BYTE_LIMIT, Statuses}; -#[cfg(FISH_USE_POSIX_SPAWN)] +#[cfg(have_posix_spawn)] use crate::env_dispatch::use_posix_spawn; use crate::fds::make_fd_blocking; use crate::fds::{PIPE_ERROR, make_autoclose_pipes, open_cloexec}; @@ -23,7 +23,7 @@ child_setup_process, execute_fork, execute_setpgid, report_setpgid_error, safe_report_exec_error, }; -#[cfg(FISH_USE_POSIX_SPAWN)] +#[cfg(have_posix_spawn)] use crate::fork_exec::spawn::PosixSpawner; use crate::function::{self, FunctionProperties}; use crate::io::{ @@ -33,7 +33,7 @@ use crate::nix::{getpid, isatty}; use crate::null_terminated_array::OwningNullTerminatedArray; use crate::parser::{Block, BlockId, BlockType, EvalRes, Parser}; -#[cfg(FISH_USE_POSIX_SPAWN)] +#[cfg(have_posix_spawn)] use crate::proc::Pid; use crate::proc::{ InternalProc, Job, JobGroupRef, ProcStatus, Process, ProcessType, hup_jobs, @@ -456,7 +456,7 @@ fn launch_process_nofork(vars: &EnvStack, p: &Process) -> ! { // To avoid the race between the caller calling tcsetpgrp() and the client checking the // foreground process group, we don't use posix_spawn if we're going to foreground the process. (If // we use fork(), we can call tcsetpgrp after the fork, before the exec, and avoid the race). -#[cfg(FISH_USE_POSIX_SPAWN)] +#[cfg(have_posix_spawn)] fn can_use_posix_spawn_for_job(job: &Job, dup2s: &Dup2List) -> bool { // Is it globally disabled? if !use_posix_spawn() { @@ -884,7 +884,7 @@ fn exec_external_command( let actual_cmd = wcs2zstring(&p.actual_cmd); - #[cfg(FISH_USE_POSIX_SPAWN)] + #[cfg(have_posix_spawn)] // Prefer to use posix_spawn, since it's faster on some systems like OS X. if can_use_posix_spawn_for_job(j, &dup2s) { let file = &parser.libdata().current_filename; diff --git a/src/fd_monitor.rs b/src/fd_monitor.rs index 7a25a4c33..d9f180809 100644 --- a/src/fd_monitor.rs +++ b/src/fd_monitor.rs @@ -1,3 +1,4 @@ +use cfg_if::cfg_if; #[cfg(not(target_has_atomic = "64"))] use portable_atomic::AtomicU64; use std::collections::HashMap; @@ -17,10 +18,13 @@ use errno::errno; use libc::{EAGAIN, EINTR, EWOULDBLOCK, c_void}; -#[cfg(not(HAVE_EVENTFD))] -use crate::fds::{make_autoclose_pipes, make_fd_nonblocking}; -#[cfg(HAVE_EVENTFD)] -use libc::{EFD_CLOEXEC, EFD_NONBLOCK}; +cfg_if!( + if #[cfg(have_eventfd)] { + use libc::{EFD_CLOEXEC, EFD_NONBLOCK}; + } else { + use crate::fds::{make_autoclose_pipes, make_fd_nonblocking}; + } +); /// An event signaller implemented using a file descriptor, so it can plug into /// [`select()`](libc::select). @@ -33,7 +37,7 @@ pub struct FdEventSignaller { // Always the read end of the fd; maybe the write end as well. fd: OwnedFd, - #[cfg(not(HAVE_EVENTFD))] + #[cfg(not(have_eventfd))] write: OwnedFd, } @@ -41,31 +45,30 @@ impl FdEventSignaller { /// The default constructor will abort on failure (fd exhaustion). /// This should only be used during startup. pub fn new() -> Self { - #[cfg(HAVE_EVENTFD)] - { - // Note we do not want to use EFD_SEMAPHORE because we are binary (not counting) semaphore. - let fd = unsafe { libc::eventfd(0, EFD_CLOEXEC | EFD_NONBLOCK) }; - if fd < 0 { - perror("eventfd"); - exit_without_destructors(1); + cfg_if!( + if #[cfg(have_eventfd)] { + // Note we do not want to use EFD_SEMAPHORE because we are binary (not counting) semaphore. + let fd = unsafe { libc::eventfd(0, EFD_CLOEXEC | EFD_NONBLOCK) }; + if fd < 0 { + perror("eventfd"); + exit_without_destructors(1); + } + return Self { + fd: unsafe { OwnedFd::from_raw_fd(fd) }, + }; + } else { + // Implementation using pipes. + let Ok(pipes) = make_autoclose_pipes() else { + exit_without_destructors(1); + }; + make_fd_nonblocking(pipes.read.as_raw_fd()).unwrap(); + make_fd_nonblocking(pipes.write.as_raw_fd()).unwrap(); + return Self { + fd: pipes.read, + write: pipes.write, + }; } - Self { - fd: unsafe { OwnedFd::from_raw_fd(fd) }, - } - } - #[cfg(not(HAVE_EVENTFD))] - { - // Implementation using pipes. - let Ok(pipes) = make_autoclose_pipes() else { - exit_without_destructors(1); - }; - make_fd_nonblocking(pipes.read.as_raw_fd()).unwrap(); - make_fd_nonblocking(pipes.write.as_raw_fd()).unwrap(); - Self { - fd: pipes.read, - write: pipes.write, - } - } + ); } /// Return the fd to read from, for notification. @@ -80,10 +83,13 @@ pub fn try_consume(&self) -> bool { // If we are using eventfd, we want to read a single uint64. // If we are using pipes, read a lot; note this may leave data on the pipe if post has been // called many more times. In no case do we care about the data which is read. - #[cfg(HAVE_EVENTFD)] - let mut buff = [0_u64; 1]; - #[cfg(not(HAVE_EVENTFD))] - let mut buff = [0_u8; 1024]; + cfg_if!( + if #[cfg(have_eventfd)] { + let mut buff = [0_u64; 1]; + } else { + let mut buff = [0_u8; 1024]; + } + ); let mut ret; loop { ret = unsafe { @@ -107,10 +113,13 @@ pub fn try_consume(&self) -> bool { /// This retries on EINTR. pub fn post(&self) { // eventfd writes uint64; pipes write 1 byte. - #[cfg(HAVE_EVENTFD)] - let c = 1_u64; - #[cfg(not(HAVE_EVENTFD))] - let c = 1_u8; + cfg_if!( + if #[cfg(have_eventfd)] { + let c = 1_u64; + } else { + let c = 1_u8; + } + ); let mut ret; loop { let bytes = c.to_ne_bytes(); @@ -146,10 +155,13 @@ pub fn poll(&self, wait: bool /* = false */) -> bool { /// Return the fd to write to. fn write_fd(&self) -> RawFd { - #[cfg(HAVE_EVENTFD)] - return self.fd.as_raw_fd(); - #[cfg(not(HAVE_EVENTFD))] - return self.write.as_raw_fd(); + cfg_if!( + if #[cfg(have_eventfd)] { + return self.fd.as_raw_fd(); + } else { + return self.write.as_raw_fd(); + } + ); } } diff --git a/src/fds.rs b/src/fds.rs index 228e91a97..b774c4ebc 100644 --- a/src/fds.rs +++ b/src/fds.rs @@ -3,6 +3,7 @@ use crate::signal::signal_check_cancel; use crate::wchar::prelude::*; use crate::wutil::perror; +use cfg_if::cfg_if; use libc::{EINTR, F_GETFD, F_GETFL, F_SETFD, F_SETFL, FD_CLOEXEC, O_NONBLOCK, c_int}; use nix::fcntl::FcntlArg; use nix::{fcntl::OFlag, unistd}; @@ -136,27 +137,30 @@ pub struct AutoClosePipes { pub fn make_autoclose_pipes() -> nix::Result { #[allow(unused_mut, unused_assignments)] let mut already_cloexec = false; - #[cfg(HAVE_PIPE2)] - let pipes = match nix::unistd::pipe2(OFlag::O_CLOEXEC) { - Ok(pipes) => { - already_cloexec = true; - pipes + cfg_if!( + if #[cfg(have_pipe2)] { + let pipes = match nix::unistd::pipe2(OFlag::O_CLOEXEC) { + Ok(pipes) => { + already_cloexec = true; + pipes + } + Err(err) => { + FLOG!(warning, PIPE_ERROR.localize()); + perror("pipe2"); + return Err(err); + } + }; + } else { + let pipes = match nix::unistd::pipe() { + Ok(pipes) => pipes, + Err(err) => { + FLOG!(warning, PIPE_ERROR.localize()); + perror("pipe"); + return Err(err); + } + }; } - Err(err) => { - FLOG!(warning, PIPE_ERROR.localize()); - perror("pipe2"); - return Err(err); - } - }; - #[cfg(not(HAVE_PIPE2))] - let pipes = match nix::unistd::pipe() { - Ok(pipes) => pipes, - Err(err) => { - FLOG!(warning, PIPE_ERROR.localize()); - perror("pipe"); - return Err(err); - } - }; + ); let readp = pipes.0; let writep = pipes.1; diff --git a/src/fork_exec/mod.rs b/src/fork_exec/mod.rs index 4d3f235dc..4df8daf6f 100644 --- a/src/fork_exec/mod.rs +++ b/src/fork_exec/mod.rs @@ -4,7 +4,7 @@ pub mod flog_safe; pub mod postfork; -#[cfg(FISH_USE_POSIX_SPAWN)] +#[cfg(have_posix_spawn)] pub mod spawn; use crate::proc::Job; use libc::{SIGINT, SIGQUIT}; diff --git a/src/locale.rs b/src/locale.rs index d50a4bbf5..8d37f2a6a 100644 --- a/src/locale.rs +++ b/src/locale.rs @@ -105,7 +105,7 @@ unsafe fn lconv_to_locale(lconv: &libc::lconv) -> Locale { } /// Read the numeric locale, or None on any failure. -#[cfg(localeconv_l)] +#[cfg(have_localeconv_l)] unsafe fn read_locale() -> Option { unsafe extern "C" { unsafe fn localeconv_l(loc: libc::locale_t) -> *const libc::lconv; @@ -130,7 +130,7 @@ unsafe fn read_locale() -> Option { result } -#[cfg(not(localeconv_l))] +#[cfg(not(have_localeconv_l))] unsafe fn read_locale() -> Option { // Bleh, we have to go through localeconv, which races with setlocale. // TODO: There has to be a better way to do this. diff --git a/src/proc.rs b/src/proc.rs index 0a00e1bdf..2b481ef00 100644 --- a/src/proc.rs +++ b/src/proc.rs @@ -22,6 +22,7 @@ use crate::wchar::prelude::*; use crate::wchar_ext::ToWString; use crate::wutil::{wbasename, wperror}; +use cfg_if::cfg_if; use libc::{ _SC_CLK_TCK, EXIT_SUCCESS, SIG_DFL, SIG_IGN, SIGABRT, SIGBUS, SIGCONT, SIGFPE, SIGHUP, SIGILL, SIGINT, SIGKILL, SIGPIPE, SIGQUIT, SIGSEGV, SIGSYS, SIGTTOU, WCONTINUED, WEXITSTATUS, @@ -125,14 +126,17 @@ pub fn is_empty(&self) -> bool { /// Encode a return value `ret` and signal `sig` into a status value like waitpid() does. const fn w_exitcode(ret: i32, sig: i32) -> i32 { - #[cfg(HAVE_WAITSTATUS_SIGNAL_RET)] - // It's encoded signal and then status - // The return status is in the lower byte. - return (sig << 8) | ret; - #[cfg(not(HAVE_WAITSTATUS_SIGNAL_RET))] - // The status is encoded in the upper byte. - // This should be W_EXITCODE(ret, sig) but that's not available everywhere. - return (ret << 8) | sig; + cfg_if!( + if #[cfg(waitstatus_signal_ret)] { + // It's encoded signal and then status + // The return status is in the lower byte. + return (sig << 8) | ret; + } else { + // The status is encoded in the upper byte. + // This should be W_EXITCODE(ret, sig) but that's not available everywhere. + return (ret << 8) | sig; + } + ); } /// Construct from a status returned from a waitpid call.