Switch more to File/BorrowedFd from OwnedFd/RawFd

More work in prep for having wopen_cloexec() return `File` directly.

This eliminates checking for an invalid fd and makes both ownership and
mutability clear (some more operations that involve changes to the underlying
state of the fd now require `&mut File` instead of just a `RawFd`).

Code that clearly does not use non-blocking IO is ported to use
`Write::write_all()` directly instead of our rusty port of the `write_loop()`
function (which handles EAGAIN/EWOULDBLOCK in addition to EINTR, while
`write_all()` only handles the latter).

src/autoload.rs

@@ -320,7 +320,7 @@ fn locate_file(&self, cmd: &wstr) -> Option<AutoloadableFile> {
 #[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();

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<OwnedFd> {
+    fn open_and_acquire_lock(&mut self) -> Option<File> {
         // 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<OwnedFd> {
             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<usize>
         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
         }

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);
         }
     }
 }

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 {