Rollup merge of #138832 - ChrisDenton:with_native_path, r=joboet

Start using `with_native_path` in `std::sys::fs`

Ideally, each platform should use their own native path type internally. This will, for example, allow passing a `CStr` directly to `std::fs::File::open` and therefore avoid the need for allocating a new null-terminated C string.

However, doing that for every function and platform all at once makes for a large PR that is way too prone to breaking. So this PR does some minimal refactoring which should help progress towards that goal. The changes are Unix-only and even then I avoided functions that require more changes so that this PR is just moving things around.

r? joboet

3 files changed, 181 insertions, 115 deletions

diff --git a/library/std/src/fs.rs b/library/std/src/fs.rs
index fa98db69306..801baf3d990 100644
--- a/library/std/src/fs.rs
+++ b/library/std/src/fs.rs
@@ -2370,7 +2370,7 @@ impl AsInner<fs_imp::DirEntry> for DirEntry {
 #[doc(alias = "rm", alias = "unlink", alias = "DeleteFile")]
 #[stable(feature = "rust1", since = "1.0.0")]
 pub fn remove_file<P: AsRef<Path>>(path: P) -> io::Result<()> {
-    fs_imp::unlink(path.as_ref())
+    fs_imp::remove_file(path.as_ref())
 }
 
 /// Given a path, queries the file system to get information about a file,
@@ -2409,7 +2409,7 @@ pub fn remove_file<P: AsRef<Path>>(path: P) -> io::Result<()> {
 #[doc(alias = "stat")]
 #[stable(feature = "rust1", since = "1.0.0")]
 pub fn metadata<P: AsRef<Path>>(path: P) -> io::Result<Metadata> {
-    fs_imp::stat(path.as_ref()).map(Metadata)
+    fs_imp::metadata(path.as_ref()).map(Metadata)
 }
 
 /// Queries the metadata about a file without following symlinks.
@@ -2444,7 +2444,7 @@ pub fn metadata<P: AsRef<Path>>(path: P) -> io::Result<Metadata> {
 #[doc(alias = "lstat")]
 #[stable(feature = "symlink_metadata", since = "1.1.0")]
 pub fn symlink_metadata<P: AsRef<Path>>(path: P) -> io::Result<Metadata> {
-    fs_imp::lstat(path.as_ref()).map(Metadata)
+    fs_imp::symlink_metadata(path.as_ref()).map(Metadata)
 }
 
 /// Renames a file or directory to a new name, replacing the original file if
@@ -2598,7 +2598,7 @@ pub fn copy<P: AsRef<Path>, Q: AsRef<Path>>(from: P, to: Q) -> io::Result<u64> {
 #[doc(alias = "CreateHardLink", alias = "linkat")]
 #[stable(feature = "rust1", since = "1.0.0")]
 pub fn hard_link<P: AsRef<Path>, Q: AsRef<Path>>(original: P, link: Q) -> io::Result<()> {
-    fs_imp::link(original.as_ref(), link.as_ref())
+    fs_imp::hard_link(original.as_ref(), link.as_ref())
 }
 
 /// Creates a new symbolic link on the filesystem.
@@ -2664,7 +2664,7 @@ pub fn soft_link<P: AsRef<Path>, Q: AsRef<Path>>(original: P, link: Q) -> io::Re
 /// ```
 #[stable(feature = "rust1", since = "1.0.0")]
 pub fn read_link<P: AsRef<Path>>(path: P) -> io::Result<PathBuf> {
-    fs_imp::readlink(path.as_ref())
+    fs_imp::read_link(path.as_ref())
 }
 
 /// Returns the canonical, absolute form of a path with all intermediate
@@ -2840,7 +2840,7 @@ pub fn create_dir_all<P: AsRef<Path>>(path: P) -> io::Result<()> {
 #[doc(alias = "rmdir", alias = "RemoveDirectory")]
 #[stable(feature = "rust1", since = "1.0.0")]
 pub fn remove_dir<P: AsRef<Path>>(path: P) -> io::Result<()> {
-    fs_imp::rmdir(path.as_ref())
+    fs_imp::remove_dir(path.as_ref())
 }
 
 /// Removes a directory at this path, after removing all its contents. Use
@@ -2967,7 +2967,7 @@ pub fn remove_dir_all<P: AsRef<Path>>(path: P) -> io::Result<()> {
 #[doc(alias = "ls", alias = "opendir", alias = "FindFirstFile", alias = "FindNextFile")]
 #[stable(feature = "rust1", since = "1.0.0")]
 pub fn read_dir<P: AsRef<Path>>(path: P) -> io::Result<ReadDir> {
-    fs_imp::readdir(path.as_ref()).map(ReadDir)
+    fs_imp::read_dir(path.as_ref()).map(ReadDir)
 }
 
 /// Changes the permissions found on a file or a directory.
@@ -3003,7 +3003,7 @@ pub fn read_dir<P: AsRef<Path>>(path: P) -> io::Result<ReadDir> {
 #[doc(alias = "chmod", alias = "SetFileAttributes")]
 #[stable(feature = "set_permissions", since = "1.1.0")]
 pub fn set_permissions<P: AsRef<Path>>(path: P, perm: Permissions) -> io::Result<()> {
-    fs_imp::set_perm(path.as_ref(), perm.0)
+    fs_imp::set_permissions(path.as_ref(), perm.0)
 }
 
 impl DirBuilder {
diff --git a/library/std/src/sys/fs/mod.rs b/library/std/src/sys/fs/mod.rs
index c2e19eb393a..3b176d0d16c 100644
--- a/library/std/src/sys/fs/mod.rs
+++ b/library/std/src/sys/fs/mod.rs
@@ -1,28 +1,115 @@
 #![deny(unsafe_op_in_unsafe_fn)]
 
+use crate::io;
+use crate::path::{Path, PathBuf};
+
 pub mod common;
 
 cfg_if::cfg_if! {
     if #[cfg(target_family = "unix")] {
         mod unix;
-        pub use unix::*;
+        use unix as imp;
+        pub use unix::{chown, fchown, lchown};
+        #[cfg(not(target_os = "fuchsia"))]
+        pub use unix::chroot;
+        pub(crate) use unix::debug_assert_fd_is_open;
+        #[cfg(any(target_os = "linux", target_os = "android"))]
+        pub(crate) use unix::CachedFileMetadata;
+        use crate::sys::common::small_c_string::run_path_with_cstr as with_native_path;
     } else if #[cfg(target_os = "windows")] {
         mod windows;
-        pub use windows::*;
+        use windows as imp;
+        pub use windows::{symlink_inner, junction_point};
     } else if #[cfg(target_os = "hermit")] {
         mod hermit;
-        pub use hermit::*;
+        use hermit as imp;
     } else if #[cfg(target_os = "solid_asp3")] {
         mod solid;
-        pub use solid::*;
+        use solid as imp;
     } else if #[cfg(target_os = "uefi")] {
         mod uefi;
-        pub use uefi::*;
+        use uefi as imp;
     } else if #[cfg(target_os = "wasi")] {
         mod wasi;
-        pub use wasi::*;
+        use wasi as imp;
     } else {
         mod unsupported;
-        pub use unsupported::*;
+        use unsupported as imp;
     }
 }
+
+// FIXME: Replace this with platform-specific path conversion functions.
+#[cfg(not(target_family = "unix"))]
+#[inline]
+pub fn with_native_path<T>(path: &Path, f: &dyn Fn(&Path) -> io::Result<T>) -> io::Result<T> {
+    f(path)
+}
+
+pub use imp::{
+    DirBuilder, DirEntry, File, FileAttr, FilePermissions, FileTimes, FileType, OpenOptions,
+    ReadDir,
+};
+
+pub fn read_dir(path: &Path) -> io::Result<ReadDir> {
+    // FIXME: use with_native_path
+    imp::readdir(path)
+}
+
+pub fn remove_file(path: &Path) -> io::Result<()> {
+    with_native_path(path, &imp::unlink)
+}
+
+pub fn rename(old: &Path, new: &Path) -> io::Result<()> {
+    with_native_path(old, &|old| with_native_path(new, &|new| imp::rename(old, new)))
+}
+
+pub fn remove_dir(path: &Path) -> io::Result<()> {
+    with_native_path(path, &imp::rmdir)
+}
+
+pub fn remove_dir_all(path: &Path) -> io::Result<()> {
+    // FIXME: use with_native_path
+    imp::remove_dir_all(path)
+}
+
+pub fn read_link(path: &Path) -> io::Result<PathBuf> {
+    with_native_path(path, &imp::readlink)
+}
+
+pub fn symlink(original: &Path, link: &Path) -> io::Result<()> {
+    with_native_path(original, &|original| {
+        with_native_path(link, &|link| imp::symlink(original, link))
+    })
+}
+
+pub fn hard_link(original: &Path, link: &Path) -> io::Result<()> {
+    with_native_path(original, &|original| {
+        with_native_path(link, &|link| imp::link(original, link))
+    })
+}
+
+pub fn metadata(path: &Path) -> io::Result<FileAttr> {
+    with_native_path(path, &imp::stat)
+}
+
+pub fn symlink_metadata(path: &Path) -> io::Result<FileAttr> {
+    with_native_path(path, &imp::lstat)
+}
+
+pub fn set_permissions(path: &Path, perm: FilePermissions) -> io::Result<()> {
+    with_native_path(path, &|path| imp::set_perm(path, perm.clone()))
+}
+
+pub fn canonicalize(path: &Path) -> io::Result<PathBuf> {
+    with_native_path(path, &imp::canonicalize)
+}
+
+pub fn copy(from: &Path, to: &Path) -> io::Result<u64> {
+    // FIXME: use with_native_path
+    imp::copy(from, to)
+}
+
+pub fn exists(path: &Path) -> io::Result<bool> {
+    // FIXME: use with_native_path
+    imp::exists(path)
+}
diff --git a/library/std/src/sys/fs/unix.rs b/library/std/src/sys/fs/unix.rs
index 60e6559fa6b..87865be0387 100644
--- a/library/std/src/sys/fs/unix.rs
+++ b/library/std/src/sys/fs/unix.rs
@@ -926,7 +926,7 @@ impl DirEntry {
         miri
     ))]
     pub fn metadata(&self) -> io::Result<FileAttr> {
-        lstat(&self.path())
+        run_path_with_cstr(&self.path(), &lstat)
     }
 
     #[cfg(any(
