Add iflag=fullblock to dd#355
Conversation
rust-coreutils’ dd implementation tends to perform partial writes, which leads to partial reads and data loss as a result. Strictly speaking, without the fullblock option there is no guarantee that full blocks will be read. Even with GNU dd, partial reads and writes can occur, although they hardly ever happen.
Summary of ChangesHello @svlv, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the reliability of Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request aims to improve the reliability of dd operations by using the iflag=fullblock option, which is a commendable goal for ensuring data integrity. However, the current implementation has a critical flaw in its feature detection logic. The test for iflag=fullblock is combined with conv=sync, but these two flags are incompatible in GNU dd. This incompatibility causes the feature detection to fail on systems with GNU dd, meaning the new, improved code path is never executed. I have provided a specific comment with a suggested fix for the test condition to ensure the change works as intended across different environments.
| # Test for ibs, obs and conv feature | ||
| if dd if=/dev/zero of=/dev/null count=1 ibs=512 obs=512 conv=sync 2> /dev/null; then | ||
| # Test for ibs, obs, fullblock and conv feature | ||
| if dd if=/dev/zero of=/dev/null count=1 ibs=512 obs=512 iflag=fullblock conv=sync 2> /dev/null; then |
There was a problem hiding this comment.
The iflag=fullblock and conv=sync options are incompatible in GNU dd. Your test dd ... iflag=fullblock conv=sync will fail on systems with GNU dd by returning a non-zero exit code. Since stderr is redirected to /dev/null, the if condition will silently evaluate to false, and the else branch will be taken. This renders your change to use iflag=fullblock ineffective on platforms using GNU coreutils.
To fix this, you should test for conv=sync and iflag=fullblock in separate dd commands joined by &&.
| if dd if=/dev/zero of=/dev/null count=1 ibs=512 obs=512 iflag=fullblock conv=sync 2> /dev/null; then | |
| if dd if=/dev/zero of=/dev/null count=1 ibs=512 obs=512 conv=sync 2>/dev/null && dd if=/dev/zero of=/dev/null count=0 iflag=fullblock 2>/dev/null; then |
|
Thanks for this. I think Gemini has a good point there, also wondering why the NetBSD tests are failing. There's a possibility this is unrelated as the VM for this platform has been pretty flaky for a while, though it seems to actually fail with checksums not matching. |
|
Hm, I haven't found any incompatibility with using both I'll try to check what can be wrong on NetBSD. |
|
I’ve set up a fresh VM running NetBSD 10.1 and managed to reproduce the issue.
There is a fallback in MS_dd(), so this case should be handled correctly. However, I then realized that MS_dd() is not used by default; it is only used when the --noprogress option is passed. When running with this option, MS_dd() is used and the test passes on NetBSD. Currently installation on NetBSD is broken by this change svlv@51e7299. After reverting it the test passed. Anyway, I’ll slightly modify my proposed changes. |
|
Seems dd on NetBSD doesn't work as expected when count=0 is passed |
|
Tests are still failing on NetBSD... |
Yes, as I mentioned previously, tests are failing on NetBSD due to the fix 51e7299. After reverting it, the test passed on my VM running NetBSD.
4 bytes transferred, not 3, as it should be. So, it is not related to this PR. |
There was a problem hiding this comment.
This PR is being reviewed by Cursor Bugbot
Details
You are on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
|
@svlv Good point, I have reverted that commit and everything passes now. |
|
From my point of view, that fix was correct, however, it does not work with dd on NetBSD. |
|
From my point of view, I'd rather maximize the compatibility across all platforms, as evidenced by the test results, so unless we can add a test that can prove where that fix made an actual difference instead of just breaking NetBSD support, I'd rather keep it that way. I'll merge your PR. |
rust-coreutils’ dd implementation tends to perform partial writes, which leads to partial reads and data loss as a result. Strictly speaking, without the fullblock option there is no guarantee that full blocks will be read. Even with GNU dd, partial reads and writes can occur, although they hardly ever happen.
Note
Improves archive extraction reliability by using
dd’s full-block reads when supported.MS_dd, detect support foriflag=fullblockand, if available, pass it to theddinvocation used for block copyingiflag=fullblockisn’t supportedWritten by Cursor Bugbot for commit f37330b. Configure here.