dd: open stdin from file descriptor when possible by jfinkels · Pull Request #4189 · uutils/coreutils

jfinkels · 2022-11-27T22:42:54Z

~~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 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

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.

github-actions · 2022-11-30T08:26:44Z

GNU testsuite comparison:

Congrats! The gnu test tests/dd/skip-seek2 is no longer failing!
GNU test failed: tests/dd/no-allocate. tests/dd/no-allocate is passing on 'main'. Maybe you have to rebase?
GNU test failed: tests/misc/tee. tests/misc/tee is passing on 'main'. Maybe you have to rebase?
GNU test failed: tests/tail-2/inotify-dir-recreate. tests/tail-2/inotify-dir-recreate is passing on 'main'. Maybe you have to rebase?
Congrats! The gnu test tests/dd/no-allocate is no longer ERROR!

github-actions · 2022-12-02T06:57:11Z

GNU testsuite comparison:

Congrats! The gnu test tests/dd/skip-seek2 is no longer failing!
Congrats! The gnu test tests/tail-2/inotify-dir-recreate is no longer failing!
GNU test failed: tests/dd/no-allocate. tests/dd/no-allocate is passing on 'main'. Maybe you have to rebase?
GNU test failed: tests/misc/timeout. tests/misc/timeout is passing on 'main'. Maybe you have to rebase?
Congrats! The gnu test tests/dd/no-allocate is no longer ERROR!

github-actions · 2022-12-02T14:44:55Z

GNU testsuite comparison:

Congrats! The gnu test tests/dd/skip-seek2 is no longer failing!
GNU test failed: tests/dd/no-allocate. tests/dd/no-allocate is passing on 'main'. Maybe you have to rebase?
GNU test failed: tests/tail-2/inotify-dir-recreate. tests/tail-2/inotify-dir-recreate is passing on 'main'. Maybe you have to rebase?
Congrats! The gnu test tests/dd/no-allocate is no longer ERROR!

github-actions · 2023-02-18T21:53:24Z

GNU testsuite comparison:

Congrats! The gnu test tests/dd/skip-seek2 is no longer failing!
GNU test failed: tests/dd/no-allocate. tests/dd/no-allocate is passing on 'main'. Maybe you have to rebase?
GNU test failed: tests/rm/rm2. tests/rm/rm2 is passing on 'main'. Maybe you have to rebase?
Congrats! The gnu test tests/dd/no-allocate is no longer ERROR!

jfinkels · 2023-02-23T02:19:21Z

I rebased this on main now that #4164 has been merged.

github-actions · 2023-02-23T02:34:24Z

GNU testsuite comparison:

Congrats! The gnu test tests/dd/skip-seek2 is no longer failing!
Congrats! The gnu test tests/tail-2/inotify-dir-recreate is no longer failing!

tertsdiepraam · 2023-02-25T11:31:09Z

Could you add a test for this somehow? Maybe by testing the script from the description?

jfinkels · 2023-02-25T19:00:05Z

I'll give it a shot and report back.

tertsdiepraam · 2023-02-26T12:26:14Z

Not sure, but this might help: #4293

jfinkels · 2023-03-05T03:12:44Z

@Joining7943 since you worked on the UCommand implementation recently in #4293, do you know if it provides an obvious way for us to create a test like the following, where there are two invocations of the dd util joined by an &&:

$ printf "abcdef\n" | (dd bs=1 skip=3 count=0 && dd)  2> /dev/null
def

Joining7943 · 2023-03-05T10:39:44Z

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 cmd shell supports the parentheses the same way the sh shell does. Maybe somethink like this?

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();
}

jfinkels · 2023-03-05T18:53:39Z

I'll try that. Thank you!

Joining7943 · 2023-03-06T10:05:17Z

Sure :)

jfinkels · 2023-03-10T15:33:18Z

Rebased on main and added a test.

github-actions · 2023-03-10T17:21:52Z

GNU testsuite comparison:

Congrats! The gnu test tests/dd/skip-seek2 is no longer failing!

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

github-actions · 2023-03-11T21:57:04Z

GNU testsuite comparison:

Congrats! The gnu test tests/dd/skip-seek2 is no longer failing!
GNU test failed: tests/misc/tee. tests/misc/tee is passing on 'main'. Maybe you have to rebase?

jfinkels · 2023-03-11T22:38:53Z

The remaining CI error is due to #4487. Other than that, this is ready for review again