@@ -1657,7 +1657,7 @@ impl fmt::Debug for File {
         fn get_path(fd: c_int) -> Option<PathBuf> {
             let mut p = PathBuf::from("/proc/self/fd");
             p.push(&fd.to_string());
-            readlink(&p).ok()
+            run_path_with_cstr(&p, &readlink).ok()
         }
 
         #[cfg(any(target_vendor = "apple", target_os = "netbsd"))]
@@ -1675,7 +1675,7 @@ impl fmt::Debug for File {
                         // fallback to procfs as last resort
                         let mut p = PathBuf::from("/proc/self/fd");
                         p.push(&fd.to_string());
-                        return readlink(&p).ok();
+                        return run_path_with_cstr(&p, &readlink).ok()
                     } else {
                         return None;
                     }
@@ -1830,127 +1830,106 @@ pub fn readdir(path: &Path) -> io::Result<ReadDir> {
     }
 }
 
-pub fn unlink(p: &Path) -> io::Result<()> {
-    run_path_with_cstr(p, &|p| cvt(unsafe { libc::unlink(p.as_ptr()) }).map(|_| ()))
+pub fn unlink(p: &CStr) -> io::Result<()> {
+    cvt(unsafe { libc::unlink(p.as_ptr()) }).map(|_| ())
 }
 
-pub fn rename(old: &Path, new: &Path) -> io::Result<()> {
-    run_path_with_cstr(old, &|old| {
-        run_path_with_cstr(new, &|new| {
-            cvt(unsafe { libc::rename(old.as_ptr(), new.as_ptr()) }).map(|_| ())
-        })
-    })
+pub fn rename(old: &CStr, new: &CStr) -> io::Result<()> {
+    cvt(unsafe { libc::rename(old.as_ptr(), new.as_ptr()) }).map(|_| ())
 }
 
-pub fn set_perm(p: &Path, perm: FilePermissions) -> io::Result<()> {
-    run_path_with_cstr(p, &|p| cvt_r(|| unsafe { libc::chmod(p.as_ptr(), perm.mode) }).map(|_| ()))
+pub fn set_perm(p: &CStr, perm: FilePermissions) -> io::Result<()> {
+    cvt_r(|| unsafe { libc::chmod(p.as_ptr(), perm.mode) }).map(|_| ())
 }
 
-pub fn rmdir(p: &Path) -> io::Result<()> {
-    run_path_with_cstr(p, &|p| cvt(unsafe { libc::rmdir(p.as_ptr()) }).map(|_| ()))
+pub fn rmdir(p: &CStr) -> io::Result<()> {
+    cvt(unsafe { libc::rmdir(p.as_ptr()) }).map(|_| ())
 }
 
-pub fn readlink(p: &Path) -> io::Result<PathBuf> {
-    run_path_with_cstr(p, &|c_path| {
-        let p = c_path.as_ptr();
-
-        let mut buf = Vec::with_capacity(256);
+pub fn readlink(c_path: &CStr) -> io::Result<PathBuf> {
+    let p = c_path.as_ptr();
 
-        loop {
-            let buf_read =
-                cvt(unsafe { libc::readlink(p, buf.as_mut_ptr() as *mut _, buf.capacity()) })?
-                    as usize;
+    let mut buf = Vec::with_capacity(256);
 
-            unsafe {
-                buf.set_len(buf_read);
-            }
+    loop {
+        let buf_read =
+            cvt(unsafe { libc::readlink(p, buf.as_mut_ptr() as *mut _, buf.capacity()) })? as usize;
 
-            if buf_read != buf.capacity() {
-                buf.shrink_to_fit();
+        unsafe {
+            buf.set_len(buf_read);
+        }
 
-                return Ok(PathBuf::from(OsString::from_vec(buf)));
-            }
+        if buf_read != buf.capacity() {
+            buf.shrink_to_fit();
 
-            // Trigger the internal buffer resizing logic of `Vec` by requiring
-            // more space than the current capacity. The length is guaranteed to be
-            // the same as the capacity due to the if statement above.
-            buf.reserve(1);
+            return Ok(PathBuf::from(OsString::from_vec(buf)));
         }
-    })
+
+        // Trigger the internal buffer resizing logic of `Vec` by requiring
+        // more space than the current capacity. The length is guaranteed to be
+        // the same as the capacity due to the if statement above.
+        buf.reserve(1);
+    }
 }
 
-pub fn symlink(original: &Path, link: &Path) -> io::Result<()> {
-    run_path_with_cstr(original, &|original| {
-        run_path_with_cstr(link, &|link| {
-            cvt(unsafe { libc::symlink(original.as_ptr(), link.as_ptr()) }).map(|_| ())
-        })
-    })
+pub fn symlink(original: &CStr, link: &CStr) -> io::Result<()> {
+    cvt(unsafe { libc::symlink(original.as_ptr(), link.as_ptr()) }).map(|_| ())
 }
 
-pub fn link(original: &Path, link: &Path) -> io::Result<()> {
-    run_path_with_cstr(original, &|original| {
-        run_path_with_cstr(link, &|link| {
-            cfg_if::cfg_if! {
-                if #[cfg(any(target_os = "vxworks", target_os = "redox", target_os = "android", target_os = "espidf", target_os = "horizon", target_os = "vita", target_env = "nto70"))] {
-                    // VxWorks, Redox and ESP-IDF lack `linkat`, so use `link` instead. POSIX leaves
-                    // it implementation-defined whether `link` follows symlinks, so rely on the
-                    // `symlink_hard_link` test in library/std/src/fs/tests.rs to check the behavior.
-                    // Android has `linkat` on newer versions, but we happen to know `link`
-                    // always has the correct behavior, so it's here as well.
-                    cvt(unsafe { libc::link(original.as_ptr(), link.as_ptr()) })?;
-                } else {
-                    // Where we can, use `linkat` instead of `link`; see the comment above
-                    // this one for details on why.
-                    cvt(unsafe { libc::linkat(libc::AT_FDCWD, original.as_ptr(), libc::AT_FDCWD, link.as_ptr(), 0) })?;
-                }
-            }
-            Ok(())
-        })
-    })
+pub fn link(original: &CStr, link: &CStr) -> io::Result<()> {
+    cfg_if::cfg_if! {
+        if #[cfg(any(target_os = "vxworks", target_os = "redox", target_os = "android", target_os = "espidf", target_os = "horizon", target_os = "vita", target_env = "nto70"))] {
+            // VxWorks, Redox and ESP-IDF lack `linkat`, so use `link` instead. POSIX leaves
+            // it implementation-defined whether `link` follows symlinks, so rely on the
+            // `symlink_hard_link` test in library/std/src/fs/tests.rs to check the behavior.
+            // Android has `linkat` on newer versions, but we happen to know `link`
+            // always has the correct behavior, so it's here as well.
+            cvt(unsafe { libc::link(original.as_ptr(), link.as_ptr()) })?;
+        } else {
+            // Where we can, use `linkat` instead of `link`; see the comment above
+            // this one for details on why.
+            cvt(unsafe { libc::linkat(libc::AT_FDCWD, original.as_ptr(), libc::AT_FDCWD, link.as_ptr(), 0) })?;
+        }
+    }
+    Ok(())
 }
 
-pub fn stat(p: &Path) -> io::Result<FileAttr> {
-    run_path_with_cstr(p, &|p| {
-        cfg_has_statx! {
-            if let Some(ret) = unsafe { try_statx(
-                libc::AT_FDCWD,
-                p.as_ptr(),
-                libc::AT_STATX_SYNC_AS_STAT,
-                libc::STATX_BASIC_STATS | libc::STATX_BTIME,
-            ) } {
-                return ret;
-            }
+pub fn stat(p: &CStr) -> io::Result<FileAttr> {
+    cfg_has_statx! {
+        if let Some(ret) = unsafe { try_statx(
+            libc::AT_FDCWD,
+            p.as_ptr(),
+            libc::AT_STATX_SYNC_AS_STAT,
+            libc::STATX_BASIC_STATS | libc::STATX_BTIME,
+        ) } {
+            return ret;
         }
+    }
 
-        let mut stat: stat64 = unsafe { mem::zeroed() };
-        cvt(unsafe { stat64(p.as_ptr(), &mut stat) })?;
-        Ok(FileAttr::from_stat64(stat))
-    })
+    let mut stat: stat64 = unsafe { mem::zeroed() };
+    cvt(unsafe { stat64(p.as_ptr(), &mut stat) })?;
+    Ok(FileAttr::from_stat64(stat))
 }
 
-pub fn lstat(p: &Path) -> io::Result<FileAttr> {
-    run_path_with_cstr(p, &|p| {
-        cfg_has_statx! {
-            if let Some(ret) = unsafe { try_statx(
-                libc::AT_FDCWD,
-                p.as_ptr(),
-                libc::AT_SYMLINK_NOFOLLOW | libc::AT_STATX_SYNC_AS_STAT,
-                libc::STATX_BASIC_STATS | libc::STATX_BTIME,
-            ) } {
-                return ret;
-            }
+pub fn lstat(p: &CStr) -> io::Result<FileAttr> {
+    cfg_has_statx! {
+        if let Some(ret) = unsafe { try_statx(
+            libc::AT_FDCWD,
+            p.as_ptr(),
+            libc::AT_SYMLINK_NOFOLLOW | libc::AT_STATX_SYNC_AS_STAT,
+            libc::STATX_BASIC_STATS | libc::STATX_BTIME,
+        ) } {
+            return ret;
         }
+    }
 
-        let mut stat: stat64 = unsafe { mem::zeroed() };
-        cvt(unsafe { lstat64(p.as_ptr(), &mut stat) })?;
-        Ok(FileAttr::from_stat64(stat))
-    })
+    let mut stat: stat64 = unsafe { mem::zeroed() };
+    cvt(unsafe { lstat64(p.as_ptr(), &mut stat) })?;
+    Ok(FileAttr::from_stat64(stat))
 }
 
-pub fn canonicalize(p: &Path) -> io::Result<PathBuf> {
-    let r = run_path_with_cstr(p, &|path| unsafe {
-        Ok(libc::realpath(path.as_ptr(), ptr::null_mut()))
-    })?;
+pub fn canonicalize(path: &CStr) -> io::Result<PathBuf> {
+    let r = unsafe { libc::realpath(path.as_ptr(), ptr::null_mut()) };
     if r.is_null() {
         return Err(io::Error::last_os_error());
     }
@@ -2328,19 +2307,19 @@ mod remove_dir_impl {
         Ok(())
     }
 
-    fn remove_dir_all_modern(p: &Path) -> io::Result<()> {
+    fn remove_dir_all_modern(p: &CStr) -> io::Result<()> {
         // We cannot just call remove_dir_all_recursive() here because that would not delete a passed
         // symlink. No need to worry about races, because remove_dir_all_recursive() does not recurse
         // into symlinks.
         let attr = lstat(p)?;
         if attr.file_type().is_symlink() {
-            crate::fs::remove_file(p)
+            super::unlink(p)
         } else {
-            run_path_with_cstr(p, &|p| remove_dir_all_recursive(None, &p))
+            remove_dir_all_recursive(None, &p)
         }
     }
 
     pub fn remove_dir_all(p: &Path) -> io::Result<()> {
-        remove_dir_all_modern(p)
+        run_path_with_cstr(p, &remove_dir_all_modern)
     }
 }