Skip to content

Commit 83f162c

Browse files
committed
feat(sync): remove unwrap and align with GNU behavior
1 parent 2a314c7 commit 83f162c

File tree

3 files changed

+103
-11
lines changed

3 files changed

+103
-11
lines changed

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -128,6 +128,7 @@ semiprimes
128128
setcap
129129
setfacl
130130
setfattr
131+
SETFL
131132
shortcode
132133
shortcodes
133134
siginfo

src/uu/sync/src/sync.rs

Lines changed: 22 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -29,11 +29,16 @@ static ARG_FILES: &str = "files";
2929

3030
#[cfg(unix)]
3131
mod platform {
32+
#[cfg(any(target_os = "linux", target_os = "android"))]
33+
use nix::fcntl::{FcntlArg, OFlag, fcntl};
3234
use nix::unistd::sync;
3335
#[cfg(any(target_os = "linux", target_os = "android"))]
3436
use nix::unistd::{fdatasync, syncfs};
3537
#[cfg(any(target_os = "linux", target_os = "android"))]
3638
use std::fs::File;
39+
#[cfg(any(target_os = "linux", target_os = "android"))]
40+
use uucore::error::FromIo;
41+
3742
use uucore::error::UResult;
3843

3944
pub fn do_sync() -> UResult<()> {
@@ -44,7 +49,9 @@ mod platform {
4449
#[cfg(any(target_os = "linux", target_os = "android"))]
4550
pub fn do_syncfs(files: Vec<String>) -> UResult<()> {
4651
for path in files {
47-
let f = File::open(path).unwrap();
52+
let f = File::open(&path).map_err_context(|| path.clone())?;
53+
// Reset O_NONBLOCK flag if it was set (matches GNU behavior)
54+
let _ = fcntl(&f, FcntlArg::F_SETFL(OFlag::empty()));
4855
syncfs(f)?;
4956
}
5057
Ok(())
@@ -53,7 +60,9 @@ mod platform {
5360
#[cfg(any(target_os = "linux", target_os = "android"))]
5461
pub fn do_fdatasync(files: Vec<String>) -> UResult<()> {
5562
for path in files {
56-
let f = File::open(path).unwrap();
63+
let f = File::open(&path).map_err_context(|| path.clone())?;
64+
// Reset O_NONBLOCK flag if it was set (matches GNU behavior)
65+
let _ = fcntl(&f, FcntlArg::F_SETFL(OFlag::empty()));
5766
fdatasync(f)?;
5867
}
5968
Ok(())
@@ -157,15 +166,17 @@ mod platform {
157166

158167
pub fn do_syncfs(files: Vec<String>) -> UResult<()> {
159168
for path in files {
160-
flush_volume(
161-
Path::new(&path)
162-
.components()
163-
.next()
164-
.unwrap()
165-
.as_os_str()
166-
.to_str()
167-
.unwrap(),
168-
)?;
169+
let maybe_first = Path::new(&path).components().next();
170+
let vol_name = match maybe_first {
171+
Some(c) => c.as_os_str().to_string_lossy().into_owned(),
172+
None => {
173+
return Err(USimpleError::new(
174+
1,
175+
translate!("sync-error-no-such-file", "file" => path),
176+
));
177+
}
178+
};
179+
flush_volume(&vol_name)?;
169180
}
170181
Ok(())
171182
}

tests/by-util/test_sync.rs

Lines changed: 80 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -90,3 +90,83 @@ fn test_sync_no_permission_file() {
9090
ts.ccmd("chmod").arg("0200").arg(f).succeeds();
9191
ts.ucmd().arg(f).succeeds();
9292
}
93+
94+
#[cfg(any(target_os = "linux", target_os = "android"))]
95+
#[test]
96+
fn test_sync_data_nonblock_flag_reset() {
97+
// Test that O_NONBLOCK flag is properly reset when syncing files
98+
// This verifies the fix in commit 05612a4792ba2a5ad509cf995b91e868fc261229
99+
use uutests::util::TestScenario;
100+
use uutests::util_name;
101+
102+
let ts = TestScenario::new(util_name!());
103+
let at = &ts.fixtures;
104+
let test_file = "test_file.txt";
105+
106+
// Create a test file
107+
at.write(test_file, "test content");
108+
109+
// Run sync --data with the file - should succeed
110+
ts.ucmd().arg("--data").arg(test_file).succeeds();
111+
}
112+
113+
#[cfg(any(target_os = "linux", target_os = "android"))]
114+
#[test]
115+
fn test_sync_fs_nonblock_flag_reset() {
116+
// Test that O_NONBLOCK flag is properly reset when syncing filesystems
117+
use std::fs;
118+
use tempfile::tempdir;
119+
120+
let temporary_directory = tempdir().unwrap();
121+
let temporary_path = fs::canonicalize(temporary_directory.path()).unwrap();
122+
123+
// Run sync --file-system with the path - should succeed
124+
new_ucmd!()
125+
.arg("--file-system")
126+
.arg(&temporary_path)
127+
.succeeds();
128+
}
129+
130+
#[cfg(any(target_os = "linux", target_os = "android"))]
131+
#[test]
132+
fn test_sync_fdatasync_error_handling() {
133+
// Test that fdatasync properly handles file opening errors
134+
// This verifies the error handling fix in commit 05612a4792ba2a5ad509cf995b91e868fc261229
135+
new_ucmd!()
136+
.arg("--data")
137+
.arg("/nonexistent/path/to/file")
138+
.fails()
139+
.stderr_contains("error opening");
140+
}
141+
142+
#[cfg(target_os = "macos")]
143+
#[test]
144+
fn test_sync_syncfs_error_handling_macos() {
145+
// Test that syncfs properly handles invalid paths on macOS
146+
// This verifies the error handling fix in commit 05612a4792ba2a5ad509cf995b91e868fc261229
147+
new_ucmd!()
148+
.arg("--file-system")
149+
.arg("/nonexistent/path/to/file")
150+
.fails()
151+
.stderr_contains("error opening");
152+
}
153+
154+
#[test]
155+
fn test_sync_multiple_files() {
156+
// Test syncing multiple files at once
157+
use std::fs;
158+
use tempfile::tempdir;
159+
160+
let temporary_directory = tempdir().unwrap();
161+
let temp_path = temporary_directory.path();
162+
163+
// Create multiple test files
164+
let file1 = temp_path.join("file1.txt");
165+
let file2 = temp_path.join("file2.txt");
166+
167+
fs::write(&file1, "content1").unwrap();
168+
fs::write(&file2, "content2").unwrap();
169+
170+
// Sync both files
171+
new_ucmd!().arg("--data").arg(&file1).arg(&file2).succeeds();
172+
}

0 commit comments

Comments
 (0)