src/uu/dd/src/dd.rs

tertsdiepraam · 2023-03-12T13:01:07Z

src/uu/dd/src/dd.rs

                Err(e) => Err(e),
            },
+            #[cfg(unix)]
+            Self::StdinFile(f) => match io::copy(&mut f.take(n), &mut io::sink()) {


I suppose we can't use file seeking operations here, because stdin doesn't support that even when you open as file?

I didn't try, but that was my assumption.

Is it worth trying? I'm fine with the current implementation too.

I can take a look now and remind myself what happens when using seek(). I'll report back

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.

Alright, thanks!

github-actions · 2023-03-12T14:56:19Z

GNU testsuite comparison:

Congrats! The gnu test tests/dd/skip-seek2 is no longer failing!
GNU test failed: tests/tail-2/inotify-dir-recreate. tests/tail-2/inotify-dir-recreate is passing on 'main'. Maybe you have to rebase?

sylvestre force-pushed the dd-stdin-from-file-descriptor branch from 533d73c to 5a5ca37 Compare November 29, 2022 21:53

jfinkels force-pushed the dd-stdin-from-file-descriptor branch from 5a5ca37 to 11c7f40 Compare December 2, 2022 02:58

sylvestre force-pushed the dd-stdin-from-file-descriptor branch from 11c7f40 to d0d72cd Compare December 2, 2022 09:39

sylvestre force-pushed the dd-stdin-from-file-descriptor branch from d0d72cd to 0ef5112 Compare February 18, 2023 20:24

jfinkels force-pushed the dd-stdin-from-file-descriptor branch from 0ef5112 to 6af72d0 Compare February 23, 2023 01:29

jfinkels marked this pull request as ready for review February 23, 2023 02:18

jfinkels force-pushed the dd-stdin-from-file-descriptor branch from 6af72d0 to f4620f6 Compare March 10, 2023 15:33

jfinkels force-pushed the dd-stdin-from-file-descriptor branch from f4620f6 to 9cb6b4a Compare March 11, 2023 20:56

Merge branch 'main' into dd-stdin-from-file-descriptor

7a6092b

tertsdiepraam reviewed Mar 12, 2023

View reviewed changes

tertsdiepraam approved these changes Mar 17, 2023

View reviewed changes

tertsdiepraam merged commit e3aab30 into uutils:main Mar 18, 2023

This was referenced Mar 18, 2023

dd: open stdout from file descriptor when possible #4542

Closed

dd: fix GNU test 'dd/skip-seek-past-dev' #4527

Merged

jfinkels mentioned this pull request Dec 29, 2024

head: fails to leave remaining bytes of file available for subsequent commands #7028

Closed

Uh oh!

Conversation

jfinkels commented Nov 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Nov 30, 2022

Uh oh!

github-actions bot commented Dec 2, 2022

Uh oh!

github-actions bot commented Dec 2, 2022

Uh oh!

github-actions bot commented Feb 18, 2023

Uh oh!

jfinkels commented Feb 23, 2023

Uh oh!

github-actions bot commented Feb 23, 2023

Uh oh!

tertsdiepraam commented Feb 25, 2023

Uh oh!

jfinkels commented Feb 25, 2023

Uh oh!

tertsdiepraam commented Feb 26, 2023

Uh oh!

jfinkels commented Mar 5, 2023

Uh oh!

Joining7943 commented Mar 5, 2023

Uh oh!

jfinkels commented Mar 5, 2023

Uh oh!

Joining7943 commented Mar 6, 2023

Uh oh!

jfinkels commented Mar 10, 2023

Uh oh!

github-actions bot commented Mar 10, 2023

Uh oh!

github-actions bot commented Mar 11, 2023

Uh oh!

jfinkels commented Mar 11, 2023

Uh oh!

Uh oh!

tertsdiepraam Mar 12, 2023

Choose a reason for hiding this comment

Uh oh!

jfinkels Mar 16, 2023

Choose a reason for hiding this comment

Uh oh!

tertsdiepraam Mar 17, 2023

Choose a reason for hiding this comment

Uh oh!

jfinkels Mar 17, 2023

Choose a reason for hiding this comment

Uh oh!

jfinkels Mar 17, 2023

Choose a reason for hiding this comment

Uh oh!

tertsdiepraam Mar 18, 2023

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Mar 12, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jfinkels commented Nov 27, 2022 •

edited

Loading