dd: open stdout from file descriptor when possible #4542

jfinkels · 2023-03-18T17:13:11Z

(Complements pull request #4189.)

Open stdout using its file descriptor so that a dd skip=N command in a subprocess does not close stdout.

For example, before this commit, multiple instances of dd writing to stdout and appearing in a single command line could incorrectly result in an empty stdout for each instance of dd after the first:

$ printf "abcde\n" | (dd bs=1 skip=1 count=1 && dd bs=1 skip=1)  2> /dev/null
b

After this commit, each dd process writes to the file descriptor referring to stdout:

$ printf "abcde\n" | (dd bs=1 skip=1 count=1 && dd bs=1 skip=1)  2> /dev/null
bde

jfinkels · 2023-03-18T17:23:45Z

src/uu/dd/src/dd.rs

-        None if is_stdout_redirected_to_seekable_file() => {
-            Output::new_file(Path::new(&stdout_canonicalized()), &settings)?
-        }


I believe this workaround is now handled more appropriately in the new_stdout() case.

But the seeking is different now, isn't it? Because StdoutFile copies 0's instead of seeking. Also does it matter that the StdoutFile is only available on unix, while this match expression works on all platforms?

@jfinkels ping ? :)

But the seeking is different now, isn't it? Because StdoutFile copies 0's instead of seeking.

Hmm, yes I guess this removal was premature. It seems like there should be a way to unify these two code paths in a cleaner way, but maybe I'll just postpone that until a later pull request. For example, maybe the decision about whether the destination is seekable could move inside the new_stdout() constructor.

Oh actually, I forgot---if I leave this branch in, then the new test case fails:

$ printf "abcde\n" | (dd bs=1 skip=1 count=1 && dd bs=1 skip=1) 2> /dev/null bde

(this comes from the GNU test suite). For reference, we added the if is_stdout_redirected_to_seekable_file() in pull request #3880.

Let me see if there's a way to reconcile the two.

