Merge pull request #9437 from naoNao89/sync-remove-unwrap

sync: Reset O_NONBLOCK flag after file validation to match GNU behavior

.vscode/cspell.dictionaries/jargon.wordlist.txt

@@ -136,6 +136,7 @@ semiprimes
 setcap
 setfacl
 setfattr
+SETFL
 setlocale
 shortcode
 shortcodes

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<String>) -> 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<String>) -> 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<String>) -> 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(())
     }

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<String> {
+        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<char> = 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)]

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