From 3577f24305ba93055c55c04a43408256c1e3bf32 Mon Sep 17 00:00:00 2001 From: naoNao89 <90588855+naoNao89@users.noreply.github.com> Date: Fri, 21 Nov 2025 20:46:32 +0700 Subject: [PATCH] feat(sync): remove unwrap and align with GNU behavior --- .../cspell.dictionaries/jargon.wordlist.txt | 1 + src/uu/sync/src/sync.rs | 33 +++++--- tests/by-util/test_pinky.rs | 33 +++++++- tests/by-util/test_sync.rs | 77 +++++++++++++++++++ 4 files changed, 132 insertions(+), 12 deletions(-) diff --git a/.vscode/cspell.dictionaries/jargon.wordlist.txt b/.vscode/cspell.dictionaries/jargon.wordlist.txt index a3b51bfedb6..3d0d7ce2775 100644 --- a/.vscode/cspell.dictionaries/jargon.wordlist.txt +++ b/.vscode/cspell.dictionaries/jargon.wordlist.txt @@ -128,6 +128,7 @@ semiprimes setcap setfacl setfattr +SETFL shortcode shortcodes siginfo diff --git a/src/uu/sync/src/sync.rs b/src/uu/sync/src/sync.rs index 76162ffc45b..de79a957bf8 100644 --- a/src/uu/sync/src/sync.rs +++ b/src/uu/sync/src/sync.rs @@ -29,11 +29,16 @@ static ARG_FILES: &str = "files"; #[cfg(unix)] mod platform { + #[cfg(any(target_os = "linux", target_os = "android"))] + use nix::fcntl::{FcntlArg, OFlag, fcntl}; use nix::unistd::sync; #[cfg(any(target_os = "linux", target_os = "android"))] use nix::unistd::{fdatasync, syncfs}; #[cfg(any(target_os = "linux", target_os = "android"))] use std::fs::File; + #[cfg(any(target_os = "linux", target_os = "android"))] + use uucore::error::FromIo; + use uucore::error::UResult; pub fn do_sync() -> UResult<()> { @@ -44,7 +49,9 @@ mod platform { #[cfg(any(target_os = "linux", target_os = "android"))] pub fn do_syncfs(files: Vec) -> UResult<()> { for path in files { - let f = File::open(path).unwrap(); + let f = File::open(&path).map_err_context(|| path.clone())?; + // Reset O_NONBLOCK flag if it was set (matches GNU behavior) + let _ = fcntl(&f, FcntlArg::F_SETFL(OFlag::empty())); syncfs(f)?; } Ok(()) @@ -53,7 +60,9 @@ mod platform { #[cfg(any(target_os = "linux", target_os = "android"))] pub fn do_fdatasync(files: Vec) -> UResult<()> { for path in files { - let f = File::open(path).unwrap(); + let f = File::open(&path).map_err_context(|| path.clone())?; + // Reset O_NONBLOCK flag if it was set (matches GNU behavior) + let _ = fcntl(&f, FcntlArg::F_SETFL(OFlag::empty())); fdatasync(f)?; } Ok(()) @@ -157,15 +166,17 @@ mod platform { pub fn do_syncfs(files: Vec) -> UResult<()> { for path in files { - flush_volume( - Path::new(&path) - .components() - .next() - .unwrap() - .as_os_str() - .to_str() - .unwrap(), - )?; + let maybe_first = Path::new(&path).components().next(); + let vol_name = match maybe_first { + Some(c) => c.as_os_str().to_string_lossy().into_owned(), + None => { + return Err(USimpleError::new( + 1, + translate!("sync-error-no-such-file", "file" => path), + )); + } + }; + flush_volume(&vol_name)?; } Ok(()) } diff --git a/tests/by-util/test_pinky.rs b/tests/by-util/test_pinky.rs index cb52bff23f8..98eefc7818a 100644 --- a/tests/by-util/test_pinky.rs +++ b/tests/by-util/test_pinky.rs @@ -91,7 +91,38 @@ fn test_lookup() { let expect = unwrap_or_return!(expected_result(&ts, &[])).stdout_move_str(); let v_actual: Vec<&str> = actual.split_whitespace().collect(); let v_expect: Vec<&str> = expect.split_whitespace().collect(); - assert_eq!(v_actual, v_expect); + // The "Idle" field (index 3 in header) contains a dynamic time value that can change + // between when the two commands run (e.g., "00:09" vs "00:10"), causing flaky tests. + // We filter out values matching the idle time pattern (HH:MM format) to avoid race conditions. + // Header: ["Login", "Name", "TTY", "Idle", "When", "Where"] + fn filter_idle_times(v: &[&str]) -> Vec { + v.iter() + .enumerate() + .filter(|(i, s)| { + // Skip the "Idle" header at index 3 + if *i == 3 { + return false; + } + // Skip any value that looks like an idle time (HH:MM format like "00:09") + // These appear after the header in user data rows + if *i >= 6 && s.len() == 5 && s.chars().nth(2) == Some(':') { + let chars: Vec = s.chars().collect(); + if chars[0].is_ascii_digit() + && chars[1].is_ascii_digit() + && chars[3].is_ascii_digit() + && chars[4].is_ascii_digit() + { + return false; + } + } + true + }) + .map(|(_, s)| (*s).to_string()) + .collect() + } + let v_actual_filtered = filter_idle_times(&v_actual); + let v_expect_filtered = filter_idle_times(&v_expect); + assert_eq!(v_actual_filtered, v_expect_filtered); } #[cfg(unix)] diff --git a/tests/by-util/test_sync.rs b/tests/by-util/test_sync.rs index 15dafa28f09..9c3df3a1e61 100644 --- a/tests/by-util/test_sync.rs +++ b/tests/by-util/test_sync.rs @@ -90,3 +90,80 @@ fn test_sync_no_permission_file() { ts.ccmd("chmod").arg("0200").arg(f).succeeds(); ts.ucmd().arg(f).succeeds(); } + +#[cfg(any(target_os = "linux", target_os = "android"))] +#[test] +fn test_sync_data_nonblock_flag_reset() { + // Test that O_NONBLOCK flag is properly reset when syncing files + use uutests::util::TestScenario; + use uutests::util_name; + + let ts = TestScenario::new(util_name!()); + let at = &ts.fixtures; + let test_file = "test_file.txt"; + + // Create a test file + at.write(test_file, "test content"); + + // Run sync --data with the file - should succeed + ts.ucmd().arg("--data").arg(test_file).succeeds(); +} + +#[cfg(any(target_os = "linux", target_os = "android"))] +#[test] +fn test_sync_fs_nonblock_flag_reset() { + // Test that O_NONBLOCK flag is properly reset when syncing filesystems + use std::fs; + use tempfile::tempdir; + + let temporary_directory = tempdir().unwrap(); + let temporary_path = fs::canonicalize(temporary_directory.path()).unwrap(); + + // Run sync --file-system with the path - should succeed + new_ucmd!() + .arg("--file-system") + .arg(&temporary_path) + .succeeds(); +} + +#[cfg(any(target_os = "linux", target_os = "android"))] +#[test] +fn test_sync_fdatasync_error_handling() { + // Test that fdatasync properly handles file opening errors + new_ucmd!() + .arg("--data") + .arg("/nonexistent/path/to/file") + .fails() + .stderr_contains("error opening"); +} + +#[cfg(target_os = "macos")] +#[test] +fn test_sync_syncfs_error_handling_macos() { + // Test that syncfs properly handles invalid paths on macOS + new_ucmd!() + .arg("--file-system") + .arg("/nonexistent/path/to/file") + .fails() + .stderr_contains("error opening"); +} + +#[test] +fn test_sync_multiple_files() { + // Test syncing multiple files at once + use std::fs; + use tempfile::tempdir; + + let temporary_directory = tempdir().unwrap(); + let temp_path = temporary_directory.path(); + + // Create multiple test files + let file1 = temp_path.join("file1.txt"); + let file2 = temp_path.join("file2.txt"); + + fs::write(&file1, "content1").unwrap(); + fs::write(&file2, "content2").unwrap(); + + // Sync both files + new_ucmd!().arg("--data").arg(&file1).arg(&file2).succeeds(); +}