-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
dd: open stdin from file descriptor when possible #4189
dd: open stdin from file descriptor when possible #4189
Conversation
533d73c
to
5a5ca37
Compare
GNU testsuite comparison:
|
5a5ca37
to
11c7f40
Compare
GNU testsuite comparison:
|
11c7f40
to
d0d72cd
Compare
GNU testsuite comparison:
|
d0d72cd
to
0ef5112
Compare
GNU testsuite comparison:
|
0ef5112
to
6af72d0
Compare
I rebased this on |
GNU testsuite comparison:
|
Could you add a test for this somehow? Maybe by testing the script from the description? |
I'll give it a shot and report back. |
Not sure, but this might help: #4293 |
@Joining7943 since you worked on the
|
I agree with @tertsdiepraam. The most obvious way I can think of, is to use the shell, although I don't know if the windows use crate::common::util::; // includes the constant TESTS_BINARY
#[cfg(feature = "printf")
#[test]
fn test() {
UCommand::new().arg(format!("{TESTS_BINARY} printf 'abcdef\n' | ( {TESTS_BINARY} dd bs=1 skip=3 count=0 && {TESTS_BINARY} dd ) 2> /dev/null")).run();
// or if you need to pass different arguments to the shell for unix and windows
UCommand::new().arg(
#[cfg(unix)]
// unix arg
#[cfg(windows)]
// windows arg
).run();
} |
I'll try that. Thank you! |
Sure :) |
6af72d0
to
f4620f6
Compare
Rebased on main and added a test. |
GNU testsuite comparison:
|
Open stdin using its file descriptor so that a `dd skip=N` command in a subshell does not consume all bytes from stdin. For example, before this commit, multiple instances of `dd` reading from stdin and appearing in a single command line would incorrectly result in an empty stdin for each instance of `dd` after the first: $ printf "abcdef\n" | (dd bs=1 skip=3 count=0 && dd) 2> /dev/null # incorrectly results in no output After this commit, the `dd skip=3` process reads three bytes from the file descriptor referring to stdin without draining the remaining three bytes when it terminates: $ printf "abcdef\n" | (dd bs=1 skip=3 count=0 && dd) 2> /dev/null def
f4620f6
to
9cb6b4a
Compare
GNU testsuite comparison:
|
The remaining CI error is due to #4487. Other than that, this is ready for review again |
@@ -116,6 +142,15 @@ impl Source { | |||
Ok(m) => Ok(m), | |||
Err(e) => Err(e), | |||
}, | |||
#[cfg(unix)] | |||
Self::StdinFile(f) => match io::copy(&mut f.take(n), &mut io::sink()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose we can't use file seeking operations here, because stdin doesn't support that even when you open as file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't try, but that was my assumption.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it worth trying? I'm fine with the current implementation too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can take a look now and remind myself what happens when using seek()
. I'll report back
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems to cause a bunch of Broken pipe (os error 32)
and Illegal seek
errors throughout the tests if I use f.seek()
here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, thanks!
GNU testsuite comparison:
|
This is a draft pull request because it includes the changes from pull request #4164. Review and merge that one first.This pull request causes
dd
to open stdin using its file descriptor so that add skip=N
command in a subshell does not consume all bytes from stdin.For example, before this commit, multiple instances of
dd
reading from stdin and appearing in a single command line would incorrectly result in an empty stdin for each instance ofdd
after the first:After this commit, the
dd skip=3
process reads three bytes from the file descriptor referring to stdin without draining the remaining three bytes when it terminates:This implementation is inspired by #3662. I don't think this quite resolves #3008 but it is in the right direction. It does seem to get GNU test
tests/dd/skip-seek2.sh
to pass.