Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Signal enum #362

Merged
merged 1 commit into from
Jun 27, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
249 changes: 174 additions & 75 deletions src/sys/signal.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,60 @@
// See http://rust-lang.org/COPYRIGHT.

use libc;
use {Errno, Result};
use {Errno, Error, Result};
use std::mem;
use std::ptr;

pub use libc::{
// Currently there is only one definition of c_int in libc, as well as only one
// type for signal constants.
// We would prefer to use the libc::c_int alias in the repr attribute. Unfortunately
// this is not (yet) possible.
#[derive(Clone, Copy, Debug, Eq, PartialEq)]
#[repr(i32)]
pub enum Signal {
SIGHUP = libc::SIGHUP,
SIGINT = libc::SIGINT,
SIGQUIT = libc::SIGQUIT,
SIGILL = libc::SIGILL,
SIGTRAP = libc::SIGTRAP,
SIGABRT = libc::SIGABRT,
SIGBUS = libc::SIGBUS,
SIGFPE = libc::SIGFPE,
SIGKILL = libc::SIGKILL,
SIGUSR1 = libc::SIGUSR1,
SIGSEGV = libc::SIGSEGV,
SIGUSR2 = libc::SIGUSR2,
SIGPIPE = libc::SIGPIPE,
SIGALRM = libc::SIGALRM,
SIGTERM = libc::SIGTERM,
#[cfg(not(target_os = "macos"))]
SIGSTKFLT = libc::SIGSTKFLT,
SIGCHLD = libc::SIGCHLD,
SIGCONT = libc::SIGCONT,
SIGSTOP = libc::SIGSTOP,
SIGTSTP = libc::SIGTSTP,
SIGTTIN = libc::SIGTTIN,
SIGTTOU = libc::SIGTTOU,
SIGURG = libc::SIGURG,
SIGXCPU = libc::SIGXCPU,
SIGXFSZ = libc::SIGXFSZ,
SIGVTALRM = libc::SIGVTALRM,
SIGPROF = libc::SIGPROF,
SIGWINCH = libc::SIGWINCH,
SIGIO = libc::SIGIO,
#[cfg(not(target_os = "macos"))]
SIGPWR = libc::SIGPWR,
SIGSYS = libc::SIGSYS,
#[cfg(target_os = "macos")]
SIGEMT = libc::SIGEMT,
#[cfg(target_os = "macos")]
SIGINFO = libc::SIGINFO,
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The following type and function are only needed internally. They are used in the implementation of the iterator and in wait.rs. Because of the latter I hade to make them public (for now).

I propose putting the whole code into a private module (src/internal/sys/signal.rs) and then reexport what is supposed to be public here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For hiding, your proposal is the right thing to do. Though there's no need to put everything in that module I think: circular dependencies among modules in a crate are ok. You could have an internal module depend on this module, and define signal_from. Then this module and wait could both depend on that module.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll implement that.

pub use self::Signal::*;

#[cfg(not(target_os = "macos"))]
const SIGNALS: [Signal; 31] = [
SIGHUP,
SIGINT,
SIGQUIT,
Expand All @@ -22,6 +71,7 @@ pub use libc::{
SIGPIPE,
SIGALRM,
SIGTERM,
SIGSTKFLT,
SIGCHLD,
SIGCONT,
SIGSTOP,
Expand All @@ -35,26 +85,83 @@ pub use libc::{
SIGPROF,
SIGWINCH,
SIGIO,
SIGSYS,
};

SIGPWR,
SIGSYS];
#[cfg(target_os = "macos")]
pub use libc::{
const SIGNALS: [Signal; 31] = [
SIGHUP,
SIGINT,
SIGQUIT,
SIGILL,
SIGTRAP,
SIGABRT,
SIGBUS,
SIGFPE,
SIGKILL,
SIGUSR1,
SIGSEGV,
SIGUSR2,
SIGPIPE,
SIGALRM,
SIGTERM,
SIGCHLD,
SIGCONT,
SIGSTOP,
SIGTSTP,
SIGTTIN,
SIGTTOU,
SIGURG,
SIGXCPU,
SIGXFSZ,
SIGVTALRM,
SIGPROF,
SIGWINCH,
SIGIO,
SIGSYS,
SIGEMT,
SIGINFO,
};

#[cfg(not(target_os = "macos"))]
pub use libc::{
SIGPWR,
SIGSTKFLT,
SIGIOT, // Alias for SIGABRT
SIGPOLL, // Alias for SIGIO
SIGUNUSED, // Alias for 31
};
SIGINFO];

pub const NSIG: libc::c_int = 32;

pub struct SignalIterator {
next: usize,
}

impl Iterator for SignalIterator {
type Item = Signal;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I considered a nother method of iterating over enumerations. It uses an array of all elements and uses the slice module to produce and iterator. I think the following is a little clearer than an array which contains all the elements of the enumeration again. In particular, when the elements of the enumeration differ between platforms.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this approach is better.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have two problems with this: Its slower. It cannot be used with enumerations whose variant representation dont form a single interval in the natural numbers. Therefore, I actually prefer the other approach, even though it is more verbose and probably makes the executable slightly bigger.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After my last change it is no longer slower. However, we still cannot use its pattern for non consecutive enums.

Copy link
Member

@kamalmarhubi kamalmarhubi May 5, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

perhaps it's better to always have a static array of all the enum variants after all?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if the concern is returning a slice iterator, we could newtype it to make it opaque. that would simplify implementation and work for non-contiguous enums.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It probably is. I'll change it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why would returning a slice iterator be a problem?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's part of the public interface, so a change to the implementation would be breaking.

(my kingdom for an impl Trait!)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we should export our own iterator.


fn next(&mut self) -> Option<Signal> {
if self.next < SIGNALS.len() {
let next_signal = SIGNALS[self.next];
self.next += 1;
Some(next_signal)
} else {
None
}
}
}

impl Signal {
pub fn iterator() -> SignalIterator {
SignalIterator{next: 0}
}

// We do not implement the From trait, because it is supposed to be infallible.
// With Rust RFC 1542 comes the appropriate trait TryFrom. Once it is
// implemented, we'll replace this function.
#[inline]
pub fn from_c_int(signum: libc::c_int) -> Result<Signal> {
match 0 < signum && signum < NSIG {
true => Ok(unsafe { mem::transmute(signum) }),
false => Err(Error::invalid_argument()),
}
}
}

pub const SIGIOT : Signal = SIGABRT;
pub const SIGPOLL : Signal = SIGIO;
pub const SIGUNUSED : Signal = SIGSYS;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are these aliases cross-platform?


bitflags!{
flags SaFlags: libc::c_int {
const SA_NOCLDSTOP = libc::SA_NOCLDSTOP,
Expand All @@ -80,7 +187,6 @@ pub struct SigSet {
sigset: libc::sigset_t
}

pub type SigNum = libc::c_int;

impl SigSet {
pub fn all() -> SigSet {
Expand All @@ -97,40 +203,33 @@ impl SigSet {
SigSet { sigset: sigset }
}

pub fn add(&mut self, signum: SigNum) -> Result<()> {
let res = unsafe { libc::sigaddset(&mut self.sigset as *mut libc::sigset_t, signum) };

Errno::result(res).map(drop)
pub fn add(&mut self, signal: Signal) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should rename this to insert to match the *Map and *Set APIs.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could do that. Though, I would want to do that in a separate PR

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm ok with pushing that change to a later PR. This one can just be the type changing to an enum.

unsafe { libc::sigaddset(&mut self.sigset as *mut libc::sigset_t, signal as libc::c_int) };
}

pub fn clear(&mut self) -> Result<()> {
let res = unsafe { libc::sigemptyset(&mut self.sigset as *mut libc::sigset_t) };

Errno::result(res).map(drop)
pub fn clear(&mut self) {
unsafe { libc::sigemptyset(&mut self.sigset as *mut libc::sigset_t) };
}

pub fn remove(&mut self, signum: SigNum) -> Result<()> {
let res = unsafe { libc::sigdelset(&mut self.sigset as *mut libc::sigset_t, signum) };

Errno::result(res).map(drop)
pub fn remove(&mut self, signal: Signal) {
unsafe { libc::sigdelset(&mut self.sigset as *mut libc::sigset_t, signal as libc::c_int) };
}

pub fn extend(&mut self, other: &SigSet) -> Result<()> {
for i in 1..NSIG {
if try!(other.contains(i)) {
try!(self.add(i));
}
pub fn contains(&self, signal: Signal) -> bool {
let res = unsafe { libc::sigismember(&self.sigset as *const libc::sigset_t, signal as libc::c_int) };

match res {
1 => true,
0 => false,
_ => unreachable!("unexpected value from sigismember"),
}
Ok(())
}

pub fn contains(&self, signum: SigNum) -> Result<bool> {
let res = unsafe { libc::sigismember(&self.sigset as *const libc::sigset_t, signum) };

match try!(Errno::result(res)) {
1 => Ok(true),
0 => Ok(false),
_ => unreachable!("unexpected value from sigismember"),
pub fn extend(&mut self, other: &SigSet) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

later PR could then also make this take FromIterator<Item=Signal>, I like this :-)

for signal in Signal::iterator() {
if other.contains(signal) {
self.add(signal);
}
}
}

Expand Down Expand Up @@ -165,11 +264,11 @@ impl SigSet {

/// Suspends execution of the calling thread until one of the signals in the
/// signal mask becomes pending, and returns the accepted signal.
pub fn wait(&self) -> Result<SigNum> {
let mut signum: SigNum = unsafe { mem::uninitialized() };
pub fn wait(&self) -> Result<Signal> {
let mut signum: libc::c_int = unsafe { mem::uninitialized() };
let res = unsafe { libc::sigwait(&self.sigset as *const libc::sigset_t, &mut signum) };

Errno::result(res).map(|_| signum)
Errno::result(res).map(|_| Signal::from_c_int(signum).unwrap())
}
}

Expand All @@ -185,8 +284,8 @@ impl AsRef<libc::sigset_t> for SigSet {
pub enum SigHandler {
SigDfl,
SigIgn,
Handler(extern fn(SigNum)),
SigAction(extern fn(SigNum, *mut libc::siginfo_t, *mut libc::c_void))
Handler(extern fn(libc::c_int)),
SigAction(extern fn(libc::c_int, *mut libc::siginfo_t, *mut libc::c_void))
}

pub struct SigAction {
Expand Down Expand Up @@ -214,11 +313,11 @@ impl SigAction {
}
}

pub unsafe fn sigaction(signum: SigNum, sigaction: &SigAction) -> Result<SigAction> {
pub unsafe fn sigaction(signal: Signal, sigaction: &SigAction) -> Result<SigAction> {
let mut oldact = mem::uninitialized::<libc::sigaction>();

let res =
libc::sigaction(signum, &sigaction.sigaction as *const libc::sigaction, &mut oldact as *mut libc::sigaction);
libc::sigaction(signal as libc::c_int, &sigaction.sigaction as *const libc::sigaction, &mut oldact as *mut libc::sigaction);

Errno::result(res).map(|_| SigAction { sigaction: oldact })
}
Expand Down Expand Up @@ -257,14 +356,14 @@ pub fn pthread_sigmask(how: SigFlags,
Errno::result(res).map(drop)
}

pub fn kill(pid: libc::pid_t, signum: SigNum) -> Result<()> {
let res = unsafe { libc::kill(pid, signum) };
pub fn kill(pid: libc::pid_t, signal: Signal) -> Result<()> {
let res = unsafe { libc::kill(pid, signal as libc::c_int) };

Errno::result(res).map(drop)
}

pub fn raise(signum: SigNum) -> Result<()> {
let res = unsafe { libc::raise(signum) };
pub fn raise(signal: Signal) -> Result<()> {
let res = unsafe { libc::raise(signal as libc::c_int) };

Errno::result(res).map(drop)
}
Expand All @@ -276,70 +375,70 @@ mod tests {
#[test]
fn test_contains() {
let mut mask = SigSet::empty();
mask.add(SIGUSR1).unwrap();
mask.add(SIGUSR1);

assert_eq!(mask.contains(SIGUSR1), Ok(true));
assert_eq!(mask.contains(SIGUSR2), Ok(false));
assert!(mask.contains(SIGUSR1));
assert!(!mask.contains(SIGUSR2));

let all = SigSet::all();
assert_eq!(all.contains(SIGUSR1), Ok(true));
assert_eq!(all.contains(SIGUSR2), Ok(true));
assert!(all.contains(SIGUSR1));
assert!(all.contains(SIGUSR2));
}

#[test]
fn test_clear() {
let mut set = SigSet::all();
set.clear().unwrap();
for i in 1..NSIG {
assert_eq!(set.contains(i), Ok(false));
set.clear();
for signal in Signal::iterator() {
assert!(!set.contains(signal));
}
}

#[test]
fn test_extend() {
let mut one_signal = SigSet::empty();
one_signal.add(SIGUSR1).unwrap();
one_signal.add(SIGUSR1);

let mut two_signals = SigSet::empty();
two_signals.add(SIGUSR2).unwrap();
two_signals.extend(&one_signal).unwrap();
two_signals.add(SIGUSR2);
two_signals.extend(&one_signal);

assert_eq!(two_signals.contains(SIGUSR1), Ok(true));
assert_eq!(two_signals.contains(SIGUSR2), Ok(true));
assert!(two_signals.contains(SIGUSR1));
assert!(two_signals.contains(SIGUSR2));
}

#[test]
fn test_thread_signal_block() {
let mut mask = SigSet::empty();
mask.add(SIGUSR1).unwrap();
mask.add(SIGUSR1);

assert!(mask.thread_block().is_ok());
}

#[test]
fn test_thread_signal_swap() {
let mut mask = SigSet::empty();
mask.add(SIGUSR1).unwrap();
mask.add(SIGUSR1);
mask.thread_block().unwrap();

assert!(SigSet::thread_get_mask().unwrap().contains(SIGUSR1).unwrap());
assert!(SigSet::thread_get_mask().unwrap().contains(SIGUSR1));

let mask2 = SigSet::empty();
mask.add(SIGUSR2).unwrap();
mask.add(SIGUSR2);

let oldmask = mask2.thread_swap_mask(SIG_SETMASK).unwrap();

assert!(oldmask.contains(SIGUSR1).unwrap());
assert!(!oldmask.contains(SIGUSR2).unwrap());
assert!(oldmask.contains(SIGUSR1));
assert!(!oldmask.contains(SIGUSR2));
}

// TODO(#251): Re-enable after figuring out flakiness.
#[cfg(not(any(target_os = "macos", target_os = "ios")))]
#[test]
fn test_sigwait() {
let mut mask = SigSet::empty();
mask.add(SIGUSR1).unwrap();
mask.add(SIGUSR2).unwrap();
mask.add(SIGUSR1);
mask.add(SIGUSR2);
mask.thread_block().unwrap();

raise(SIGUSR1).unwrap();
Expand Down
Loading