nix-rust / nix

Rust friendly bindings to *nix APIs
MIT License
2.57k stars 650 forks source link

Add sigaction() versions that allow specify the signal by its number #2449

Open ChrysoliteAzalea opened 2 weeks ago

ChrysoliteAzalea commented 2 weeks ago

Currently, the nix::sys::signal::sigaction() function uses the Signal enum, which only covers standard signals, but not real-time signals. Using real-time signals with nix requires using the std::mem::transmute() function to assign an impossible value to the Signal enum, which is undefined behavior.

What does this PR do

In this PR, I add two new functions: nix::sys::signal::sigaction_numeric(), that uses signal numbers instead of the enum, and nix::sys::signal::sigaction_noretrieve(), which also doesn't retrieve the old signal handler by supplying the null pointer as the oldact argument (to avoid trouble if the handler has been installed by the C code).

Checklist:

SteveLauC commented 2 weeks ago

Hi, thanks for the work on realtime signals.

[?] A change log has been added if this PR modifies nix's API

Nix needs a CHANGELOG entry for those PRs that modify the API, please refer to CONTRIBUTING.md on how to do this.


Related to:

From my side, the thing that blocks us from supporting realtime signals is that some platforms define the signal number as constants, while other platforms use function. And signal numbers on Linux/glibc is a runtime thing since glibc uses the first 2 or 3 rt signals for internal usage. So we cannot easily add them to the existing enum Signal type.

This PR proposes APIs that use raw c_int types with sigaction(), instead of adding new interfaces as workaround, I think we should also think about how to make rt signals work with existing interfaces, e.g., as mentioned in #495, sigwait() and waitpid() will panic when encountering rt signal, I am thinking about changing enum Signal to something like this:

#[repr(transparent)]
pub struct Signal(libc::c_int);

impl Signal {
    pub const SIGINT: Signal = Signal(libc::SIGINT);

    pub fn realtime(sig_num: libc::c_int) -> Result<Self, nix::errno::Errno> {
        let min = libc::SIGRTMIN();
        let max = libc::SIGRTMAX();

        if sig_num < min || sig_num > max {
            return Err(nix::errno::Errno::EINVAL);
        }

        Ok(Signal(sig_num))
    }
}

By using a wrapper of c_int, rt signals can be accommodated in it, though we will lost the convenient pattern matching after this conversion.

cc @asomers, would like to hear your thoughts on rt signal support.

SteveLauC commented 2 weeks ago
pub enum Signal {
    Standard(nix::signal::Signal),
    Realtime(libc::c_int),
}

Or maybe something like this? Though I am not sure if realtime signal is supported by all the OSes supported by Nix.

ChrysoliteAzalea commented 2 weeks ago

I wonder, should I modify existing functions, or add new one to avoid breaking changes to existing applications?

SteveLauC commented 2 weeks ago

avoid breaking changes to existing applications?

Breaking change is fine, the things I care about most are the usability and ergonomics of the new interfaces.