diff --git a/src/autoload.rs b/src/autoload.rs index 6d93c17e9..ef952835d 100644 --- a/src/autoload.rs +++ b/src/autoload.rs @@ -320,7 +320,7 @@ fn locate_file(&self, cmd: &wstr) -> Option { #[serial] fn test_autoload() { test_init(); - use crate::common::{charptr2wcstring, wcs2zstring, write_loop}; + use crate::common::{charptr2wcstring, wcs2zstring}; use crate::fds::wopen_cloexec; use crate::wutil::sprintf; use nix::fcntl::OFlag; @@ -335,6 +335,8 @@ macro_rules! run { fn touch_file(path: &wstr) { use nix::sys::stat::Mode; + use std::fs::File; + use std::io::Write; let fd = wopen_cloexec( path, @@ -342,7 +344,8 @@ fn touch_file(path: &wstr) { Mode::from_bits_truncate(0o666), ) .unwrap(); - write_loop(&fd, "Hello".as_bytes()).unwrap(); + let mut file = File::from(fd); + file.write_all(b"Hello").unwrap(); } let mut t1 = "/tmp/fish_test_autoload.XXXXXX\0".as_bytes().to_vec(); diff --git a/src/env_universal_common.rs b/src/env_universal_common.rs index e6113621a..a05fbf474 100644 --- a/src/env_universal_common.rs +++ b/src/env_universal_common.rs @@ -24,7 +24,7 @@ use std::ffi::CString; use std::fs::File; use std::mem::MaybeUninit; -use std::os::fd::{AsFd, AsRawFd, OwnedFd, RawFd}; +use std::os::fd::{AsFd, AsRawFd, RawFd}; use std::os::unix::prelude::MetadataExt; // Pull in the O_EXLOCK constant if it is defined, otherwise set it to 0. @@ -235,14 +235,13 @@ pub fn sync(&mut self, callbacks: &mut CallbackDataList) -> bool { FLOG!(uvar_file, "universal log performing full sync"); // Open the file. - let vars_fd = self.open_and_acquire_lock(); - let Some(vars_fd) = vars_fd else { + let Some(mut vars_file) = self.open_and_acquire_lock() else { FLOG!(uvar_file, "universal log open_and_acquire_lock() failed"); return false; }; // Read from it. - self.load_from_fd(vars_fd.as_raw_fd(), callbacks); + self.load_from_fd(&mut vars_file, callbacks); if self.ok_to_save { self.save(&directory) @@ -391,21 +390,24 @@ fn load_from_path_narrow(&mut self, callbacks: &mut CallbackDataList) -> bool { let Ok(fd) = open_cloexec(&self.narrow_vars_path, OFlag::O_RDONLY, Mode::empty()) else { return false; }; + let mut file = File::from(fd); FLOG!(uvar_file, "universal log reading from file"); - self.load_from_fd(fd.as_raw_fd(), callbacks); + self.load_from_fd(&mut file, callbacks); true } - fn load_from_fd(&mut self, fd: RawFd, callbacks: &mut CallbackDataList) { + // Load environment variables from the opened [`File`] `file`. It must be mutable because we + // will read from the underlying fd. + fn load_from_fd(&mut self, file: &mut File, callbacks: &mut CallbackDataList) { // Get the dev / inode. - let current_file = file_id_for_fd(fd); + let current_file = file_id_for_fd(file.as_fd()); if current_file == self.last_read_file { FLOG!(uvar_file, "universal log sync elided based on fstat()"); } else { // Read a variables table from the file. let mut new_vars = VarTable::new(); - let format = Self::read_message_internal(fd, &mut new_vars); + let format = Self::read_message_internal(file.as_raw_fd(), &mut new_vars); // Hacky: if the read format is in the future, avoid overwriting the file: never try to // save. @@ -423,7 +425,7 @@ fn load_from_fd(&mut self, fd: RawFd, callbacks: &mut CallbackDataList) { } // Functions concerned with saving. - fn open_and_acquire_lock(&mut self) -> Option { + fn open_and_acquire_lock(&mut self) -> Option { // Attempt to open the file for reading at the given path, atomically acquiring a lock. On BSD, // we can use O_EXLOCK. On Linux, we open the file, take a lock, and then compare fstat() to // stat(); if they match, it means that the file was not replaced before we acquired the lock. @@ -438,59 +440,54 @@ fn open_and_acquire_lock(&mut self) -> Option { locked_by_open = true; } - let mut res_fd = None; - while res_fd.is_none() { - let fd = match wopen_cloexec(&self.vars_path, flags, Mode::from_bits_truncate(0o644)) { - Ok(fd) => fd, - Err(err) => { - if err == nix::Error::EINTR { - continue; // signaled; try again - } - - if !O_EXLOCK.is_empty() { - if flags.intersects(O_EXLOCK) - && [nix::Error::ENOTSUP, nix::Error::EOPNOTSUPP].contains(&err) - { - // Filesystem probably does not support locking. Give up on locking. - // Note that on Linux the two errno symbols have the same value but on BSD they're - // different. - flags &= !O_EXLOCK; - self.do_flock = false; - locked_by_open = false; - continue; + loop { + let mut file = + match wopen_cloexec(&self.vars_path, flags, Mode::from_bits_truncate(0o644)) { + Ok(fd) => File::from(fd), + Err(nix::Error::EINTR) => continue, + Err(err) => { + if !O_EXLOCK.is_empty() { + if flags.intersects(O_EXLOCK) + && [nix::Error::ENOTSUP, nix::Error::EOPNOTSUPP].contains(&err) + { + // Filesystem probably does not support locking. Give up on locking. + // Note that on Linux the two errno symbols have the same value but on BSD they're + // different. + flags &= !O_EXLOCK; + self.do_flock = false; + locked_by_open = false; + continue; + } } + FLOG!( + error, + wgettext_fmt!( + "Unable to open universal variable file '%s': %s", + &self.vars_path, + err.to_string() + ) + ); + return None; } - FLOG!( - error, - wgettext_fmt!( - "Unable to open universal variable file '%s': %s", - &self.vars_path, - err.to_string() - ) - ); - break; - } - }; + }; // Lock if we want to lock and open() didn't do it for us. // If flock fails, give up on locking forever. if self.do_flock && !locked_by_open { - if !flock_uvar_file(fd.as_raw_fd()) { + if !flock_uvar_file(&mut file) { self.do_flock = false; } } // Hopefully we got the lock. However, it's possible the file changed out from under us // while we were waiting for the lock. Make sure that didn't happen. - if file_id_for_fd(fd.as_raw_fd()) != file_id_for_path(&self.vars_path) { + if file_id_for_fd(file.as_fd()) != file_id_for_path(&self.vars_path) { // Oops, it changed! Try again. - drop(fd); + drop(file); } else { - res_fd = Some(fd); + return Some(file); } } - - res_fd } fn open_temporary_file( @@ -542,7 +539,7 @@ fn write_to_fd(&mut self, fd: impl AsFd, path: &wstr) -> std::io::Result match res.as_ref() { Ok(_) => { // Since we just wrote out this file, it matches our internal state; pretend we read from it. - self.last_read_file = file_id_for_fd(fd.as_raw_fd()); + self.last_read_file = file_id_for_fd(fd); } Err(err) => { let error = Errno(err.raw_os_error().unwrap()); @@ -1009,9 +1006,9 @@ fn encode_serialized(vals: &[WString]) -> WString { /// Try locking the file. /// \return true on success, false on error. -fn flock_uvar_file(fd: RawFd) -> bool { +fn flock_uvar_file(file: &mut File) -> bool { let start_time = timef(); - while unsafe { libc::flock(fd, LOCK_EX) } == -1 { + while unsafe { libc::flock(file.as_raw_fd(), LOCK_EX) } == -1 { if errno().0 != EINTR { return false; // do nothing per issue #2149 } diff --git a/src/history.rs b/src/history.rs index 2ed1fd9fc..2b3024539 100644 --- a/src/history.rs +++ b/src/history.rs @@ -495,13 +495,13 @@ fn load_old_if_needed(&mut self) { let locked = unsafe { Self::maybe_lock_file(&file, LOCK_SH) }; self.file_contents = HistoryFileContents::create(&mut file); self.history_file_id = if self.file_contents.is_some() { - file_id_for_fd(file.as_raw_fd()) + file_id_for_fd(file.as_fd()) } else { INVALID_FILE_ID }; if locked { unsafe { - Self::unlock_file(file.as_raw_fd()); + Self::unlock_file(&mut file); } } @@ -677,7 +677,7 @@ fn save_internal_via_rewrite(&mut self) { let orig_file_id = target_file_before .as_ref() - .map(|fd| file_id_for_fd(fd.as_raw_fd())) + .map(|fd| file_id_for_fd(fd.as_fd())) .unwrap_or(INVALID_FILE_ID); // Open any target file, but do not lock it right away @@ -804,7 +804,7 @@ fn save_internal_via_appending(&mut self) -> bool { // After locking it, we need to stat the file at the path; if there is a new file there, it // means the file was replaced and we have to try again. // Limit our max tries so we don't do this forever. - let mut history_fd = None; + let mut history_file = None; for _i in 0..MAX_SAVE_TRIES { let Ok(fd) = wopen_cloexec( &history_path, @@ -814,6 +814,7 @@ fn save_internal_via_appending(&mut self) -> bool { // can't open, we're hosed break; }; + let mut file = File::from(fd); // Exclusive lock on the entire file. This is released when we close the file (below). This // may fail on (e.g.) lockless NFS. If so, proceed as if it did not fail; the risk is that @@ -821,21 +822,21 @@ fn save_internal_via_appending(&mut self) -> bool { // forcing everything through the slow copy-move mode. We try to minimize this possibility // by writing with O_APPEND. unsafe { - Self::maybe_lock_file(&fd, LOCK_EX); + Self::maybe_lock_file(&mut file, LOCK_EX); } - let file_id = file_id_for_fd(fd.as_raw_fd()); + let file_id = file_id_for_fd(file.as_fd()); if file_id_for_path(&history_path) == file_id { // File IDs match, so the file we opened is still at that path // We're going to use this fd if file_id != self.history_file_id { file_changed = true; } - history_fd = Some(fd); + history_file = Some(file); break; } } - if let Some(history_fd) = history_fd { + if let Some(history_file) = history_file { // We (hopefully successfully) took the exclusive lock. Append to the file. // Note that this is sketchy for a few reasons: // - Another shell may have appended its own items with a later timestamp, so our file may @@ -866,7 +867,7 @@ fn save_internal_via_appending(&mut self) -> bool { append_history_item_to_buffer(item, &mut buffer); res = flush_to_fd( &mut buffer, - history_fd.as_raw_fd(), + history_file.as_raw_fd(), HISTORY_OUTPUT_BUFFER_SIZE, ); if res.is_err() { @@ -878,7 +879,7 @@ fn save_internal_via_appending(&mut self) -> bool { } if res.is_ok() { - res = flush_to_fd(&mut buffer, history_fd.as_raw_fd(), 0); + res = flush_to_fd(&mut buffer, history_file.as_raw_fd(), 0); } // Since we just modified the file, update our history_file_id to match its current state @@ -886,11 +887,11 @@ fn save_internal_via_appending(&mut self) -> bool { // write. // We don't update the mapping since we only appended to the file, and everything we // appended remains in our new_items - self.history_file_id = file_id_for_fd(history_fd.as_raw_fd()); + self.history_file_id = file_id_for_fd(history_file.as_fd()); ok = res.is_ok(); - drop(history_fd); + drop(history_file); } // If someone has replaced the file, forget our file state. @@ -1355,9 +1356,9 @@ unsafe fn maybe_lock_file(fd: impl AsFd, lock_type: libc::c_int) -> bool { /// # Safety /// /// `fd` must be a valid argument to `flock(2)` with `LOCK_UN`. - unsafe fn unlock_file(fd: RawFd) { + unsafe fn unlock_file(file: &mut File) { unsafe { - libc::flock(fd, LOCK_UN); + libc::flock(file.as_raw_fd(), LOCK_UN); } } } diff --git a/src/wutil/mod.rs b/src/wutil/mod.rs index 89c1402bd..114352dda 100644 --- a/src/wutil/mod.rs +++ b/src/wutil/mod.rs @@ -561,17 +561,17 @@ pub fn older_than(&self, rhs: &FileId) -> bool { pub const INVALID_FILE_ID: FileId = FileId::new(); -pub fn file_id_for_fd(fd: RawFd) -> FileId { +pub fn file_id_for_fd(fd: BorrowedFd<'_>) -> FileId { let mut result = INVALID_FILE_ID; let mut buf: libc::stat = unsafe { std::mem::zeroed() }; - if fd >= 0 && unsafe { libc::fstat(fd, &mut buf) } == 0 { + if unsafe { libc::fstat(fd.as_raw_fd(), &mut buf) } == 0 { result = FileId::from_stat(&buf); } result } pub fn file_id_for_autoclose_fd(fd: &AutoCloseFd) -> FileId { - file_id_for_fd(fd.fd()) + file_id_for_fd(fd.as_fd()) } pub fn file_id_for_path(path: &wstr) -> FileId {