From 371451d69c5737fe845531ef58c939459e9c6679 Mon Sep 17 00:00:00 2001 From: ddidderr Date: Mon, 20 Jul 2026 04:03:41 +0200 Subject: [PATCH] refactor(cli): move --list multi-file policy to Rust Move stdin and empty-input validation, header selection, per-file iteration, status aggregation, and multi-file total selection into the Rust CLI policy layer. The callback projection keeps C responsible for its private file-info storage, file parsing, diagnostics, human-readable formatting, and per-file listing. Only successful and frame-error records contribute to the aggregate, matching the original early-return behavior for invalid or truncated inputs. Test Plan: - `rustfmt --edition 2021 --check rust/src/fileio_prefs.rs` -- passed. - `git diff --cached --check` -- passed. - Cargo, native, and full test commands were not run per the explicit no-heavy-command constraint. --- programs/fileio.c | 174 +++++++++++------ rust/src/fileio_prefs.rs | 412 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 526 insertions(+), 60 deletions(-) diff --git a/programs/fileio.c b/programs/fileio.c index 53bcd84ee..b806ce72c 100644 --- a/programs/fileio.c +++ b/programs/fileio.c @@ -4544,15 +4544,6 @@ typedef char FIO_rust_file_info_size[ ? 7 * sizeof(U64) : 13 * sizeof(U32))) ? 1 : -1]; -void FIO_rust_addFInfo(fileInfo_t* output, fileInfo_t const* fi1, fileInfo_t const* fi2); - -static fileInfo_t FIO_addFInfo(fileInfo_t fi1, fileInfo_t fi2) -{ - fileInfo_t total; - FIO_rust_addFInfo(&total, &fi1, &fi2); - return total; -} - typedef enum { info_success=0, info_frame_error=1, @@ -4561,6 +4552,35 @@ typedef enum { info_truncated_input=4 } InfoError; +/* Keep the private fileInfo_t layout in C. This projection contains only the + * fields needed by the --list total row and crosses the Rust policy boundary + * after FIO_listFile has finished its C-owned open/parse/display work. */ +typedef struct { + U64 decompressedSize; + U64 compressedSize; + int numActualFrames; + int numSkippableFrames; + int decompUnavailable; + int usesCheck; + U32 nbFiles; +} FIO_listFileInfoProjection_t; + +typedef struct { + void* opaque; + int (*isStdin)(void* opaque, const char* fileName); + void (*displayStdinError)(void* opaque); + void (*displayNoFilesError)(void* opaque); + void (*displayHeader)(void* opaque); + int (*listFile)(void* opaque, const char* fileName, int displayLevel, + FIO_listFileInfoProjection_t* info); + void (*displayTotal)(void* opaque, + const FIO_listFileInfoProjection_t* total); +} FIO_listMultipleFilesCallbacks_t; + +int FIO_rust_listMultipleFiles(unsigned numFiles, const char** filenameTable, + int displayLevel, + const FIO_listMultipleFilesCallbacks_t* callbacks); + enum { FIO_RUST_ANALYZE_DIAG_SEEKED_PAST_FILE = 0, FIO_RUST_ANALYZE_DIAG_INCOMPLETE_FRAME = 1, @@ -4733,7 +4753,8 @@ displayInfo(const char* inFileName, const fileInfo_t* info, int displayLevel) } static int -FIO_listFile(fileInfo_t* total, const char* inFileName, int displayLevel) +FIO_listFile(const char* inFileName, int displayLevel, + FIO_listFileInfoProjection_t* output) { fileInfo_t info; memset(&info, 0, sizeof(info)); @@ -4761,63 +4782,96 @@ FIO_listFile(fileInfo_t* total, const char* inFileName, int displayLevel) } displayInfo(inFileName, &info, displayLevel); - *total = FIO_addFInfo(*total, info); + output->decompressedSize = info.decompressedSize; + output->compressedSize = info.compressedSize; + output->numActualFrames = info.numActualFrames; + output->numSkippableFrames = info.numSkippableFrames; + output->decompUnavailable = info.decompUnavailable; + output->usesCheck = info.usesCheck; + output->nbFiles = info.nbFiles; assert(error == info_success || error == info_frame_error); return (int)error; } } +static int +FIO_listFileIsStdin(void* opaque, const char* fileName) +{ + (void)opaque; + return !strcmp(fileName, stdinmark); +} + +static void +FIO_listFileDisplayStdinError(void* opaque) +{ + (void)opaque; + DISPLAYLEVEL(1, "zstd: --list does not support reading from standard input"); + DISPLAYLEVEL(1, " \n"); +} + +static void +FIO_listFileDisplayNoFilesError(void* opaque) +{ + (void)opaque; + if (!UTIL_isConsole(stdin)) { + DISPLAYLEVEL(1, "zstd: --list does not support reading from standard input \n"); + } + DISPLAYLEVEL(1, "No files given \n"); +} + +static void +FIO_listFileDisplayHeader(void* opaque) +{ + (void)opaque; + DISPLAYOUT("Frames Skips Compressed Uncompressed Ratio Check Filename\n"); +} + +static int +FIO_listFileCallback(void* opaque, const char* fileName, int displayLevel, + FIO_listFileInfoProjection_t* info) +{ + (void)opaque; + memset(info, 0, sizeof(*info)); + return FIO_listFile(fileName, displayLevel, info); +} + +static void +FIO_listFileDisplayTotal(void* opaque, + const FIO_listFileInfoProjection_t* total) +{ + UTIL_HumanReadableSize_t const compressed_hrs = UTIL_makeHumanReadableSize(total->compressedSize); + UTIL_HumanReadableSize_t const decompressed_hrs = UTIL_makeHumanReadableSize(total->decompressedSize); + double const ratio = (total->compressedSize == 0) ? 0 : ((double)total->decompressedSize)/(double)total->compressedSize; + const char* const checkString = (total->usesCheck ? "XXH64" : ""); + (void)opaque; + DISPLAYOUT("----------------------------------------------------------------- \n"); + if (total->decompUnavailable) { + DISPLAYOUT("%6d %5d %6.*f%4s %5s %u files\n", + total->numSkippableFrames + total->numActualFrames, + total->numSkippableFrames, + compressed_hrs.precision, compressed_hrs.value, compressed_hrs.suffix, + checkString, (unsigned)total->nbFiles); + } else { + DISPLAYOUT("%6d %5d %6.*f%4s %8.*f%4s %5.3f %5s %u files\n", + total->numSkippableFrames + total->numActualFrames, + total->numSkippableFrames, + compressed_hrs.precision, compressed_hrs.value, compressed_hrs.suffix, + decompressed_hrs.precision, decompressed_hrs.value, decompressed_hrs.suffix, + ratio, checkString, (unsigned)total->nbFiles); + } +} + int FIO_listMultipleFiles(unsigned numFiles, const char** filenameTable, int displayLevel) { - /* ensure no specified input is stdin (needs fseek() capability) */ - { unsigned u; - for (u=0; u 1 && displayLevel <= 2) { /* display total */ - UTIL_HumanReadableSize_t const compressed_hrs = UTIL_makeHumanReadableSize(total.compressedSize); - UTIL_HumanReadableSize_t const decompressed_hrs = UTIL_makeHumanReadableSize(total.decompressedSize); - double const ratio = (total.compressedSize == 0) ? 0 : ((double)total.decompressedSize)/(double)total.compressedSize; - const char* const checkString = (total.usesCheck ? "XXH64" : ""); - DISPLAYOUT("----------------------------------------------------------------- \n"); - if (total.decompUnavailable) { - DISPLAYOUT("%6d %5d %6.*f%4s %5s %u files\n", - total.numSkippableFrames + total.numActualFrames, - total.numSkippableFrames, - compressed_hrs.precision, compressed_hrs.value, compressed_hrs.suffix, - checkString, (unsigned)total.nbFiles); - } else { - DISPLAYOUT("%6d %5d %6.*f%4s %8.*f%4s %5.3f %5s %u files\n", - total.numSkippableFrames + total.numActualFrames, - total.numSkippableFrames, - compressed_hrs.precision, compressed_hrs.value, compressed_hrs.suffix, - decompressed_hrs.precision, decompressed_hrs.value, decompressed_hrs.suffix, - ratio, checkString, (unsigned)total.nbFiles); - } } - return error; - } + FIO_listMultipleFilesCallbacks_t callbacks; + callbacks.opaque = NULL; + callbacks.isStdin = FIO_listFileIsStdin; + callbacks.displayStdinError = FIO_listFileDisplayStdinError; + callbacks.displayNoFilesError = FIO_listFileDisplayNoFilesError; + callbacks.displayHeader = FIO_listFileDisplayHeader; + callbacks.listFile = FIO_listFileCallback; + callbacks.displayTotal = FIO_listFileDisplayTotal; + return FIO_rust_listMultipleFiles(numFiles, filenameTable, displayLevel, &callbacks); } diff --git a/rust/src/fileio_prefs.rs b/rust/src/fileio_prefs.rs index 8b360db44..6d6b63e97 100644 --- a/rust/src/fileio_prefs.rs +++ b/rust/src/fileio_prefs.rs @@ -28,6 +28,8 @@ const FIO_MULTI_FILES_ACTION_FATAL_TEST_REMOVE: c_int = 2; const FIO_MULTI_FILES_ACTION_DISABLE_REMOVE: c_int = 3; const FIO_MULTI_FILES_ACTION_QUIET_ABORT: c_int = 4; const FIO_MULTI_FILES_ACTION_CONFIRM: c_int = 5; +const FIO_LIST_INFO_SUCCESS: c_int = 0; +const FIO_LIST_INFO_FRAME_ERROR: c_int = 1; const UTIL_FILESIZE_UNKNOWN: u64 = u64::MAX; const ZSTD_WINDOWLOG_MIN: u32 = 10; const ZSTD_WINDOWLOG_MAX: u32 = if size_of::() == 4 { 30 } else { 31 }; @@ -86,6 +88,44 @@ pub struct FIO_fileInfo_t { dictID: c_uint, } +/// The subset of C's private `fileInfo_t` needed by the `--list` aggregate +/// row. C keeps opening, parsing, and formatting each file; Rust owns only +/// the policy that iterates and aggregates these values. +#[repr(C)] +#[derive(Clone, Copy, Debug, Default, PartialEq, Eq)] +pub struct FIO_listFileInfoProjection_t { + decompressedSize: u64, + compressedSize: u64, + numActualFrames: c_int, + numSkippableFrames: c_int, + decompUnavailable: c_int, + usesCheck: c_int, + nbFiles: u32, +} + +pub type FIO_listMultipleFilesIsStdinFn = unsafe extern "C" fn(*mut c_void, *const c_char) -> c_int; +pub type FIO_listMultipleFilesDisplayFn = unsafe extern "C" fn(*mut c_void); +pub type FIO_listMultipleFilesListFileFn = unsafe extern "C" fn( + *mut c_void, + *const c_char, + c_int, + *mut FIO_listFileInfoProjection_t, +) -> c_int; +pub type FIO_listMultipleFilesDisplayTotalFn = + unsafe extern "C" fn(*mut c_void, *const FIO_listFileInfoProjection_t); + +/// Callback projection for the C-owned `--list` file operations. +#[repr(C)] +pub struct FIO_listMultipleFilesCallbacks_t { + opaque: *mut c_void, + isStdin: Option, + displayStdinError: Option, + displayNoFilesError: Option, + displayHeader: Option, + listFile: Option, + displayTotal: Option, +} + #[cfg(feature = "decompression")] const FIO_INFO_SUCCESS: c_int = 0; #[cfg(feature = "decompression")] @@ -1443,6 +1483,102 @@ pub unsafe extern "C" fn FIO_rust_addFInfo( unsafe { output.write(total) }; } +#[inline] +fn add_list_file_info( + first: FIO_listFileInfoProjection_t, + second: FIO_listFileInfoProjection_t, +) -> FIO_listFileInfoProjection_t { + FIO_listFileInfoProjection_t { + decompressedSize: first.decompressedSize.wrapping_add(second.decompressedSize), + compressedSize: first.compressedSize.wrapping_add(second.compressedSize), + numActualFrames: first.numActualFrames.wrapping_add(second.numActualFrames), + numSkippableFrames: first + .numSkippableFrames + .wrapping_add(second.numSkippableFrames), + decompUnavailable: first.decompUnavailable | second.decompUnavailable, + usesCheck: first.usesCheck & second.usesCheck, + nbFiles: first.nbFiles.wrapping_add(second.nbFiles), + } +} + +/// Run the policy surrounding C's `--list` file operation. +/// +/// Rust owns validation, iteration, error aggregation, and the total-row +/// decision. The callbacks retain C's private file-info operations and all +/// user-visible formatting so the private `fileInfo_t` never crosses the ABI. +#[no_mangle] +pub unsafe extern "C" fn FIO_rust_listMultipleFiles( + numFiles: c_uint, + filenameTable: *const *const c_char, + displayLevel: c_int, + callbacks: *const FIO_listMultipleFilesCallbacks_t, +) -> c_int { + assert!(!callbacks.is_null()); + let callbacks = unsafe { &*callbacks }; + if numFiles != 0 { + assert!(!filenameTable.is_null()); + } + + let is_stdin = callbacks + .isStdin + .expect("--list stdin callback is required"); + let display_stdin_error = callbacks + .displayStdinError + .expect("--list stdin-error callback is required"); + let display_no_files_error = callbacks + .displayNoFilesError + .expect("--list no-files callback is required"); + + for index in 0..numFiles as usize { + let file_name = unsafe { *filenameTable.add(index) }; + assert!(!file_name.is_null()); + if unsafe { is_stdin(callbacks.opaque, file_name) } != 0 { + unsafe { display_stdin_error(callbacks.opaque) }; + return 1; + } + } + + if numFiles == 0 { + unsafe { display_no_files_error(callbacks.opaque) }; + return 1; + } + + if displayLevel <= 2 { + let display_header = callbacks + .displayHeader + .expect("--list header callback is required"); + unsafe { display_header(callbacks.opaque) }; + } + + let list_file = callbacks + .listFile + .expect("--list file callback is required"); + let mut error = 0; + let mut total = FIO_listFileInfoProjection_t { + usesCheck: 1, + ..FIO_listFileInfoProjection_t::default() + }; + + for index in 0..numFiles as usize { + let file_name = unsafe { *filenameTable.add(index) }; + let mut info = FIO_listFileInfoProjection_t::default(); + let status = unsafe { list_file(callbacks.opaque, file_name, displayLevel, &mut info) }; + error |= status; + if status == FIO_LIST_INFO_SUCCESS || status == FIO_LIST_INFO_FRAME_ERROR { + total = add_list_file_info(total, info); + } + } + + if numFiles > 1 && displayLevel <= 2 { + let display_total = callbacks + .displayTotal + .expect("--list total callback is required"); + unsafe { display_total(callbacks.opaque, &total) }; + } + + error +} + #[inline] fn lz4_block_size_from_block_id(id: c_int) -> c_int { 1 << (8 + 2 * id) @@ -1737,6 +1873,79 @@ mod tests { std::env::temp_dir().join(format!("zstd-fileio-prefs-{}-{name}", std::process::id())) } + #[derive(Default)] + struct ListMultipleFilesTestState { + events: Vec<&'static str>, + statuses: Vec, + infos: Vec, + list_calls: usize, + display_levels: Vec, + total: Option, + } + + unsafe extern "C" fn list_multiple_is_stdin( + opaque: *mut c_void, + file_name: *const c_char, + ) -> c_int { + let state = unsafe { &mut *opaque.cast::() }; + state.events.push("stdin"); + let file_name = unsafe { CStr::from_ptr(file_name).to_bytes() }; + c_int::from(file_name == b"/*stdin*\\") + } + + unsafe extern "C" fn list_multiple_display_stdin_error(opaque: *mut c_void) { + let state = unsafe { &mut *opaque.cast::() }; + state.events.push("stdin-error"); + } + + unsafe extern "C" fn list_multiple_display_no_files_error(opaque: *mut c_void) { + let state = unsafe { &mut *opaque.cast::() }; + state.events.push("no-files"); + } + + unsafe extern "C" fn list_multiple_display_header(opaque: *mut c_void) { + let state = unsafe { &mut *opaque.cast::() }; + state.events.push("header"); + } + + unsafe extern "C" fn list_multiple_list_file( + opaque: *mut c_void, + _file_name: *const c_char, + display_level: c_int, + info: *mut FIO_listFileInfoProjection_t, + ) -> c_int { + let state = unsafe { &mut *opaque.cast::() }; + let index = state.list_calls; + state.list_calls += 1; + state.events.push("list"); + state.display_levels.push(display_level); + unsafe { info.write(state.infos[index]) }; + state.statuses[index] + } + + unsafe extern "C" fn list_multiple_display_total( + opaque: *mut c_void, + total: *const FIO_listFileInfoProjection_t, + ) { + let state = unsafe { &mut *opaque.cast::() }; + state.events.push("total"); + state.total = Some(unsafe { *total }); + } + + fn list_multiple_callbacks( + state: &mut ListMultipleFilesTestState, + ) -> FIO_listMultipleFilesCallbacks_t { + FIO_listMultipleFilesCallbacks_t { + opaque: (state as *mut ListMultipleFilesTestState).cast(), + isStdin: Some(list_multiple_is_stdin), + displayStdinError: Some(list_multiple_display_stdin_error), + displayNoFilesError: Some(list_multiple_display_no_files_error), + displayHeader: Some(list_multiple_display_header), + listFile: Some(list_multiple_list_file), + displayTotal: Some(list_multiple_display_total), + } + } + #[cfg(feature = "decompression")] struct TemporaryCFile(*mut libc::FILE); @@ -2269,6 +2478,209 @@ mod tests { assert_eq!(total.dictID, 0); } + #[test] + fn list_multiple_files_rejects_stdin_before_display_or_iteration() { + let names = [ + CString::new("first.zst").unwrap(), + CString::new("/*stdin*\\").unwrap(), + CString::new("third.zst").unwrap(), + ]; + let name_table: Vec<_> = names.iter().map(|name| name.as_ptr()).collect(); + let mut state = ListMultipleFilesTestState::default(); + let callbacks = list_multiple_callbacks(&mut state); + + let status = unsafe { + FIO_rust_listMultipleFiles( + name_table.len() as c_uint, + name_table.as_ptr(), + 0, + &callbacks, + ) + }; + + assert_eq!(status, 1); + assert_eq!(state.events, vec!["stdin", "stdin", "stdin-error"]); + assert_eq!(state.list_calls, 0); + assert!(state.total.is_none()); + } + + #[test] + fn list_multiple_files_reports_empty_input_without_header() { + let mut state = ListMultipleFilesTestState::default(); + let callbacks = list_multiple_callbacks(&mut state); + + let status = unsafe { FIO_rust_listMultipleFiles(0, ptr::null(), 0, &callbacks) }; + + assert_eq!(status, 1); + assert_eq!(state.events, vec!["no-files"]); + assert_eq!(state.list_calls, 0); + assert!(state.total.is_none()); + } + + #[test] + fn list_multiple_files_continues_after_errors_and_aggregates_total() { + let names = [ + CString::new("first.zst").unwrap(), + CString::new("second.zst").unwrap(), + CString::new("third.zst").unwrap(), + ]; + let name_table: Vec<_> = names.iter().map(|name| name.as_ptr()).collect(); + let mut state = ListMultipleFilesTestState { + statuses: vec![0, 1, 4], + infos: vec![ + FIO_listFileInfoProjection_t { + decompressedSize: 10, + compressedSize: 4, + numActualFrames: 1, + numSkippableFrames: 2, + decompUnavailable: 0, + usesCheck: 1, + nbFiles: 1, + }, + FIO_listFileInfoProjection_t { + decompressedSize: 20, + compressedSize: 6, + numActualFrames: 3, + numSkippableFrames: 0, + decompUnavailable: 0, + usesCheck: 1, + nbFiles: 1, + }, + FIO_listFileInfoProjection_t::default(), + ], + ..ListMultipleFilesTestState::default() + }; + let callbacks = list_multiple_callbacks(&mut state); + + let status = unsafe { + FIO_rust_listMultipleFiles( + name_table.len() as c_uint, + name_table.as_ptr(), + 2, + &callbacks, + ) + }; + + assert_eq!(status, 1 | 4); + assert_eq!(state.list_calls, 3); + assert_eq!(state.display_levels, vec![2, 2, 2]); + assert_eq!( + state.events, + vec!["stdin", "stdin", "stdin", "header", "list", "list", "list", "total"] + ); + assert_eq!( + state.total, + Some(FIO_listFileInfoProjection_t { + decompressedSize: 30, + compressedSize: 10, + numActualFrames: 4, + numSkippableFrames: 2, + decompUnavailable: 0, + usesCheck: 1, + nbFiles: 2, + }) + ); + } + + #[test] + fn list_multiple_files_limits_header_and_total_to_original_thresholds() { + let names = [ + CString::new("first.zst").unwrap(), + CString::new("second.zst").unwrap(), + ]; + let name_table: Vec<_> = names.iter().map(|name| name.as_ptr()).collect(); + let mut state = ListMultipleFilesTestState { + statuses: vec![0, 0], + infos: vec![FIO_listFileInfoProjection_t::default(); 2], + ..ListMultipleFilesTestState::default() + }; + let callbacks = list_multiple_callbacks(&mut state); + + let status = unsafe { + FIO_rust_listMultipleFiles( + name_table.len() as c_uint, + name_table.as_ptr(), + 3, + &callbacks, + ) + }; + + assert_eq!(status, 0); + assert_eq!(state.events, vec!["stdin", "stdin", "list", "list"]); + assert!(state.total.is_none()); + + let single_name = [CString::new("single.zst").unwrap()]; + let single_table = [single_name[0].as_ptr()]; + let mut state = ListMultipleFilesTestState { + statuses: vec![0], + infos: vec![FIO_listFileInfoProjection_t::default()], + ..ListMultipleFilesTestState::default() + }; + let callbacks = list_multiple_callbacks(&mut state); + + let status = unsafe { FIO_rust_listMultipleFiles(1, single_table.as_ptr(), 2, &callbacks) }; + + assert_eq!(status, 0); + assert_eq!(state.events, vec!["stdin", "header", "list"]); + assert!(state.total.is_none()); + } + + #[test] + fn list_multiple_files_abi_matches_c_projection_layouts() { + let word = size_of::<*mut c_void>(); + let callback = size_of::>(); + let projection_alignment = align_of::(); + let projection_payload = 2 * size_of::() + 4 * size_of::() + size_of::(); + let projection_size = (projection_payload + projection_alignment - 1) + / projection_alignment + * projection_alignment; + + assert_eq!( + align_of::(), + align_of::() + ); + assert_eq!( + offset_of!(FIO_listFileInfoProjection_t, decompressedSize), + 0 + ); + assert_eq!( + offset_of!(FIO_listFileInfoProjection_t, compressedSize), + size_of::() + ); + assert_eq!( + offset_of!(FIO_listFileInfoProjection_t, numActualFrames), + 2 * size_of::() + ); + assert_eq!( + offset_of!(FIO_listFileInfoProjection_t, nbFiles), + 2 * size_of::() + 4 * size_of::() + ); + assert_eq!(size_of::(), projection_size); + + assert_eq!( + size_of::>(), + callback + ); + assert_eq!( + size_of::>(), + callback + ); + assert_eq!( + size_of::>(), + callback + ); + assert_eq!(offset_of!(FIO_listMultipleFilesCallbacks_t, opaque), 0); + assert_eq!(offset_of!(FIO_listMultipleFilesCallbacks_t, isStdin), word); + assert_eq!( + offset_of!(FIO_listMultipleFilesCallbacks_t, displayTotal), + word + 5 * callback + ); + assert_eq!( + size_of::(), + word + 6 * callback + ); + } + #[test] fn file_info_layout_matches_private_c_record() { let word = size_of::();