Merge #73

73: Remove unnecessary immediately invoked closures r=petreeftime a=souze

For the tests, they where likely added before tests could return Result.
For the code, likely got easier to handle with thiserror crate

Co-authored-by: Erik Karlsson <triggger@gmail.com>

Author: bors[bot]

src/process.rs

@@ -88,47 +88,44 @@ fn ptsname_r(fd: &PtyMaster) -> nix::Result<String> {
 impl PtyProcess {
     /// Start a process in a forked pty
     pub fn new(mut command: Command) -> Result<Self, Error> {
-        || -> nix::Result<Self> {
-            // Open a new PTY master
-            let master_fd = posix_openpt(OFlag::O_RDWR)?;
+        // Open a new PTY master
+        let master_fd = posix_openpt(OFlag::O_RDWR)?;
 
-            // Allow a slave to be generated for it
-            grantpt(&master_fd)?;
-            unlockpt(&master_fd)?;
+        // Allow a slave to be generated for it
+        grantpt(&master_fd)?;
+        unlockpt(&master_fd)?;
 
-            // on Linux this is the libc function, on OSX this is our implementation of ptsname_r
-            let slave_name = ptsname_r(&master_fd)?;
+        // on Linux this is the libc function, on OSX this is our implementation of ptsname_r
+        let slave_name = ptsname_r(&master_fd)?;
 
-            match fork()? {
-                ForkResult::Child => {
-                    setsid()?; // create new session with child as session leader
-                    let slave_fd = open(
-                        std::path::Path::new(&slave_name),
-                        OFlag::O_RDWR,
-                        stat::Mode::empty(),
-                    )?;
+        match fork()? {
+            ForkResult::Child => {
+                setsid()?; // create new session with child as session leader
+                let slave_fd = open(
+                    std::path::Path::new(&slave_name),
+                    OFlag::O_RDWR,
+                    stat::Mode::empty(),
+                )?;
 
-                    // assign stdin, stdout, stderr to the tty, just like a terminal does
-                    dup2(slave_fd, STDIN_FILENO)?;
-                    dup2(slave_fd, STDOUT_FILENO)?;
-                    dup2(slave_fd, STDERR_FILENO)?;
+                // assign stdin, stdout, stderr to the tty, just like a terminal does
+                dup2(slave_fd, STDIN_FILENO)?;
+                dup2(slave_fd, STDOUT_FILENO)?;
+                dup2(slave_fd, STDERR_FILENO)?;
 
-                    // set echo off
-                    let mut flags = termios::tcgetattr(STDIN_FILENO)?;
-                    flags.local_flags &= !termios::LocalFlags::ECHO;
-                    termios::tcsetattr(STDIN_FILENO, termios::SetArg::TCSANOW, &flags)?;
+                // set echo off
+                let mut flags = termios::tcgetattr(STDIN_FILENO)?;
+                flags.local_flags &= !termios::LocalFlags::ECHO;
+                termios::tcsetattr(STDIN_FILENO, termios::SetArg::TCSANOW, &flags)?;
 
-                    command.exec();
-                    Err(nix::Error::last())
-                }
-                ForkResult::Parent { child: child_pid } => Ok(PtyProcess {
-                    pty: master_fd,
-                    child_pid,
-                    kill_timeout: None,
-                }),
+                command.exec();
+                Err(Error::Nix(nix::Error::last()))
             }
-        }()
-        .map_err(Error::from)
+            ForkResult::Parent { child: child_pid } => Ok(PtyProcess {
+                pty: master_fd,
+                child_pid,
+                kill_timeout: None,
+            }),
+        }
     }
 
     /// Get handle to pty fork for reading/writing
@@ -242,28 +239,23 @@ mod tests {
 
     #[test]
     /// Open cat, write string, read back string twice, send Ctrl^C and check that cat exited
-    fn test_cat() {
-        // wrapping into closure so I can use ?
-        || -> std::io::Result<()> {
-            let process = PtyProcess::new(Command::new("cat")).expect("could not execute cat");
-            let f = process.get_file_handle();
-            let mut writer = LineWriter::new(&f);
-            let mut reader = BufReader::new(&f);
-            let _ = writer.write(b"hello cat\n")?;
-            let mut buf = String::new();
-            reader.read_line(&mut buf)?;
-            assert_eq!(buf, "hello cat\r\n");
+    fn test_cat() -> std::io::Result<()> {
+        let process = PtyProcess::new(Command::new("cat")).expect("could not execute cat");
+        let f = process.get_file_handle();
+        let mut writer = LineWriter::new(&f);
+        let mut reader = BufReader::new(&f);
+        let _ = writer.write(b"hello cat\n")?;
+        let mut buf = String::new();
+        reader.read_line(&mut buf)?;
+        assert_eq!(buf, "hello cat\r\n");
 
-            // this sleep solves an edge case of some cases when cat is somehow not "ready"
-            // to take the ^C (occasional test hangs)
-            thread::sleep(time::Duration::from_millis(100));
-            writer.write_all(&[3])?; // send ^C
-            writer.flush()?;
-            let should =
-                wait::WaitStatus::Signaled(process.child_pid, signal::Signal::SIGINT, false);
-            assert_eq!(should, wait::waitpid(process.child_pid, None).unwrap());
-            Ok(())
-        }()
-        .unwrap_or_else(|e| panic!("test_cat failed: {}", e));
+        // this sleep solves an edge case of some cases when cat is somehow not "ready"
+        // to take the ^C (occasional test hangs)
+        thread::sleep(time::Duration::from_millis(100));
+        writer.write_all(&[3])?; // send ^C
+        writer.flush()?;
+        let should = wait::WaitStatus::Signaled(process.child_pid, signal::Signal::SIGINT, false);
+        assert_eq!(should, wait::waitpid(process.child_pid, None).unwrap());
+        Ok(())
     }
 }

src/reader.rs

@@ -123,29 +123,27 @@ impl NBReader {
         let (tx, rx) = channel();
 
         // spawn a thread which reads one char and sends it to tx
-        thread::spawn(move || {
-            let _ = || -> Result<(), Error> {
-                let mut reader = BufReader::new(f);
-                let mut byte = [0u8];
-                loop {
-                    match reader.read(&mut byte) {
-                        Ok(0) => {
-                            tx.send(Ok(PipedChar::EOF))
-                                .map_err(|_| Error::MpscSendError)?;
-                            break;
-                        }
-                        Ok(_) => {
-                            tx.send(Ok(PipedChar::Char(byte[0])))
-                                .map_err(|_| Error::MpscSendError)?;
-                        }
-                        Err(error) => {
-                            tx.send(Err(PipeError::IO(error)))
-                                .map_err(|_| Error::MpscSendError)?;
-                        }
+        thread::spawn(move || -> Result<(), Error> {
+            let mut reader = BufReader::new(f);
+            let mut byte = [0u8];
+            loop {
+                match reader.read(&mut byte) {
+                    Ok(0) => {
+                        tx.send(Ok(PipedChar::EOF))
+                            .map_err(|_| Error::MpscSendError)?;
+                        break;
+                    }
+                    Ok(_) => {
+                        tx.send(Ok(PipedChar::Char(byte[0])))
+                            .map_err(|_| Error::MpscSendError)?;
+                    }
+                    Err(error) => {
+                        tx.send(Err(PipeError::IO(error)))
+                            .map_err(|_| Error::MpscSendError)?;
                     }
                 }
-                Ok(())
-            }();
+            }
+            Ok(())
             // don't do error handling as on an error it was most probably
             // the main thread which exited (remote hangup)
         });

src/session.rs

@@ -431,34 +431,28 @@ mod tests {
     use super::*;
 
     #[test]
-    fn test_read_line() {
-        || -> Result<(), Error> {
-            let mut s = spawn("cat", Some(1000))?;
-            s.send_line("hans")?;
-            assert_eq!("hans", s.read_line()?);
-            let should = crate::process::wait::WaitStatus::Signaled(
-                s.process.child_pid,
-                crate::process::signal::Signal::SIGTERM,
-                false,
-            );
-            assert_eq!(should, s.process.exit()?);
-            Ok(())
-        }()
-        .unwrap_or_else(|e| panic!("test_read_line failed: {}", e));
+    fn test_read_line() -> Result<(), Error> {
+        let mut s = spawn("cat", Some(1000))?;
+        s.send_line("hans")?;
+        assert_eq!("hans", s.read_line()?);
+        let should = crate::process::wait::WaitStatus::Signaled(
+            s.process.child_pid,
+            crate::process::signal::Signal::SIGTERM,
+            false,
+        );
+        assert_eq!(should, s.process.exit()?);
+        Ok(())
     }
 
     #[test]
-    fn test_expect_eof_timeout() {
-        || -> Result<(), Error> {
-            let mut p = spawn("sleep 3", Some(1000)).expect("cannot run sleep 3");
-            match p.exp_eof() {
-                Ok(_) => panic!("should raise Timeout"),
-                Err(Error::Timeout { .. }) => {}
-                Err(_) => panic!("should raise TimeOut"),
-            }
-            Ok(())
-        }()
-        .unwrap_or_else(|e| panic!("test_timeout failed: {}", e));
+    fn test_expect_eof_timeout() -> Result<(), Error> {
+        let mut p = spawn("sleep 3", Some(1000)).expect("cannot run sleep 3");
+        match p.exp_eof() {
+            Ok(_) => panic!("should raise Timeout"),
+            Err(Error::Timeout { .. }) => {}
+            Err(_) => panic!("should raise TimeOut"),
+        }
+        Ok(())
     }
 
     #[test]
@@ -468,44 +462,35 @@ mod tests {
     }
 
     #[test]
-    fn test_expect_string() {
-        || -> Result<(), Error> {
-            let mut p = spawn("cat", Some(1000)).expect("cannot run cat");
-            p.send_line("hello world!")?;
-            p.exp_string("hello world!")?;
-            p.send_line("hello heaven!")?;
-            p.exp_string("hello heaven!")?;
-            Ok(())
-        }()
-        .unwrap_or_else(|e| panic!("test_expect_string failed: {}", e));
+    fn test_expect_string() -> Result<(), Error> {
+        let mut p = spawn("cat", Some(1000)).expect("cannot run cat");
+        p.send_line("hello world!")?;
+        p.exp_string("hello world!")?;
+        p.send_line("hello heaven!")?;
+        p.exp_string("hello heaven!")?;
+        Ok(())
     }
 
     #[test]
-    fn test_read_string_before() {
-        || -> Result<(), Error> {
-            let mut p = spawn("cat", Some(1000)).expect("cannot run cat");
-            p.send_line("lorem ipsum dolor sit amet")?;
-            assert_eq!("lorem ipsum dolor sit ", p.exp_string("amet")?);
-            Ok(())
-        }()
-        .unwrap_or_else(|e| panic!("test_read_string_before failed: {}", e));
+    fn test_read_string_before() -> Result<(), Error> {
+        let mut p = spawn("cat", Some(1000)).expect("cannot run cat");
+        p.send_line("lorem ipsum dolor sit amet")?;
+        assert_eq!("lorem ipsum dolor sit ", p.exp_string("amet")?);
+        Ok(())
     }
 
     #[test]
-    fn test_expect_any() {
-        || -> Result<(), Error> {
-            let mut p = spawn("cat", Some(1000)).expect("cannot run cat");
-            p.send_line("Hi")?;
-            match p.exp_any(vec![
-                ReadUntil::NBytes(3),
-                ReadUntil::String("Hi".to_string()),
-            ]) {
-                Ok(s) => assert_eq!(("".to_string(), "Hi".to_string()), s),
-                Err(e) => panic!("got error: {}", e),
-            }
-            Ok(())
-        }()
-        .unwrap_or_else(|e| panic!("test_expect_any failed: {}", e));
+    fn test_expect_any() -> Result<(), Error> {
+        let mut p = spawn("cat", Some(1000)).expect("cannot run cat");
+        p.send_line("Hi")?;
+        match p.exp_any(vec![
+            ReadUntil::NBytes(3),
+            ReadUntil::String("Hi".to_string()),
+        ]) {
+            Ok(s) => assert_eq!(("".to_string(), "Hi".to_string()), s),
+            Err(e) => panic!("got error: {}", e),
+        }
+        Ok(())
     }
 
     #[test]
@@ -519,46 +504,37 @@ mod tests {
     }
 
     #[test]
-    fn test_kill_timeout() {
-        || -> Result<(), Error> {
-            let mut p = spawn_bash(Some(1000))?;
-            p.execute("cat <(echo ready) -", "ready")?;
-            Ok(())
-        }()
-        .unwrap_or_else(|e| panic!("test_kill_timeout failed: {}", e));
+    fn test_kill_timeout() -> Result<(), Error> {
+        let mut p = spawn_bash(Some(1000))?;
+        p.execute("cat <(echo ready) -", "ready")?;
+        Ok(())
         // p is dropped here and kill is sent immediately to bash
         // Since that is not enough to make bash exit, a kill -9 is sent within 1s (timeout)
     }
 
     #[test]
-    fn test_bash() {
-        || -> Result<(), Error> {
-            let mut p = spawn_bash(Some(1000))?;
-            p.send_line("cd /tmp/")?;
-            p.wait_for_prompt()?;
-            p.send_line("pwd")?;
-            assert_eq!("/tmp\r\n", p.wait_for_prompt()?);
-            Ok(())
-        }()
-        .unwrap_or_else(|e| panic!("test_bash failed: {}", e));
+    fn test_bash() -> Result<(), Error> {
+        let mut p = spawn_bash(Some(1000))?;
+        p.send_line("cd /tmp/")?;
+        p.wait_for_prompt()?;
+        p.send_line("pwd")?;
+        assert_eq!("/tmp\r\n", p.wait_for_prompt()?);
+        Ok(())
     }
 
     #[test]
-    fn test_bash_control_chars() {
-        || -> Result<(), Error> {
-            let mut p = spawn_bash(Some(1000))?;
-            p.execute("cat <(echo ready) -", "ready")?;
-            p.send_control('c')?; // abort: SIGINT
-            p.wait_for_prompt()?;
-            p.execute("cat <(echo ready) -", "ready")?;
-            p.send_control('z')?; // suspend:SIGTSTPcon
-            p.exp_regex(r"(Stopped|suspended)\s+cat .*")?;
-            p.send_line("fg")?;
-            p.execute("cat <(echo ready) -", "ready")?;
-            p.send_control('c')?;
-            Ok(())
-        }()
-        .unwrap_or_else(|e| panic!("test_bash_control_chars failed: {}", e));
+    fn test_bash_control_chars() -> Result<(), Error> {
+        let mut p = spawn_bash(Some(1000))?;
+        p.execute("cat <(echo ready) -", "ready")?;
+        p.send_control('c')?; // abort: SIGINT
+        p.wait_for_prompt()?;
+        p.execute("cat <(echo ready) -", "ready")?;
+        p.send_control('z')?; // suspend:SIGTSTPcon
+        p.exp_regex(r"(Stopped|suspended)\s+cat .*")?;
+        p.send_line("fg")?;
+        p.execute("cat <(echo ready) -", "ready")?;
+        p.send_control('c')?;
+        Ok(())
     }
 
     #[test]