Skip to content

cksum: get rid of print! and println! to avoid panicking on write errors#12099

Open
RenjiSann wants to merge 2 commits intouutils:mainfrom
RenjiSann:cksum-fix-print
Open

cksum: get rid of print! and println! to avoid panicking on write errors#12099
RenjiSann wants to merge 2 commits intouutils:mainfrom
RenjiSann:cksum-fix-print

Conversation

@RenjiSann
Copy link
Copy Markdown
Collaborator

Fix for #10947

Also needed to do some shenanigans in the uucore::bin macro to avoid having an extra flush failing.

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Apr 30, 2026

GNU testsuite comparison:

GNU test failed: tests/tail/pid-pipe. tests/tail/pid-pipe is passing on 'main'. Maybe you have to rebase?
Skip an intermittent issue tests/cut/bounded-memory (fails in this run but passes in the 'main' branch)
Skipping an intermittent issue tests/date/date-locale-hour (passes in this run but fails in the 'main' branch)
Skipping an intermittent issue tests/tail/symlink (passes in this run but fails in the 'main' branch)
Congrats! The gnu test tests/misc/tty-eof is no longer failing!
Note: The gnu test tests/tail/tail-n0f is now being skipped but was previously passing.
Congrats! The gnu test tests/cp/link-heap is now passing!
Congrats! The gnu test tests/dd/no-allocate is now passing!
Congrats! The gnu test tests/seq/seq-epipe is now passing!

digest_output.write_raw(io::stdout())?;
digest_output
.write_raw(&mut w)
.map_err_context(|| "write error".into())?;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
.map_err_context(|| "write error".into())?;
.map_err_context(|| translate!("checksum-error-write"))?;

string should be localized

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done 👍

I've also created a shorthand function to centralize all calls to this .map_err_context(|| "write error".into()).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess I'd prefer something more idiomatic like:

res.map_err(ChecksumError::Write)

possible implementation:

pub enum ChecksumError {
    ...
    #[error("{}", translate!("common-write-error", "error" => strip_errno(.0)))]
    Write(io::Error),
}

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done 👍

@RenjiSann RenjiSann force-pushed the cksum-fix-print branch 2 times, most recently from 355ddc8 to ad9b814 Compare April 30, 2026 20:51
@codspeed-hq
Copy link
Copy Markdown

codspeed-hq Bot commented Apr 30, 2026

Merging this PR will degrade performance by 16.95%

❌ 1 regressed benchmark
✅ 310 untouched benchmarks
⏩ 46 skipped benchmarks1

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

Mode Benchmark BASE HEAD Efficiency
Simulation hostname_ip_lookup[100000] 90.4 µs 108.8 µs -16.95%

Comparing RenjiSann:cksum-fix-print (7e4e632) with main (843b202)

Open in CodSpeed

Footnotes

  1. 46 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

@RenjiSann
Copy link
Copy Markdown
Collaborator Author

Note that GNU seems to inconsistenly handle whether it shows the No space left on device detail depending on the presence of the -z flag (which replace line breaks by nul bytes).

I assume this is because the way the error is displayed is different between the writes and the final flush that happens in -z mode. I don't think it's relevant to follow this behavior here.

$ uu_cksum /dev/null > /dev/full
cksum: write error: No space left on device

$ gnu_cksum /dev/null > /dev/full       
cksum: write error                               # <-- GNU most probably prints the error differently here

$ uu_cksum /dev/null > /dev/full -z
cksum: write error: No space left on device

$ gnu_cksum /dev/null > /dev/full -z       
cksum: write error: No space left on device

@xtqqczze
Copy link
Copy Markdown
Contributor

$ gnu_cksum /dev/null > /dev/full
cksum: write error # <-- GNU most probably prints the error differently here

maybe you should report that to the coreutils/coreutils repo

return Ok(());
}
// Flush the writer if we didn't write newlines.
w.flush().map_err(|e| ChecksumError::Write(e).into())
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
w.flush().map_err(|e| ChecksumError::Write(e).into())
w.flush().map_err(ChecksumError::Write)?;
Ok(())

nitpick: I'm fine with it either way

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks !

@xtqqczze
Copy link
Copy Markdown
Contributor

some shenanigans in the uucore::bin macro to avoid having an extra flush failing.

Good work with the macros, I'd been wondering whether it was possible to avoid that flush.

}

if options.line_ending != LineEnding::Newline {
// Flush the writer if we didn't write newlines.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess the comment is redundant now

@RenjiSann
Copy link
Copy Markdown
Collaborator Author

$ gnu_cksum /dev/null > /dev/full
cksum: write error # <-- GNU most probably prints the error differently here

maybe you should report that to the coreutils/coreutils repo

Done ! coreutils/coreutils#258

some shenanigans in the uucore::bin macro to avoid having an extra flush failing.

Good work with the macros, I'd been wondering whether it was possible to avoid that flush.

Thanks :)
It's a bit rough but it does not make a too big change. If we need to do the same thing for other binaries, maybe I'll try to find a cleaner way to do it ^^

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