touch: use write-open + futimens on Unix to trigger IN_CLOSE_WRITE#9813
touch: use write-open + futimens on Unix to trigger IN_CLOSE_WRITE#9813mattsu2020 wants to merge 1 commit intouutils:mainfrom
Conversation
|
is it possible to add tests ? thanks |
add |
|
sorry, it needs to be rebased |
|
GNU testsuite comparison: |
|
GNU testsuite comparison: |
Merging this PR will improve performance by 3.42%
Performance Changes
Comparing Footnotes
|
|
GNU testsuite comparison: |
src/uu/touch/src/touch.rs
Outdated
| let mtime_spec = TimeSpec::new(mtime.unix_seconds() as _, mtime.nanoseconds() as _); | ||
|
|
||
| futimens(&file, &atime_spec, &mtime_spec) | ||
| .map_err(|err| std::io::Error::from_raw_os_error(err as i32)) |
There was a problem hiding this comment.
he conversion from nix::Error to std::io::Error is incorrect
| #[cfg(unix)] | ||
| { | ||
| // Open write-only and use futimens to trigger IN_CLOSE_WRITE on Linux. | ||
| if !is_stdout && try_futimens_via_write_fd(path, atime, mtime).is_ok() { |
There was a problem hiding this comment.
Silent error swallowing with is_ok() loses information about why futimens failed.
src/uu/touch/src/touch.rs
Outdated
| /// access and modification times on the open FD (not by path), which also | ||
| /// triggers `IN_CLOSE_WRITE` on Linux when the FD is closed. | ||
| fn try_futimens_via_write_fd(path: &Path, atime: FileTime, mtime: FileTime) -> std::io::Result<()> { | ||
| let metadata = fs::metadata(path)?; |
There was a problem hiding this comment.
Unnecessary metadata check before open. Opens file twice (metadata + open). Should check metadata after opening via file.metadata() to avoid TOCTOU and reduce syscalls.
src/uu/touch/src/touch.rs
Outdated
| fn try_futimens_via_write_fd(path: &Path, atime: FileTime, mtime: FileTime) -> std::io::Result<()> { | ||
| let metadata = fs::metadata(path)?; | ||
| if !metadata.is_file() { | ||
| return Err(Error::other("not a regular file")); |
src/uu/touch/src/touch.rs
Outdated
| } | ||
|
|
||
| let file = OpenOptions::new().write(true).open(path)?; | ||
| let atime_spec = TimeSpec::new(atime.unix_seconds() as _, atime.nanoseconds() as _); |
There was a problem hiding this comment.
Unsafe type cast 'as _' for nanoseconds. Should explicitly cast to i32: atime.nanoseconds() as i32
|
GNU testsuite comparison: |
src/uu/touch/src/touch.rs
Outdated
| uu_app, | ||
| }; | ||
|
|
||
| #[cfg(unix)] |
There was a problem hiding this comment.
Redundant import - std::fs is already imported unconditionally at line 26
|
GNU testsuite comparison: |
|
GNU testsuite comparison: |
|
GNU testsuite comparison: |
d741ba8 to
8e88196
Compare
|
GNU testsuite comparison: |
On Unix, try futimens through a write-opened file descriptor to trigger IN_CLOSE_WRITE semantics, and fall back to set_file_times when needed. Also adds a unit test for the futimens path and wires unix-only dependencies.
8e88196 to
d0822d6
Compare
|
GNU testsuite comparison: |
Summary
Make
touchopen regular files for write and callfutimenson the fd (Unix only). This matches GNU touch behavior and ensures inotify emitsIN_CLOSE_WRITEwhen timestamps are updated.Motivation
Some inotify-based reloaders watch only
IN_CLOSE_WRITE. With the current path-based timestamp update,touchemits onlyIN_ATTRIB, so those watchers miss the change. This caused infinite loops in downstream tests (e.g., 0 A.D. hotload tests on Ubuntu Questing).Changes
OpenOptions::new().write(true)and callfutimenson the fd for regular files.set_file_timespath-based update when open/futimens fails or the target is not a regular file.Testing
cargo test -p uu_touchNotes
related
#9812