refactor(IO): random fixes (#521)

* refactor(os_input_output): use Pid for child process

* fix(debug): change debug_to_file to write &[u8]

This patch changes debug_to_file to write entire slices in one call, to
avoid the overhead of opening and closing files for each byte written.

* refactor(ServerOsApi): remove unnecessary muts from methods

Co-authored-by: KOVACS Tamas <ktamas@fastmail.fm>

6 files changed, 48 insertions, 49 deletions

diff --git a/src/tests/fakes.rs b/src/tests/fakes.rs
index 09cc2ba3d..44c0b2ce2 100644
--- a/src/tests/fakes.rs
+++ b/src/tests/fakes.rs
@@ -8,7 +8,7 @@ use std::path::PathBuf;
 use std::sync::{mpsc, Arc, Condvar, Mutex};
 use std::time::{Duration, Instant};
 use zellij_client::os_input_output::ClientOsApi;
-use zellij_server::os_input_output::ServerOsApi;
+use zellij_server::os_input_output::{Pid, ServerOsApi};
 use zellij_tile::data::Palette;
 use zellij_utils::{
     channels::{ChannelWithContext, SenderType, SenderWithContext},
@@ -22,7 +22,7 @@ const MIN_TIME_BETWEEN_SNAPSHOTS: Duration = Duration::from_millis(150);
 
 #[derive(Clone)]
 pub enum IoEvent {
-    Kill(RawFd),
+    Kill(Pid),
     SetTerminalSizeUsingFd(RawFd, u16, u16),
     IntoRawMode(RawFd),
     UnsetRawMode(RawFd),
@@ -127,7 +127,7 @@ impl FakeInputOutput {
         }
         self.stdin_commands = Arc::new(Mutex::new(stdin_commands));
     }
-    pub fn add_terminal(&mut self, fd: RawFd) {
+    pub fn add_terminal(&self, fd: RawFd) {
         self.stdin_writes.lock().unwrap().insert(fd, vec![]);
     }
     pub fn add_sigwinch_event(&mut self, new_position_and_size: PositionAndSize) {
@@ -225,7 +225,7 @@ impl ClientOsApi for FakeInputOutput {
 }
 
 impl ServerOsApi for FakeInputOutput {
-    fn set_terminal_size_using_fd(&mut self, pid: RawFd, cols: u16, rows: u16) {
+    fn set_terminal_size_using_fd(&self, pid: RawFd, cols: u16, rows: u16) {
         let terminal_input = self
             .possible_tty_inputs
             .get(&cols)
@@ -239,12 +239,15 @@ impl ServerOsApi for FakeInputOutput {
             .unwrap()
             .push(IoEvent::SetTerminalSizeUsingFd(pid, cols, rows));
     }
-    fn spawn_terminal(&mut self, _file_to_open: Option<PathBuf>) -> (RawFd, RawFd) {
+    fn spawn_terminal(&self, _file_to_open: Option<PathBuf>) -> (RawFd, Pid) {
         let next_terminal_id = self.stdin_writes.lock().unwrap().keys().len() as RawFd + 1;
         self.add_terminal(next_terminal_id);
-        (next_terminal_id as i32, next_terminal_id + 1000) // secondary number is arbitrary here
+        (
+            next_terminal_id as i32,
+            Pid::from_raw(next_terminal_id + 1000),
+        ) // secondary number is arbitrary here
     }
-    fn write_to_tty_stdin(&mut self, pid: RawFd, buf: &mut [u8]) -> Result<usize, nix::Error> {
+    fn write_to_tty_stdin(&self, pid: RawFd, buf: &[u8]) -> Result<usize, nix::Error> {
         let mut stdin_writes = self.stdin_writes.lock().unwrap();
         let write_buffer = stdin_writes.get_mut(&pid).unwrap();
         let mut bytes_written = 0;
@@ -254,7 +257,7 @@ impl ServerOsApi for FakeInputOutput {
         }
         Ok(bytes_written)
     }
-    fn read_from_tty_stdout(&mut self, pid: RawFd, buf: &mut [u8]) -> Result<usize, nix::Error> {
+    fn read_from_tty_stdout(&self, pid: RawFd, buf: &mut [u8]) -> Result<usize, nix::Error> {
         let mut read_buffers = self.read_buffers.lock().unwrap();
         let mut bytes_read = 0;
         match read_buffers.get_mut(&pid) {
@@ -271,15 +274,15 @@ impl ServerOsApi for FakeInputOutput {
             None => Err(nix::Error::Sys(nix::errno::Errno::EAGAIN)),
         }
     }
-    fn tcdrain(&mut self, pid: RawFd) -> Result<(), nix::Error> {
+    fn tcdrain(&self, pid: RawFd) -> Result<(), nix::Error> {
         self.io_events.lock().unwrap().push(IoEvent::TcDrain(pid));
         Ok(())
     }
     fn box_clone(&self) -> Box<dyn ServerOsApi> {
         Box::new((*self).clone())
     }
-    fn kill(&mut self, fd: RawFd) -> Result<(), nix::Error> {
-        self.io_events.lock().unwrap().push(IoEvent::Kill(fd));
+    fn kill(&self, pid: Pid) -> Result<(), nix::Error> {
+        self.io_events.lock().unwrap().push(IoEvent::Kill(pid));
         Ok(())
     }
     fn recv_from_client(&self) -> (ClientToServerMsg, ErrorContext) {
@@ -292,7 +295,7 @@ impl ServerOsApi for FakeInputOutput {
     fn send_to_client(&self, msg: ServerToClientMsg) {
         self.send_instructions_to_client.send(msg).unwrap();
     }
-    fn add_client_sender(&mut self) {}
+    fn add_client_sender(&self) {}
     fn update_receiver(&mut self, _stream: LocalSocketStream) {}
     fn load_palette(&self) -> Palette {
         default_palette()
diff --git a/zellij-server/src/os_input_output.rs b/zellij-server/src/os_input_output.rs
index 1fef826b8..bd5e9a486 100644
--- a/zellij-server/src/os_input_output.rs
+++ b/zellij-server/src/os_input_output.rs
@@ -4,7 +4,7 @@ use nix::pty::{forkpty, Winsize};
 use nix::sys::signal::{kill, Signal};
 use nix::sys::termios;
 use nix::sys::wait::waitpid;
-use nix::unistd::{self, ForkResult, Pid};
+use nix::unistd::{self, ForkResult};
 use signal_hook::consts::*;
 use std::env;
 use std::os::unix::io::RawFd;
@@ -18,6 +18,7 @@ use zellij_utils::ipc::{
 };
 use zellij_utils::shared::default_palette;
 
+pub use nix::unistd::Pid;
 pub(crate) fn set_terminal_size_using_fd(fd: RawFd, columns: u16, rows: u16) {
     // TODO: do this with the nix ioctl
     use libc::ioctl;
@@ -76,8 +77,8 @@ fn handle_command_exit(mut child: Child) {
 /// set.
 // FIXME this should probably be split into different functions, or at least have less levels
 // of indentation in some way
-fn spawn_terminal(file_to_open: Option<PathBuf>, orig_termios: termios::Termios) -> (RawFd, RawFd) {
-    let (pid_primary, pid_secondary): (RawFd, RawFd) = {
+fn spawn_terminal(file_to_open: Option<PathBuf>, orig_termios: termios::Termios) -> (RawFd, Pid) {
+    let (pid_primary, pid_secondary): (RawFd, Pid) = {
         match forkpty(None, Some(&orig_termios)) {
             Ok(fork_pty_res) => {
                 let pid_primary = fork_pty_res.master;
@@ -112,7 +113,7 @@ fn spawn_terminal(file_to_open: Option<PathBuf>, orig_termios: termios::Termios)
                         }
                     },
                 };
-                (pid_primary, pid_secondary.as_raw())
+                (pid_primary, pid_secondary)
             }
             Err(e) => {
                 panic!("failed to fork {:?}", e);
@@ -133,20 +134,17 @@ pub struct ServerOsInputOutput {
 /// Zellij server requires.
 pub trait ServerOsApi: Send + Sync {
     /// Sets the size of the terminal associated to file descriptor `fd`.
-    fn set_terminal_size_using_fd(&mut self, fd: RawFd, cols: u16, rows: u16);
+    fn set_terminal_size_using_fd(&self, fd: RawFd, cols: u16, rows: u16);
     /// Spawn a new terminal, with an optional file to open in a terminal program.
-    fn spawn_terminal(&mut self, file_to_open: Option<PathBuf>) -> (RawFd, RawFd);
+    fn spawn_terminal(&self, file_to_open: Option<PathBuf>) -> (RawFd, Pid);
     /// Read bytes from the standard output of the virtual terminal referred to by `fd`.
-    fn read_from_tty_stdout(&mut self, fd: RawFd, buf: &mut [u8]) -> Result<usize, nix::Error>;
+    fn read_from_tty_stdout(&self, fd: RawFd, buf: &mut [u8]) -> Result<usize, nix::Error>;
     /// Write bytes to the standard input of the virtual terminal referred to by `fd`.
-    fn write_to_tty_stdin(&mut self, fd: RawFd, buf: &mut [u8]) -> Result<usize, nix::Error>;
+    fn write_to_tty_stdin(&self, fd: RawFd, buf: &[u8]) -> Result<usize, nix::Error>;
     /// Wait until all output written to the object referred to by `fd` has been transmitted.
-    fn tcdrain(&mut self, fd: RawFd) -> Result<(), nix::Error>;
+    fn tcdrain(&self, fd: RawFd) -> Result<(), nix::Error>;
     /// Terminate the process with process ID `pid`.
-    // FIXME `RawFd` is semantically the wrong type here. It should either be a raw libc::pid_t,
-    // or a nix::unistd::Pid. See `man kill.3`, nix::sys::signal::kill (both take an argument
-    // called `pid` and of type `pid_t`, and not `fd`)
-    fn kill(&mut self, pid: RawFd) -> Result<(), nix::Error>;
+    fn kill(&self, pid: Pid) -> Result<(), nix::Error>;
     /// Returns a [`Box`] pointer to this [`ServerOsApi`] struct.
     fn box_clone(&self) -> Box<dyn ServerOsApi>;
     /// Receives a message on server-side IPC channel
@@ -154,41 +152,41 @@ pub trait ServerOsApi: Send + Sync {
     /// Sends a message to client
     fn send_to_client(&self, msg: ServerToClientMsg);
     /// Adds a sender to client
-    fn add_client_sender(&mut self);
+    fn add_client_sender(&self);
     /// Update the receiver socket for the client
     fn update_receiver(&mut self, stream: LocalSocketStream);
     fn load_palette(&self) -> Palette;
 }
 
 impl ServerOsApi for ServerOsInputOutput {
-    fn set_terminal_size_using_fd(&mut self, fd: RawFd, cols: u16, rows: u16) {
+    fn set_terminal_size_using_fd(&self, fd: RawFd, cols: u16, rows: u16) {
         set_terminal_size_using_fd(fd, cols, rows);
     }
-    fn spawn_terminal(&mut self, file_to_open: Option<PathBuf>) -> (RawFd, RawFd) {
+    fn spawn_terminal(&self, file_to_open: Option<PathBuf>) -> (RawFd, Pid) {
         let orig_termios = self.orig_termios.lock().unwrap();
         spawn_terminal(file_to_open, orig_termios.clone())
     }
-    fn read_from_tty_stdout(&mut self, fd: RawFd, buf: &mut [u8]) -> Result<usize, nix::Error> {
+    fn read_from_tty_stdout(&self, fd: RawFd, buf: &mut [u8]) -> Result<usize, nix::Error> {
         unistd::read(fd, buf)
     }
-    fn write_to_tty_stdin(&mut self, fd: RawFd, buf: &mut [u8]) -> Result<usize, nix::Error> {
+    fn write_to_tty_stdin(&self, fd: RawFd, buf: &[u8]) -> Result<usize, nix::Error> {
         unistd::write(fd, buf)
     }
-    fn tcdrain(&mut self, fd: RawFd) -> Result<(), nix::Error> {
+    fn tcdrain(&self, fd: RawFd) -> Result<(), nix::Error> {
         termios::tcdrain(fd)
     }
     fn box_clone(&self) -> Box<dyn ServerOsApi> {
         Box::new((*self).clone())
     }
-    fn kill(&mut self, pid: RawFd) -> Result<(), nix::Error> {
+    fn kill(&self, pid: Pid) -> Result<(), nix::Error> {
         // TODO:
         // Ideally, we should be using SIGINT rather than SIGKILL here, but there are cases in which
         // the terminal we're trying to kill hangs on SIGINT and so all the app gets stuck
         // that's why we're sending SIGKILL here
         // A better solution would be to send SIGINT here and not wait for it, and then have
         // a background thread do the waitpid stuff and send SIGKILL if the process is stuck
-        kill(Pid::from_raw(pid), Some(Signal::SIGKILL)).unwrap();
-        waitpid(Pid::from_raw(pid), None).unwrap();
+        kill(pid, Some(Signal::SIGKILL)).unwrap();
+        waitpid(pid, None).unwrap();
         Ok(())
     }
     fn recv_from_client(&self) -> (ClientToServerMsg, ErrorContext) {
@@ -207,7 +205,7 @@ impl ServerOsApi for ServerOsInputOutput {
             .unwrap()
             .send(msg);
     }
-    fn add_client_sender(&mut self) {
+    fn add_client_sender(&self) {
         assert!(self.send_instructions_to_client.lock().unwrap().is_none());
         let sender = self
             .receive_instructions_from_client
diff --git a/zellij-server/src/pty.rs b/zellij-server/src/pty.rs
index 373b5f0b5..9bf1bddfd 100644
--- a/zellij-server/src/pty.rs
+++ b/zellij-server/src/pty.rs
@@ -8,7 +8,7 @@ use std::pin::*;
 use std::time::{Duration, Instant};
 
 use crate::{
-    os_input_output::ServerOsApi,
+    os_input_output::{Pid, ServerOsApi},
     panes::PaneId,
     screen::ScreenInstruction,
     thread_bus::{Bus, ThreadSenders},
@@ -37,7 +37,7 @@ impl ReadFromPid {
 
 impl Stream for ReadFromPid {
     type Item = Vec<u8>;
-    fn poll_next(mut self: Pin<&mut Self>, _cx: &mut Context<'_>) -> Poll<Option<Self::Item>> {
+    fn poll_next(self: Pin<&mut Self>, _cx: &mut Context<'_>) -> Poll<Option<Self::Item>> {
         let mut read_buffer = [0; 65535];
         let pid = self.pid;
         let read_result = &self.os_input.read_from_tty_stdout(pid, &mut read_buffer);
@@ -97,7 +97,7 @@ impl From<&PtyInstruction> for PtyContext {
 
 pub(crate) struct Pty {
     pub bus: Bus<PtyInstruction>,
-    pub id_to_child_pid: HashMap<RawFd, RawFd>,
+    pub id_to_child_pid: HashMap<RawFd, Pid>,
     debug_to_file: bool,
     task_handles: HashMap<RawFd, JoinHandle<()>>,
 }
@@ -177,9 +177,7 @@ fn stream_terminal_bytes(
             while let Some(bytes) = terminal_bytes.next().await {
                 let bytes_is_empty = bytes.is_empty();
                 if debug {
-                    for byte in bytes.iter() {
-                        debug_to_file(*byte, pid).unwrap();
-                    }
+                    debug_to_file(&bytes, pid).unwrap();
                 }
                 if !bytes_is_empty {
                     let _ = senders.send_to_screen(ScreenInstruction::PtyBytes(pid, bytes));
@@ -235,7 +233,7 @@ impl Pty {
         }
     }
     pub fn spawn_terminal(&mut self, file_to_open: Option<PathBuf>) -> RawFd {
-        let (pid_primary, pid_secondary): (RawFd, RawFd) = self
+        let (pid_primary, pid_secondary): (RawFd, Pid) = self
             .bus
             .os_input
             .as_mut()
@@ -255,7 +253,7 @@ impl Pty {
         let total_panes = layout.total_terminal_panes();
         let mut new_pane_pids = vec![];
         for _ in 0..total_panes {
-            let (pid_primary, pid_secondary): (RawFd, RawFd) =
+            let (pid_primary, pid_secondary): (RawFd, Pid) =
                 self.bus.os_input.as_mut().unwrap().spawn_terminal(None);
             self.id_to_child_pid.insert(pid_primary, pid_secondary);
             new_pane_pids.push(pid_primary);
diff --git a/zellij-server/src/route.rs b/zellij-server/src/route.rs
index e8ff33762..daa0c0ead 100644
--- a/zellij-server/src/route.rs
+++ b/zellij-server/src/route.rs
@@ -189,7 +189,7 @@ fn route_action(action: Action, session: &SessionMetaData, os_input: &dyn Server
 
 pub(crate) fn route_thread_main(
     sessions: Arc<RwLock<Option<SessionMetaData>>>,
-    mut os_input: Box<dyn ServerOsApi>,
+    os_input: Box<dyn ServerOsApi>,
     to_server: SenderWithContext<ServerInstruction>,
 ) {
     loop {
diff --git a/zellij-server/src/tab.rs b/zellij-server/src/tab.rs
index 69de7e861..78586d7a7 100644
--- a/zellij-server/src/tab.rs
+++ b/zellij-server/src/tab.rs
@@ -232,7 +232,7 @@ impl Tab {
         position: usize,
         name: String,
         full_screen_ws: &PositionAndSize,
-        mut os_api: Box<dyn ServerOsApi>,
+        os_api: Box<dyn ServerOsApi>,
         senders: ThreadSenders,
         max_panes: Option<usize>,
         pane_id: Option<PaneId>,
@@ -624,9 +624,9 @@ impl Tab {
         match pane_id {
             PaneId::Terminal(active_terminal_id) => {
                 let active_terminal = self.panes.get(&pane_id).unwrap();
-                let mut adjusted_input = active_terminal.adjust_input_to_terminal(input_bytes);
+                let adjusted_input = active_terminal.adjust_input_to_terminal(input_bytes);
                 self.os_api
-                    .write_to_tty_stdin(active_terminal_id, &mut adjusted_input)
+                    .write_to_tty_stdin(active_terminal_id, &adjusted_input)
                     .expect("failed to write to terminal");
                 self.os_api
                     .tcdrain(active_terminal_id)
diff --git a/zellij-utils/src/logging.rs b/zellij-utils/src/logging.rs
index d2c57612d..6d415e38f 100644
--- a/zellij-utils/src/logging.rs
+++ b/zellij-utils/src/logging.rs
@@ -71,7 +71,7 @@ pub fn _delete_log_dir() -> io::Result<()> {
     }
 }
 
-pub fn debug_to_file(message: u8, pid: RawFd) -> io::Result<()> {
+pub fn debug_to_file(message: &[u8], pid: RawFd) -> io::Result<()> {
     let mut path = PathBuf::new();
     path.push(&*ZELLIJ_TMP_LOG_DIR);
     path.push(format!("zellij-{}.log", pid.to_string()));
@@ -81,5 +81,5 @@ pub fn debug_to_file(message: u8, pid: RawFd) -> io::Result<()> {
         .create(true)
         .open(&path)?;
     set_permissions(&path)?;
-    file.write_all(&[message])
+    file.write_all(message)
 }