I just updated this branch to keep the None if is_stdout_redirected_to_seekable_file() => { branch. There seems to be an issue with the new test case I added, but I'm not sure what the problem is.

When I run the test case from the command line I get the expected behavior:

$ printf "abcde\n" | (./target/debug/dd bs=1 skip=1 count=1; ./target/debug/dd bs=1 skip=1) 2> /dev/null bde

but when I run cargo test I get a failure:

$ cargo test test_dd::test_multiple_processes_writing_stdout [...] ---- test_dd::test_multiple_processes_writing_stdout stdout ---- run: /bin/sh -c /home/jeffrey/src/coreutils/target/debug/coreutils printf 'abcde ' | ( /home/jeffrey/src/coreutils/target/debug/coreutils dd bs=1 skip=1 count=1; /home/jeffrey/src/coreutils/target/debug/coreutils dd bs=1 skip=1 ) 2> /dev/null thread 'test_dd::test_multiple_processes_writing_stdout' panicked at 'assertion failed: `(left == right)` Diff < left / right > : <de >bde

tertsdiepraam

All good apart from the comment above, which is more a question than a concern

jfinkels · 2023-05-07T01:55:49Z

I plan to look at this again soon, thanks for the ping

…

------- Original Message -------

On Friday, April 28th, 2023 at 3:34 AM, Sylvestre Ledru ***@***.***> wrote: @sylvestre commented on this pull request. --------------------------------------------------------------- In [src/uu/dd/src/dd.rs](#4542 (comment)): > - None if is_stdout_redirected_to_seekable_file() => { - Output::new_file(Path::new(&stdout_canonicalized()), &settings)? - } ***@***.***(https://github.com/jfinkels) ping ? :) — Reply to this email directly, [view it on GitHub](#4542 (comment)), or [unsubscribe](https://github.com/notifications/unsubscribe-auth/AAA5XG652MODQFJOASC66P3XDNXJ7ANCNFSM6AAAAAAV7SP2XM). You are receiving this because you were mentioned.Message ID: ***@***.***>

github-actions · 2023-05-07T19:23:45Z

GNU testsuite comparison:

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

github-actions · 2023-05-12T09:12:29Z

GNU testsuite comparison:

Congrats! The gnu test tests/dd/nocache is no longer failing!
GNU test failed: tests/dd/nocache_eof. tests/dd/nocache_eof 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?

sylvestre · 2023-05-14T08:29:37Z

Failed with:

running 1 test
run: /bin/sh -c /home/runner/work/coreutils/coreutils/target/debug/coreutils printf 'abcde
' | ( /home/runner/work/coreutils/coreutils/target/debug/coreutils dd bs=1 skip=1 count=1; /home/runner/work/coreutils/coreutils/target/debug/coreutils dd bs=1 skip=1 ) 2> /dev/null
test test_dd::test_multiple_processes_writing_stdout ... FAILED

failures:

failures:
    test_dd::test_multiple_processes_writing_stdout

test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 2676 filtered out; finished in 0.08s

jfinkels · 2023-07-03T01:17:04Z

Yeah I'm stuck here unfortunately, I don't know enough to diagnose the issue. It works as expected when I run the command manually on the command line:

$ printf "abcde\n" | (./target/debug/dd bs=1 skip=1 count=1; ./target/debug/dd bs=1 skip=1) 2> /dev/null
bde

but the same fails when running cargo test. Mentioned originally in a comment here: #4542 (comment)

github-actions · 2023-07-03T02:02:36Z

GNU testsuite comparison:

Skipping an intermittent issue tests/tail-2/inotify-dir-recreate

tertsdiepraam · 2023-07-03T11:58:55Z

I've narrowed this down to UCommand and probably the way we deal with stdout there. It works fine with Command from the standard library.

Here's a theory: we pipe the stdout to a file, what if that file is not opened in append mode, but truncated on every new command that writes to stdout?

Update: it's a bit simpler. This gives de with uutils and bde with GNU:

/bin/sh -c "printf 'abcde\n' | ( dd bs=1 skip=1 count=1; dd bs=1 skip=1 ) 2> /dev/null > a.txt"

So possibly we're being too eager with trying to seek to the start of the file?

Update 2: I think treating the implementation of treating stdout as a file directly is wrong, because the seek, truncate and append defaults are different for piping to stdout. This diff fixes the behaviour for stdout (but obviously break the implementation for normal files). Does that give you enough info to fix it?

@@ -693,7 +693,7 @@ impl<'a> Output<'a> {
             opts.write(true)
                 .create(!cflags.nocreat)
                 .create_new(cflags.excl)
-                .append(oflags.append);
+                .append(true);
 
             #[cfg(any(target_os = "linux", target_os = "android"))]
             if let Some(libc_flags) = make_linux_oflags(oflags) {
@@ -714,9 +714,6 @@ impl<'a> Output<'a> {
         // suppress the error by calling `Result::ok()`. This matches
         // the behavior of GNU `dd` when given the command-line
         // argument `of=/dev/null`.
-        if !settings.oconv.notrunc {
-            dst.set_len(settings.seek).ok();
-        }
         let density = if settings.oconv.sparse {
             Density::Sparse
         } else {

Open stdout using its file descriptor so that a `dd skip=N` command in a subprocess does not close stdout. For example, before this commit, multiple instances of `dd` writing to stdout and appearing in a single command line could incorrectly result in an empty stdout for each instance of `dd` after the first: $ printf "abcde\n" | (dd bs=1 skip=1 count=1 && dd bs=1 skip=1) 2> /dev/null b After this commit, each `dd` process writes to the file descriptor referring to stdout: $ printf "abcde\n" | (dd bs=1 skip=1 count=1 && dd bs=1 skip=1) 2> /dev/null bde

sylvestre · 2023-09-24T08:00:39Z

@jfinkels still interested in finishing this?

jfinkels · 2023-11-27T03:43:03Z

I've narrowed this down to UCommand and probably the way we deal with stdout there. It works fine with Command from the standard library.

I'll update the test to use the standard library Command as it seems a bit more predictable for this test case.

Update 2: I think treating the implementation of treating stdout as a file directly is wrong, because the seek, truncate and append defaults are different for piping to stdout. This diff fixes the behaviour for stdout (but obviously break the implementation for normal files). Does that give you enough info to fix it?

Yes, this is helpful. I'll try to adapt that idea to support all the necessary cases.

sylvestre · 2023-12-25T11:28:29Z

seems that it is stuck - closing to remove it from the list
please reopen if you want to work on it again

jfinkels mentioned this pull request Mar 18, 2023

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

Merged

jfinkels commented Mar 18, 2023

View reviewed changes

tertsdiepraam reviewed Mar 20, 2023

View reviewed changes

jfinkels force-pushed the dd-stdout-from-file-descriptor branch from fc000ea to b613dce Compare May 7, 2023 18:35

jfinkels force-pushed the dd-stdout-from-file-descriptor branch from 454e6da to fad41c2 Compare July 3, 2023 01:14

jfinkels added the help wanted label Jul 3, 2023

sylvestre force-pushed the dd-stdout-from-file-descriptor branch from fad41c2 to c1ba7c9 Compare September 24, 2023 08:00

jfinkels mentioned this pull request Nov 27, 2023

dd fails on tests/dd/not-rewound.sh #3626

Open

jfinkels linked an issue Nov 27, 2023 that may be closed by this pull request

dd fails on tests/dd/not-rewound.sh #3626

Open

sylvestre closed this Dec 25, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dd: open stdout from file descriptor when possible #4542

dd: open stdout from file descriptor when possible #4542

jfinkels commented Mar 18, 2023

jfinkels Mar 18, 2023

tertsdiepraam Mar 20, 2023

sylvestre Apr 28, 2023

jfinkels May 7, 2023

jfinkels May 7, 2023

jfinkels May 7, 2023

tertsdiepraam left a comment

jfinkels commented May 7, 2023 via email

github-actions bot commented May 7, 2023

github-actions bot commented May 12, 2023

sylvestre commented May 14, 2023

jfinkels commented Jul 3, 2023

github-actions bot commented Jul 3, 2023

tertsdiepraam commented Jul 3, 2023 •

edited

Loading

sylvestre commented Sep 24, 2023

jfinkels commented Nov 27, 2023

sylvestre commented Dec 25, 2023

dd: open stdout from file descriptor when possible #4542

dd: open stdout from file descriptor when possible #4542

Conversation

jfinkels commented Mar 18, 2023

jfinkels Mar 18, 2023

Choose a reason for hiding this comment

tertsdiepraam Mar 20, 2023

Choose a reason for hiding this comment

sylvestre Apr 28, 2023

Choose a reason for hiding this comment

jfinkels May 7, 2023

Choose a reason for hiding this comment

jfinkels May 7, 2023

Choose a reason for hiding this comment

jfinkels May 7, 2023

Choose a reason for hiding this comment

tertsdiepraam left a comment

Choose a reason for hiding this comment

jfinkels commented May 7, 2023 via email

github-actions bot commented May 7, 2023

github-actions bot commented May 12, 2023

sylvestre commented May 14, 2023

jfinkels commented Jul 3, 2023

github-actions bot commented Jul 3, 2023

tertsdiepraam commented Jul 3, 2023 • edited Loading

sylvestre commented Sep 24, 2023

jfinkels commented Nov 27, 2023

sylvestre commented Dec 25, 2023

tertsdiepraam commented Jul 3, 2023 •

edited

Loading