auto merge of #10109 : pcmattman/rust/pass-nonzero-exit-status-on-termination-by-signal, r=alexcrichton

The UvProcess exit callback is called with a zero exit status and non-zero termination signal when a child is terminated by a signal.

If a parent checks only the exit status (for example, only checks the return value from `wait()`), it may believe the process completed successfully when it actually failed.

Helpers for common use-cases are in `std::rt::io::process`.

Should resolve https://github.com/mozilla/rust/issues/10062.

3 files changed, 56 insertions, 16 deletions

diff --git a/src/libstd/rt/io/process.rs b/src/libstd/rt/io/process.rs
index ae087099d1f..6b21cde2488 100644
--- a/src/libstd/rt/io/process.rs
+++ b/src/libstd/rt/io/process.rs
@@ -18,6 +18,8 @@ use rt::io;
 use rt::io::io_error;
 use rt::rtio::{RtioProcess, IoFactory, with_local_io};
 
+use fmt;
+
 // windows values don't matter as long as they're at least one of unix's
 // TERM/KILL/INT signals
 #[cfg(windows)] pub static PleaseExitSignal: int = 15;
@@ -79,6 +81,40 @@ pub enum StdioContainer {
     CreatePipe(bool /* readable */, bool /* writable */),
 }
 
+/// Describes the result of a process after it has terminated.
+#[deriving(Eq)]
+pub enum ProcessExit {
+    /// Normal termination with an exit status.
+    ExitStatus(int),
+
+    /// Termination by signal, with the signal number.
+    ExitSignal(int),
+}
+
+impl fmt::Default for ProcessExit {
+    /// Format a ProcessExit enum, to nicely present the information.
+    fn fmt(obj: &ProcessExit, f: &mut fmt::Formatter) {
+        match *obj {
+            ExitStatus(code) =>  write!(f.buf, "exit code: {}", code),
+            ExitSignal(code) =>  write!(f.buf, "signal: {}", code),
+        }
+    }
+}
+
+impl ProcessExit {
+    /// Was termination successful? Signal termination not considered a success,
+    /// and success is defined as a zero exit status.
+    pub fn success(&self) -> bool {
+        return self.matches_exit_status(0);
+    }
+
+    /// Checks whether this ProcessExit matches the given exit status.
+    /// Termination by signal will never match an exit code.
+    pub fn matches_exit_status(&self, wanted: int) -> bool {
+        *self == ExitStatus(wanted)
+    }
+}
+
 impl Process {
     /// Creates a new pipe initialized, but not bound to any particular
     /// source/destination
@@ -122,7 +158,7 @@ impl Process {
     /// Wait for the child to exit completely, returning the status that it
     /// exited with. This function will continue to have the same return value
     /// after it has been called at least once.
-    pub fn wait(&mut self) -> int { self.handle.wait() }
+    pub fn wait(&mut self) -> ProcessExit { self.handle.wait() }
 }
 
 impl Drop for Process {
diff --git a/src/libstd/rt/rtio.rs b/src/libstd/rt/rtio.rs
index d623914cdad..9b1103b8a74 100644
--- a/src/libstd/rt/rtio.rs
+++ b/src/libstd/rt/rtio.rs
@@ -18,7 +18,7 @@ use c_str::CString;
 use ai = rt::io::net::addrinfo;
 use rt::io::IoError;
 use rt::io::signal::Signum;
-use super::io::process::ProcessConfig;
+use super::io::process::{ProcessConfig, ProcessExit};
 use super::io::net::ip::{IpAddr, SocketAddr};
 use path::Path;
 use super::io::{SeekStyle};
@@ -201,7 +201,7 @@ pub trait RtioFileStream {
 pub trait RtioProcess {
     fn id(&self) -> libc::pid_t;
     fn kill(&mut self, signal: int) -> Result<(), IoError>;
-    fn wait(&mut self) -> int;
+    fn wait(&mut self) -> ProcessExit;
 }
 
 pub trait RtioPipe {
diff --git a/src/libstd/run.rs b/src/libstd/run.rs
index 844f61dda0b..c9b33a14c52 100644
--- a/src/libstd/run.rs
+++ b/src/libstd/run.rs
@@ -18,6 +18,7 @@ use libc::{pid_t, c_int};
 use libc;
 use prelude::*;
 use rt::io::process;
+use rt::io::process::ProcessExit;
 use rt::io;
 use rt::io::Reader;
 use task;
@@ -100,7 +101,7 @@ impl <'self> ProcessOptions<'self> {
 /// The output of a finished process.
 pub struct ProcessOutput {
     /// The status (exit code) of the process.
-    status: int,
+    status: ProcessExit,
 
     /// The data that the process wrote to stdout.
     output: ~[u8],
@@ -194,7 +195,7 @@ impl Process {
      *
      * If the child has already been finished then the exit code is returned.
      */
-    pub fn finish(&mut self) -> int { self.inner.wait() }
+    pub fn finish(&mut self) -> ProcessExit { self.inner.wait() }
 
     /**
      * Closes the handle to stdin, waits for the child process to terminate, and
@@ -296,7 +297,7 @@ impl Process {
  *
  * The process's exit code
  */
-pub fn process_status(prog: &str, args: &[~str]) -> int {
+pub fn process_status(prog: &str, args: &[~str]) -> ProcessExit {
     let mut prog = Process::new(prog, args, ProcessOptions {
         env: None,
         dir: None,
@@ -340,8 +341,11 @@ mod tests {
     #[test]
     #[cfg(not(target_os="android"))] // FIXME(#10380)
     fn test_process_status() {
-        assert_eq!(run::process_status("false", []), 1);
-        assert_eq!(run::process_status("true", []), 0);
+        let mut status = run::process_status("false", []);
+        assert!(status.matches_exit_status(1));
+
+        status = run::process_status("true", []);
+        assert!(status.success());
     }
 
     #[test]
@@ -352,7 +356,7 @@ mod tests {
              = run::process_output("echo", [~"hello"]);
         let output_str = str::from_utf8(output);
 
-        assert_eq!(status, 0);
+        assert!(status.success());
         assert_eq!(output_str.trim().to_owned(), ~"hello");
         // FIXME #7224
         if !running_on_valgrind() {
@@ -367,7 +371,7 @@ mod tests {
         let run::ProcessOutput {status, output, error}
              = run::process_output("mkdir", [~"."]);
 
-        assert_eq!(status, 1);
+        assert!(status.matches_exit_status(1));
         assert_eq!(output, ~[]);
         assert!(!error.is_empty());
     }
@@ -424,15 +428,15 @@ mod tests {
     #[cfg(not(target_os="android"))] // FIXME(#10380)
     fn test_finish_once() {
         let mut prog = run::Process::new("false", [], run::ProcessOptions::new());
-        assert_eq!(prog.finish(), 1);
+        assert!(prog.finish().matches_exit_status(1));
     }
 
     #[test]
     #[cfg(not(target_os="android"))] // FIXME(#10380)
     fn test_finish_twice() {
         let mut prog = run::Process::new("false", [], run::ProcessOptions::new());
-        assert_eq!(prog.finish(), 1);
-        assert_eq!(prog.finish(), 1);
+        assert!(prog.finish().matches_exit_status(1));
+        assert!(prog.finish().matches_exit_status(1));
     }
 
     #[test]
@@ -444,7 +448,7 @@ mod tests {
             = prog.finish_with_output();
         let output_str = str::from_utf8(output);
 
-        assert_eq!(status, 0);
+        assert!(status.success());
         assert_eq!(output_str.trim().to_owned(), ~"hello");
         // FIXME #7224
         if !running_on_valgrind() {
@@ -462,7 +466,7 @@ mod tests {
 
         let output_str = str::from_utf8(output);
 
-        assert_eq!(status, 0);
+        assert!(status.success());
         assert_eq!(output_str.trim().to_owned(), ~"hello");
         // FIXME #7224
         if !running_on_valgrind() {
@@ -472,7 +476,7 @@ mod tests {
         let run::ProcessOutput {status, output, error}
             = prog.finish_with_output();
 
-        assert_eq!(status, 0);
+        assert!(status.success());
         assert_eq!(output, ~[]);
         // FIXME #7224
         if !running_on_valgrind() {