From 5dee44903641e186e3cd48020cf30ae2e06f681e Mon Sep 17 00:00:00 2001 From: ddidderr Date: Sat, 18 Jul 2026 16:52:52 +0200 Subject: [PATCH] feat(cli): move source file opening leaf to Rust Keep FIO_openSrcFile's stdin sentinel handling, Windows binary-mode setup, existing diagnostics, and FILE ownership in the C wrapper while moving only non-stdin source validation and opening behind the Rust CLI ABI. The Rust leaf returns distinct stat, non-regular, and fopen failure statuses, delegates metadata and regular/FIFO/block-device classification to the existing utility ABI, and passes the original C path directly to fopen("rb"). Opaque stat_t and FILE* pointers cross the boundary, and the output stream is published only on success; Rust never closes a returned stream. Focused backend tests cover regular-file reads, missing and directory paths, unchanged output pointers on failure, empty and spaced paths, and symlinks. FIFO acceptance remains in the preserved classification order without a test that could block while opening a named pipe. Test Plan: - `cargo test --manifest-path rust/cli/Cargo.toml --no-default-features --features cli,compression,decompression --lib fileio_backend` -- 22 passed - CLI clippy for library, benches, and tests with `cli,compression,decompression,benchmark`, before and after nightly formatting -- clean after the new-code warnings were fixed - `cargo +nightly fmt --manifest-path rust/cli/Cargo.toml --all -- --check` -- passed - Root Rust clippy with the reduced `compression` feature split for library, benches, and tests -- passed with one pre-existing warning in forbidden `rust/src/zstd_compress.rs` - `make -B -C programs -j2 zstd` -- passed - `make -B -C programs -j2 zstd-small zstd-frugal zstd-dictBuilder` -- passed with existing unused-function warnings in compact CLI builds - `make -C tests -j2 test-cli-tests` -- all 41 CLI tests passed, including file-stat coverage - `make -C tests -j2 test-zstd` -- reached dictionary training, then hit a segmentation fault in the unrelated concurrent compressor/dictionary path - `git diff --check` and `git diff --cached --check` -- passed --- programs/fileio.c | 45 ++++++---- rust/src/fileio_backend.rs | 179 ++++++++++++++++++++++++++++++++++++- 2 files changed, 206 insertions(+), 18 deletions(-) diff --git a/programs/fileio.c b/programs/fileio.c index 33caa474c..d056e6c9f 100644 --- a/programs/fileio.c +++ b/programs/fileio.c @@ -351,6 +351,16 @@ int FIO_rust_adjustMemLimitForPatchFromMode(FIO_prefs_t* prefs, unsigned long long dictSize, unsigned long long maxSrcFileSize); int FIO_rust_getDictFileStat(const char* fileName, stat_t* statBuf); +enum { + FIO_RUST_OPEN_SRC_SUCCESS = 0, + FIO_RUST_OPEN_SRC_STAT_FAILED = 1, + FIO_RUST_OPEN_SRC_NON_REGULAR = 2, + FIO_RUST_OPEN_SRC_FOPEN_FAILED = 3, +}; +int FIO_rust_openSrcFile(int allowBlockDevices, + const char* srcFileName, + stat_t* statbuf, + FILE** outFile); int FIO_rust_setDictBufferMalloc(const char* fileName, unsigned long long expectedFileSize, size_t maxSize, @@ -413,24 +423,29 @@ static FILE* FIO_openSrcFile(const FIO_prefs_t* const prefs, const char* srcFile return stdin; } - if (!UTIL_stat(srcFileName, statbuf)) { - DISPLAYLEVEL(1, "zstd: can't stat %s : %s -- ignored \n", - srcFileName, strerror(errno)); - return NULL; - } + { FILE* f = NULL; + int const status = FIO_rust_openSrcFile(allowBlockDevices, + srcFileName, + statbuf, + &f); + if (status == FIO_RUST_OPEN_SRC_STAT_FAILED) { + DISPLAYLEVEL(1, "zstd: can't stat %s : %s -- ignored \n", + srcFileName, strerror(errno)); + return NULL; + } - if (!UTIL_isRegularFileStat(statbuf) - && !UTIL_isFIFOStat(statbuf) - && !(allowBlockDevices && UTIL_isBlockDevStat(statbuf)) - ) { - DISPLAYLEVEL(1, "zstd: %s is not a regular file -- ignored \n", - srcFileName); - return NULL; - } + if (status == FIO_RUST_OPEN_SRC_NON_REGULAR) { + DISPLAYLEVEL(1, "zstd: %s is not a regular file -- ignored \n", + srcFileName); + return NULL; + } - { FILE* const f = fopen(srcFileName, "rb"); - if (f == NULL) + if (status == FIO_RUST_OPEN_SRC_FOPEN_FAILED) { DISPLAYLEVEL(1, "zstd: %s: %s \n", srcFileName, strerror(errno)); + return NULL; + } + + assert(status == FIO_RUST_OPEN_SRC_SUCCESS); return f; } } diff --git a/rust/src/fileio_backend.rs b/rust/src/fileio_backend.rs index ada4ae1f6..3a89d224e 100644 --- a/rust/src/fileio_backend.rs +++ b/rust/src/fileio_backend.rs @@ -5,8 +5,9 @@ //! //! The surrounding CLI still owns policy, diagnostics, and stream orchestration //! in C. This module implements the filesystem operations behind -//! `FIO_removeFile()` and dictionary loading, returning small status codes so -//! the C wrappers can retain their existing messages and conventions. +//! `FIO_openSrcFile()`, `FIO_removeFile()`, and dictionary loading, returning +//! small status codes so the C wrappers can retain their existing messages and +//! conventions. use std::ffi::{c_char, c_void, CStr}; use std::fs::File; @@ -30,9 +31,17 @@ const FIO_DICT_STAT_SUCCESS: c_int = 0; const FIO_DICT_STAT_FAILED: c_int = 1; const FIO_DICT_STAT_NON_REGULAR: c_int = 2; +const FIO_OPEN_SRC_SUCCESS: c_int = 0; +const FIO_OPEN_SRC_STAT_FAILED: c_int = 1; +const FIO_OPEN_SRC_NON_REGULAR: c_int = 2; +const FIO_OPEN_SRC_FOPEN_FAILED: c_int = 3; + unsafe extern "C" { fn UTIL_stat(file_name: *const c_char, stat_buf: *mut libc::stat) -> c_int; fn UTIL_isRegularFileStat(stat_buf: *const libc::stat) -> c_int; + fn UTIL_isFIFOStat(stat_buf: *const libc::stat) -> c_int; + fn UTIL_isBlockDevStat(stat_buf: *const libc::stat) -> c_int; + fn fopen(file_name: *const c_char, mode: *const c_char) -> *mut c_void; } fn path_from_c(path: *const c_char) -> Option { @@ -74,6 +83,43 @@ pub unsafe extern "C" fn FIO_rust_getDictFileStat( FIO_DICT_STAT_SUCCESS } +/// Opens a non-stdin source path and returns a status for the C policy adapter. +/// +/// `stat_buf` and `out_file` deliberately cross this boundary as opaque +/// pointers. The existing utility ABI owns the target-specific `stat_t` layout +/// and classification semantics; the C caller owns the returned `FILE*` and +/// must close it. The output is published only after `fopen("rb")` succeeds. +#[no_mangle] +pub unsafe extern "C" fn FIO_rust_openSrcFile( + allow_block_devices: c_int, + file_name: *const c_char, + stat_buf: *mut c_void, + out_file: *mut *mut c_void, +) -> c_int { + if file_name.is_null() || stat_buf.is_null() || out_file.is_null() { + return FIO_OPEN_SRC_STAT_FAILED; + } + + if unsafe { UTIL_stat(file_name, stat_buf.cast()) } == 0 { + return FIO_OPEN_SRC_STAT_FAILED; + } + + if unsafe { UTIL_isRegularFileStat(stat_buf.cast()) } == 0 + && unsafe { UTIL_isFIFOStat(stat_buf.cast()) } == 0 + && (allow_block_devices == 0 || unsafe { UTIL_isBlockDevStat(stat_buf.cast()) } == 0) + { + return FIO_OPEN_SRC_NON_REGULAR; + } + + let file = unsafe { fopen(file_name, c"rb".as_ptr().cast()) }; + if file.is_null() { + return FIO_OPEN_SRC_FOPEN_FAILED; + } + + unsafe { *out_file = file }; + FIO_OPEN_SRC_SUCCESS +} + /// Removes a regular file and returns a status consumed by the C adapter. /// /// The metadata query follows symlinks, matching the existing `stat()`-based @@ -190,7 +236,34 @@ mod tests { } fn c_path(path: &Path) -> CString { - CString::new(path.to_string_lossy().as_bytes()).expect("temporary path contains NUL") + #[cfg(unix)] + { + use std::os::unix::ffi::OsStrExt; + CString::new(path.as_os_str().as_bytes()).expect("temporary path contains NUL") + } + #[cfg(not(unix))] + { + CString::new(path.to_string_lossy().as_bytes()).expect("temporary path contains NUL") + } + } + + fn open_source(path: &Path) -> (c_int, *mut c_void) { + let path = c_path(path); + let mut stat_buf = MaybeUninit::::uninit(); + let mut out_file: *mut c_void = ptr::null_mut(); + let status = unsafe { + FIO_rust_openSrcFile( + 0, + path.as_ptr(), + stat_buf.as_mut_ptr().cast(), + &mut out_file, + ) + }; + (status, out_file) + } + + unsafe fn close_source(file: *mut c_void) { + assert_eq!(libc::fclose(file.cast()), 0); } fn get_dict_file_stat(path: Option<&Path>) -> c_int { @@ -363,6 +436,106 @@ mod tests { fs::remove_file(&target).expect("remove temporary file"); } + #[test] + fn opens_regular_file_and_reads_contents() { + let path = temp_path("open-regular"); + let payload = b"source payload"; + fs::write(&path, payload).expect("create temporary file"); + + let (status, file) = open_source(&path); + assert_eq!(status, FIO_OPEN_SRC_SUCCESS); + assert!(!file.is_null()); + + let mut contents = vec![0; payload.len()]; + let read = + unsafe { libc::fread(contents.as_mut_ptr().cast(), 1, contents.len(), file.cast()) }; + assert_eq!(read, payload.len()); + assert_eq!(contents, payload); + + unsafe { close_source(file) }; + fs::remove_file(&path).expect("remove temporary file"); + } + + #[test] + fn rejects_missing_source_path() { + let path = temp_path("open-missing"); + + let (status, file) = open_source(&path); + assert_eq!(status, FIO_OPEN_SRC_STAT_FAILED); + assert!(file.is_null()); + } + + #[test] + fn rejects_directory_source_path() { + let path = temp_path("open-directory"); + fs::create_dir(&path).expect("create temporary directory"); + + let (status, file) = open_source(&path); + assert_eq!(status, FIO_OPEN_SRC_NON_REGULAR); + assert!(file.is_null()); + + fs::remove_dir(&path).expect("remove temporary directory"); + } + + #[test] + fn leaves_output_untouched_on_failure() { + let path = temp_path("open-output"); + let path = c_path(&path); + let mut stat_buf = MaybeUninit::::uninit(); + let sentinel = ptr::dangling_mut::(); + let mut out_file = sentinel; + + let status = unsafe { + FIO_rust_openSrcFile( + 0, + path.as_ptr(), + stat_buf.as_mut_ptr().cast(), + &mut out_file, + ) + }; + assert_eq!(status, FIO_OPEN_SRC_STAT_FAILED); + assert_eq!(out_file, sentinel); + } + + #[test] + fn rejects_empty_source_path_without_output() { + let (status, file) = open_source(Path::new("")); + assert_eq!(status, FIO_OPEN_SRC_STAT_FAILED); + assert!(file.is_null()); + } + + #[test] + fn opens_source_path_with_spaces() { + let path = temp_path("open path with spaces"); + fs::write(&path, b"payload").expect("create temporary file"); + + let (status, file) = open_source(&path); + assert_eq!(status, FIO_OPEN_SRC_SUCCESS); + assert!(!file.is_null()); + + unsafe { close_source(file) }; + fs::remove_file(&path).expect("remove temporary file"); + } + + #[cfg(unix)] + #[test] + fn opens_symlink_to_regular_source_file() { + use std::os::unix::fs::symlink; + + let target = temp_path("open-symlink-target"); + let link = temp_path("open-symlink"); + fs::write(&target, b"payload").expect("create temporary file"); + symlink(&target, &link).expect("create symbolic link"); + + let (status, file) = open_source(&link); + assert_eq!(status, FIO_OPEN_SRC_SUCCESS); + assert!(!file.is_null()); + + unsafe { close_source(file) }; + fs::remove_file(&link).expect("remove symbolic link"); + fs::remove_file(&target).expect("remove temporary file"); + } + #[test] fn removes_regular_file() { let path = temp_path("regular");