Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tee: Correctly handle read-only files, avoid unnecessary wrapping #6157

Merged
merged 3 commits into from
Mar 31, 2024

Conversation

BenWiederhake
Copy link
Collaborator

This PR mainly fixes the handling of writing to existing read-only files, and adds test_readonly().

As a drive-by improvement, this PR also removes some weird wrapping that seems to be unintentional leftovers from history. In short, we used to have a NamedWriter that contained a NamedWriter that contained a NamedWriter that contained the actual file. The run-time impact should be negligible, but removing it makes the code a bit easier to read and edit.

The inspiration for this PR was working towards fixing the GNU test misc/tee.log. Note that we still don't fully pass this test because we don't implement "iopoll-powered early exit for closed pipes".

This removes two layers of Box<NamedWriter<...>>.
The indirection appears to be unintentional, and makes it harder to
understand and change the code.
src/uu/tee/src/tee.rs Outdated Show resolved Hide resolved
@BenWiederhake
Copy link
Collaborator Author

Changes since last push:

-    let path = PathBuf::from(name.clone());
+    let path = PathBuf::from(name);

As suggested by clippy stable, clippy nightly, and @sylvestre :D

Copy link

GNU testsuite comparison:

Skipping an intermittent issue tests/tail/inotify-dir-recreate (passes in this run but fails in the 'main' branch)

@BenWiederhake BenWiederhake marked this pull request as draft March 31, 2024 16:42
@BenWiederhake BenWiederhake marked this pull request as ready for review March 31, 2024 16:47
@BenWiederhake
Copy link
Collaborator Author

Changes since last push:

  • The Windows string for EPERM seems to be Access is denied. It's a weird phrase, bur I don't want to hard-code error strings in the program, so I adapted the test to use a regex instead of a fixed string.

Copy link

GNU testsuite comparison:

Skipping an intermittent issue tests/tail/inotify-dir-recreate (passes in this run but fails in the 'main' branch)

@sylvestre sylvestre merged commit 4482a62 into uutils:main Mar 31, 2024
62 checks passed
@BenWiederhake BenWiederhake deleted the dev-tee-fail-open branch March 31, 2024 23:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants