Conversation
|
GNU testsuite comparison: |
src/uu/sync/src/sync.rs
Outdated
| #[cfg(any(target_os = "linux", target_os = "android"))] | ||
| fn open_and_reset_nonblock(path: &str) -> UResult<File> { | ||
| let f = File::open(path).map_err_context(|| path.to_string())?; | ||
| let f = open_file(path.as_ref(), true).map_err_context(|| path.to_string())?; |
There was a problem hiding this comment.
I'm sorry, now that I'm testing the implementation with tail is not passing a bunch of use cases that I was thinking of and I think we won't actually be able to share the tail implementation. I'm going to post those use cases soon
There was a problem hiding this comment.
Ideally we should make the cases that we are different from the gnu implementation into integration test cases
There was a problem hiding this comment.
These are the main issues I see, because some of these behaviors are different than tail it looks like we are unable to use the same file:
#Write-only file uutils fails
echo test > /tmp/t && chmod 200 /tmp/t && sync --data /tmp/t
# FIFO without flags uutils does global sync (ignores file)
mkfifo /tmp/f && timeout 2 sync /tmp/f
# FIFO --data — both error, but different messages
mkfifo /tmp/f && timeout 2 sync --data /tmp/f
# GNU: "sync: error syncing '/tmp/f': Invalid argument"
# uutils: "sync: Invalid input"
There was a problem hiding this comment.
Maybe just for now can we start with just replacing this with:
let file = match OpenOptions::new()
.read(true)
.custom_flags(libc::O_NONBLOCK)
.open(path)
and adding the integration test for the use case in the issue?
There was a problem hiding this comment.
Thanks, will do. I couldn't find a good common place as well. feature::fs was my first thought, but that's feature gated and I wasn't too sure about forcing sync to have that feature as well.
reubenwong97
force-pushed
the
patch/sync-timeout
branch
from
2d9032f to
0a7b16c
Compare
|
GNU testsuite comparison: |
reubenwong97
changed the title
|
Hi @ChrisDryden , |
src/uu/sync/Cargo.toml
Outdated
| path = "src/sync.rs" | ||
|
|
||
| [dependencies] | ||
| libc = { workspace = true } |
There was a problem hiding this comment.
Sorry for the nits, should be good to go right after this but can you use nix:libc so that we don't have to add a new dependency here?
There was a problem hiding this comment.
Thanks for pointing this out, removed the extra dependency
src/uu/sync/src/sync.rs
Outdated
| let f = OpenOptions::new() | ||
| .read(true) | ||
| .custom_flags(libc::O_NONBLOCK) | ||
| .open(path)?; |
There was a problem hiding this comment.
Whoops missed this, the error message changed here?
.map_err_context(|| path.to_string())
Mind adding the error message to the integration test so we don't regress?
There was a problem hiding this comment.
Added new messages to the locales and mapped_err_context to the syncfs and fdatasync calls so we can get:
❯ timeout 2 ~/code/projects/rust/coreutils/target/release/coreutils sync --data /tmp/testfifo
sync: error syncing '/tmp/testfifo': Invalid inputinstead of just:
timeout 2 ~/code/projects/rust/coreutils/target/release/coreutils sync --data /tmp/testfifo
sync: Invalid inputUpdated the test to reflect this.
|
GNU testsuite comparison: |
3 participants
Closes #10736
open_file in tail opened files with the correct flags for fifo. it has been moved to uucore::fs, and re-used in sync. invalid command now exits immediately.