From f3e8272c5dab769b0e1afdea120a6641f9e74685 Mon Sep 17 00:00:00 2001 From: PolyMeilex Date: Sat, 20 Jan 2024 18:45:36 +0100 Subject: [PATCH] Move from libc read/write to nix read/write Replace std from_raw_fd/into_raw_fd dance with nix write Fixup notifyd build --- src/builtins/read.rs | 22 +++++++------ src/common.rs | 41 +++++++++++++----------- src/fd_monitor.rs | 25 ++++++++------- src/fds.rs | 14 ++------- src/fork_exec/flog_safe.rs | 4 +-- src/history/file.rs | 33 ++++++++------------ src/input_common.rs | 2 +- src/reader.rs | 52 +++++++++++++++++-------------- src/tests/fd_monitor.rs | 5 ++- src/universal_notifier/notifyd.rs | 20 ++++++------ src/wutil/mod.rs | 8 ++--- 11 files changed, 107 insertions(+), 119 deletions(-) diff --git a/src/builtins/read.rs b/src/builtins/read.rs index 2372a63db..e7467287f 100644 --- a/src/builtins/read.rs +++ b/src/builtins/read.rs @@ -285,13 +285,14 @@ fn read_in_chunks(fd: RawFd, buff: &mut WString, split_null: bool, do_seek: bool while !finished { let mut inbuf = [0_u8; READ_CHUNK_SIZE]; - let bytes_read = read_blocked(fd, &mut inbuf); - if bytes_read <= 0 { - eof = true; - break; - } - let bytes_read = bytes_read as usize; + let bytes_read = match read_blocked(fd, &mut inbuf) { + Ok(0) | Err(_) => { + eof = true; + break; + } + Ok(read) => read, + }; let bytes_consumed = inbuf[..bytes_read] .iter() @@ -352,9 +353,12 @@ fn read_one_char_at_a_time( while !finished { let mut b = [0_u8; 1]; - if read_blocked(fd, &mut b) <= 0 { - eof = true; - break; + match read_blocked(fd, &mut b) { + Ok(0) | Err(_) => { + eof = true; + break; + } + _ => {} } let b = b[0]; diff --git a/src/common.rs b/src/common.rs index 2c2c99d77..433508ed4 100644 --- a/src/common.rs +++ b/src/common.rs @@ -18,7 +18,7 @@ use crate::wutil::encoding::{mbrtowc, wcrtomb, zero_mbstate, AT_LEAST_MB_LEN_MAX use crate::wutil::fish_iswalnum; use bitflags::bitflags; use core::slice; -use libc::{EINTR, EIO, O_WRONLY, SIGTTOU, SIG_IGN, STDERR_FILENO, STDIN_FILENO, STDOUT_FILENO}; +use libc::{EIO, O_WRONLY, SIGTTOU, SIG_IGN, STDERR_FILENO, STDIN_FILENO, STDOUT_FILENO}; use num_traits::ToPrimitive; use once_cell::sync::OnceCell; use std::env; @@ -1469,10 +1469,10 @@ fn can_be_encoded(wc: char) -> bool { /// Call read, blocking and repeating on EINTR. Exits on EAGAIN. /// \return the number of bytes read, or 0 on EOF. On EAGAIN, returns -1 if nothing was read. -pub fn read_blocked(fd: RawFd, buf: &mut [u8]) -> isize { +pub fn read_blocked(fd: RawFd, buf: &mut [u8]) -> nix::Result { loop { - let res = unsafe { libc::read(fd, buf.as_mut_ptr().cast(), buf.len()) }; - if res < 0 && errno::errno().0 == EINTR { + let res = nix::unistd::read(fd, buf); + if let Err(nix::Error::EINTR) = res { continue; } return res; @@ -1496,16 +1496,17 @@ pub fn write_loop(fd: &Fd, buf: &[u8]) -> std::io::Result { let fd = fd.as_raw_fd(); let mut total = 0; while total < buf.len() { - let written = - unsafe { libc::write(fd, buf[total..].as_ptr() as *const _, buf.len() - total) }; - if written < 0 { - let errno = errno::errno().0; - if matches!(errno, libc::EAGAIN | libc::EINTR) { - continue; + match nix::unistd::write(fd, &buf[total..]) { + Ok(written) => { + total += written; + } + Err(err) => { + if matches!(err, nix::Error::EAGAIN | nix::Error::EINTR) { + continue; + } + return Err(std::io::Error::from(err)); } - return Err(std::io::Error::from_raw_os_error(errno)); } - total += written as usize; } Ok(total) } @@ -1517,15 +1518,17 @@ pub fn write_loop(fd: &Fd, buf: &[u8]) -> std::io::Result { pub fn read_loop(fd: &Fd, buf: &mut [u8]) -> std::io::Result { let fd = fd.as_raw_fd(); loop { - let read = unsafe { libc::read(fd, buf.as_mut_ptr() as *mut _, buf.len()) }; - if read < 0 { - let errno = errno::errno().0; - if matches!(errno, libc::EAGAIN | libc::EINTR) { - continue; + match nix::unistd::read(fd, buf) { + Ok(read) => { + return Ok(read); + } + Err(err) => { + if matches!(err, nix::Error::EAGAIN | nix::Error::EINTR) { + continue; + } + return Err(std::io::Error::from(err)); } - return Err(std::io::Error::from_raw_os_error(errno)); } - return Ok(read as usize); } } diff --git a/src/fd_monitor.rs b/src/fd_monitor.rs index 698974029..fe5a35861 100644 --- a/src/fd_monitor.rs +++ b/src/fd_monitor.rs @@ -120,20 +120,21 @@ impl FdEventSignaller { let c = 1_u8; let mut ret; loop { - ret = unsafe { - libc::write( - self.write_fd(), - &c as *const _ as *const c_void, - std::mem::size_of_val(&c), - ) - }; - if ret >= 0 || errno().0 != EINTR { - break; + let bytes = c.to_ne_bytes(); + ret = nix::unistd::write(self.write_fd(), &bytes); + + match ret { + Ok(_) => break, + Err(nix::Error::EINTR) => continue, + Err(_) => break, } } - // EAGAIN occurs if either the pipe buffer is full or the eventfd overflows (very unlikely). - if ret < 0 && ![EAGAIN, EWOULDBLOCK].contains(&errno().0) { - perror("write"); + + if let Err(err) = ret { + // EAGAIN occurs if either the pipe buffer is full or the eventfd overflows (very unlikely). + if ![nix::Error::EAGAIN, nix::Error::EWOULDBLOCK].contains(&err) { + perror("write"); + } } } diff --git a/src/fds.rs b/src/fds.rs index b343e76bd..ea4a742da 100644 --- a/src/fds.rs +++ b/src/fds.rs @@ -33,23 +33,13 @@ pub struct AutoCloseFd { impl Read for AutoCloseFd { fn read(&mut self, buf: &mut [u8]) -> std::io::Result { - unsafe { - match libc::read(self.as_raw_fd(), buf.as_mut_ptr() as *mut _, buf.len()) { - -1 => Err(std::io::Error::from_raw_os_error(errno::errno().0)), - bytes => Ok(bytes as usize), - } - } + nix::unistd::read(self.as_raw_fd(), buf).map_err(std::io::Error::from) } } impl Write for AutoCloseFd { fn write(&mut self, buf: &[u8]) -> std::io::Result { - unsafe { - match libc::write(self.as_raw_fd(), buf.as_ptr() as *const _, buf.len()) { - -1 => Err(std::io::Error::from_raw_os_error(errno::errno().0)), - bytes => Ok(bytes as usize), - } - } + nix::unistd::write(self.as_raw_fd(), buf).map_err(std::io::Error::from) } fn flush(&mut self) -> std::io::Result<()> { diff --git a/src/fork_exec/flog_safe.rs b/src/fork_exec/flog_safe.rs index d1c0a922b..164db4f16 100644 --- a/src/fork_exec/flog_safe.rs +++ b/src/fork_exec/flog_safe.rs @@ -84,9 +84,7 @@ pub fn flog_impl_async_safe(fd: i32, s: impl FloggableDisplayAsyncSafe) { let bytes: &[u8] = s.to_flog_str_async_safe(&mut storage); // Note we deliberately do not retry on signals, etc. // This is used to report error messages after fork() in the child process. - unsafe { - let _ = libc::write(fd, bytes.as_ptr() as *const libc::c_void, bytes.len()); - } + let _ = nix::unistd::write(fd, bytes); } /// Variant of FLOG which is async-safe to use after fork(). diff --git a/src/history/file.rs b/src/history/file.rs index c1a839c1b..887ea92ea 100644 --- a/src/history/file.rs +++ b/src/history/file.rs @@ -7,10 +7,9 @@ use std::{ time::{Duration, SystemTime, UNIX_EPOCH}, }; -use errno::errno; use libc::{ - c_void, lseek, mmap, munmap, EINTR, MAP_ANONYMOUS, MAP_FAILED, MAP_PRIVATE, PROT_READ, - PROT_WRITE, SEEK_END, SEEK_SET, + lseek, mmap, munmap, MAP_ANONYMOUS, MAP_FAILED, MAP_PRIVATE, PROT_READ, PROT_WRITE, SEEK_END, + SEEK_SET, }; use super::{HistoryItem, PersistenceMode}; @@ -137,7 +136,7 @@ impl HistoryFileContents { if unsafe { lseek(fd, 0, SEEK_SET) } != 0 { return None; } - if !read_from_fd(fd, region.as_mut()) { + if read_from_fd(fd, region.as_mut()).is_err() { return None; } } @@ -230,25 +229,19 @@ fn should_mmap() -> bool { /// Read from `fd` to fill `dest`, zeroing any unused space. // Return true on success, false on failure. -fn read_from_fd(fd: RawFd, dest: &mut [u8]) -> bool { - let mut remaining = dest.len(); - let mut nread = 0; - while remaining > 0 { - let amt = - unsafe { libc::read(fd, (&mut dest[nread]) as *mut u8 as *mut c_void, remaining) }; - if amt < 0 { - if errno().0 != EINTR { - return false; +fn read_from_fd(fd: RawFd, mut dest: &mut [u8]) -> nix::Result<()> { + while !dest.is_empty() { + match nix::unistd::read(fd, dest) { + Ok(0) => break, + Ok(amt) => { + dest = &mut dest[amt..]; } - } else if amt == 0 { - break; - } else { - remaining -= amt as usize; - nread += amt as usize; + Err(nix::Error::EINTR) => continue, + Err(err) => return Err(err), } } - dest[nread..].fill(0u8); - true + dest.fill(0u8); + Ok(()) } fn replace_all(s: &mut Vec, needle: &[u8], replacement: &[u8]) { diff --git a/src/input_common.rs b/src/input_common.rs index 3a5b15f4d..c41ff6c38 100644 --- a/src/input_common.rs +++ b/src/input_common.rs @@ -294,7 +294,7 @@ fn readb(in_fd: RawFd) -> ReadbResult { // Check stdin. if fdset.test(in_fd) { let mut arr: [u8; 1] = [0]; - if read_blocked(in_fd, &mut arr) != 1 { + if read_blocked(in_fd, &mut arr) != Ok(1) { // The terminal has been closed. return ReadbResult::Eof; } diff --git a/src/reader.rs b/src/reader.rs index 8277d2076..ad3a46d38 100644 --- a/src/reader.rs +++ b/src/reader.rs @@ -13,9 +13,9 @@ //! end of the list is reached, at which point regular searching will commence. use libc::{ - c_char, c_int, c_void, EAGAIN, ECHO, EINTR, EIO, EISDIR, ENOTTY, EPERM, ESRCH, EWOULDBLOCK, - ICANON, ICRNL, IEXTEN, INLCR, IXOFF, IXON, ONLCR, OPOST, O_NONBLOCK, O_RDONLY, SIGINT, SIGTTIN, - STDIN_FILENO, STDOUT_FILENO, S_IFDIR, TCSANOW, VMIN, VQUIT, VSUSP, VTIME, _POSIX_VDISABLE, + c_char, c_int, ECHO, EINTR, EIO, EISDIR, ENOTTY, EPERM, ESRCH, ICANON, ICRNL, IEXTEN, INLCR, + IXOFF, IXON, ONLCR, OPOST, O_NONBLOCK, O_RDONLY, SIGINT, SIGTTIN, STDIN_FILENO, STDOUT_FILENO, + S_IFDIR, TCSANOW, VMIN, VQUIT, VSUSP, VTIME, _POSIX_VDISABLE, }; use once_cell::sync::Lazy; use std::cell::UnsafeCell; @@ -688,27 +688,31 @@ fn read_ni(parser: &Parser, fd: RawFd, io: &IoChain) -> i32 { let mut fd_contents = Vec::with_capacity(usize::try_from(buf.st_size).unwrap()); loop { let mut buff = [0_u8; 4096]; - let amt = unsafe { libc::read(fd, &mut buff[0] as *mut _ as *mut c_void, buff.len()) }; - if amt > 0 { - fd_contents.extend_from_slice(&buff[..usize::try_from(amt).unwrap()]); - } else if amt == 0 { - // EOF. - break; - } else { - assert!(amt == -1); - let err = errno(); - if err.0 == EINTR { - continue; - } else if err.0 == EAGAIN || err.0 == EWOULDBLOCK && make_fd_blocking(fd).is_ok() { - // We succeeded in making the fd blocking, keep going. - continue; - } else { - // Fatal error. - FLOG!( - error, - wgettext_fmt!("Unable to read input file: %s", err.to_string()) - ); - return 1; + + match nix::unistd::read(fd, &mut buff) { + Ok(0) => { + // EOF. + break; + } + Ok(amt) => { + fd_contents.extend_from_slice(&buff[..amt]); + } + Err(err) => { + if err == nix::Error::EINTR { + continue; + } else if err == nix::Error::EAGAIN + || err == nix::Error::EWOULDBLOCK && make_fd_blocking(fd).is_ok() + { + // We succeeded in making the fd blocking, keep going. + continue; + } else { + // Fatal error. + FLOG!( + error, + wgettext_fmt!("Unable to read input file: %s", err.to_string()) + ); + return 1; + } } } } diff --git a/src/tests/fd_monitor.rs b/src/tests/fd_monitor.rs index 8712a955f..ad065b49a 100644 --- a/src/tests/fd_monitor.rs +++ b/src/tests/fd_monitor.rs @@ -76,9 +76,8 @@ impl ItemMaker { } ItemWakeReason::Readable => { let mut buf = [0u8; 1024]; - let amt = - unsafe { libc::read(fd.as_raw_fd(), buf.as_mut_ptr() as *mut _, buf.len()) }; - assert_ne!(amt, -1, "read error!"); + let res = nix::unistd::read(fd.as_raw_fd(), &mut buf); + let amt = res.expect("read error!"); self.length_read.fetch_add(amt as usize, Ordering::Relaxed); was_closed = amt == 0; } diff --git a/src/universal_notifier/notifyd.rs b/src/universal_notifier/notifyd.rs index 063ee2c66..f56ae0c3f 100644 --- a/src/universal_notifier/notifyd.rs +++ b/src/universal_notifier/notifyd.rs @@ -114,16 +114,16 @@ impl UniversalNotifier for NotifydNotifier { let mut read_something = false; let mut buff: [u8; 64] = [0; 64]; loop { - let amt_read = unsafe { - libc::read( - self.notify_fd, - buff.as_mut_ptr() as *mut libc::c_void, - buff.len(), - ) - }; - read_something = read_something || amt_read > 0; - if amt_read != buff.len() as isize { - break; + let res = nix::unistd::read(self.notify_fd, &mut buff); + + if let Ok(amt_read) = res { + read_something |= amt_read > 0; + } + + match res { + Ok(amt_read) if amt_read != buff.len() => break, + Err(_) => break, + _ => continue, } } FLOGF!( diff --git a/src/wutil/mod.rs b/src/wutil/mod.rs index 040c9a301..8e4c89424 100644 --- a/src/wutil/mod.rs +++ b/src/wutil/mod.rs @@ -401,12 +401,8 @@ pub fn wrename(old_name: &wstr, new_name: &wstr) -> libc::c_int { unsafe { libc::rename(old_narrow.as_ptr(), new_narrow.as_ptr()) } } -pub fn write_to_fd(input: &[u8], fd: RawFd) -> std::io::Result { - let mut file = unsafe { std::fs::File::from_raw_fd(fd) }; - let amt = file.write(input); - // Ensure the file is not closed. - file.into_raw_fd(); - amt +pub fn write_to_fd(input: &[u8], fd: RawFd) -> nix::Result { + nix::unistd::write(fd, input) } /// Write a wide string to a file descriptor. This avoids doing any additional allocation.