dd: allow skipping and seeking in FIFOs #4164

jfinkels · 2022-11-19T17:14:53Z

First, this pull request creates a Source enum and simplifies the Input struct in dd, mirroring pull request #4134 that introduced the Dest enum and a simplified Output struct. Second, this pull request adds support seeking in FIFOs given as either input or output (or both). For example, dd seek=1 of=fifo and dd skip=1 if=fifo will now work.

This should cause GNU test suite file tests/dd/no-allocate.sh to change from ERROR to FAIL status.

Fixes #3321.

github-actions · 2022-11-19T17:56:20Z

GNU testsuite comparison:

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

jfinkels · 2022-11-19T18:23:01Z

Well, there's a few platform-compatibility issues I'll need to work out, but this is good news:

Congrats! The gnu test tests/dd/no-allocate is no longer ERROR!

github-actions · 2022-11-19T18:33:57Z

GNU testsuite comparison:

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

github-actions · 2022-11-19T21:19:29Z

GNU testsuite comparison:

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

github-actions · 2022-11-20T18:30:40Z

GNU testsuite comparison:

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

github-actions · 2022-11-20T18:30:43Z

GNU testsuite comparison:

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

github-actions · 2022-11-20T20:55:24Z

GNU testsuite comparison:

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

tertsdiepraam

Love where this is going!

src/uu/dd/src/dd.rs

jfinkels · 2022-11-21T01:21:15Z

Looks like something is not working right on macos:

2022-11-20T20:44:52.4512090Z test test_dd::test_seek_output_fifo has been running for over 60 seconds
2022-11-20T20:44:53.4303380Z test test_dd::test_skip_input_fifo has been running for over 60 seconds

What do you think about just skipping those two tests on macos and opening a new issue requesting an implementation that works on macos? I don't have a Mac computer so it's hard for me to debug these issues.

github-actions · 2022-11-21T02:22:02Z

GNU testsuite comparison:

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

src/uu/dd/src/dd.rs

jfinkels · 2022-11-27T17:31:08Z

tests/by-util/test_dd.rs

+        .succeeds()
+        .stderr_only("0+0 records in\n0+0 records out\n");
+
+    let output = child.wait_with_output().unwrap();


If pull request #4136 gets merged, that would allow me to make these tests more concise.

That PR is now merged, do you still want to do this in this PR?

I think I was wrong when I made this comment. The child process comes from Command, not UCommand, so I don't think any changes can be made here at the moment.

You could use ts.cmd("dd") to make it a UCommand (which will still call the system dd, not uutils), but it's not a high priority.

jfinkels · 2022-11-27T22:44:51Z

I tried getting the macos build to succeed using @tertsdiepraam's suggestion, but it doesn't seem to work.

github-actions · 2022-11-30T13:53:41Z

GNU testsuite comparison:

GNU test failed: tests/dd/no-allocate. tests/dd/no-allocate 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:56:59Z

GNU testsuite comparison:

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?
Congrats! The gnu test tests/dd/no-allocate is no longer ERROR!

github-actions · 2022-12-04T05:49:39Z

GNU testsuite comparison:

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 · 2022-12-04T16:44:34Z

GNU testsuite comparison:

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?
Congrats! The gnu test tests/dd/no-allocate is no longer ERROR!

github-actions · 2022-12-06T03:48:44Z

GNU testsuite comparison:

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 · 2022-12-06T04:58:45Z

GNU testsuite comparison:

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

tertsdiepraam

I think the dd_out should also just become a free function dd, but that's not high priority. This cleans up so much already.

tertsdiepraam · 2022-12-07T20:27:04Z

tests/by-util/test_dd.rs

+        .succeeds()
+        .stderr_only("0+0 records in\n0+0 records out\n");
+
+    let output = child.wait_with_output().unwrap();


That PR is now merged, do you still want to do this in this PR?

jfinkels · 2022-12-07T22:32:09Z

I think the dd_out should also just become a free function dd, but that's not high priority.

Great minds think alike; I have such a branch on my local machine but it depends on this branch, so I was waiting until this is merged.

github-actions · 2022-12-07T23:28:57Z

GNU testsuite comparison:

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!

jfinkels · 2022-12-08T01:00:38Z

I skipped the tests for this feature on macos and freebsd because I wasn't sure how to make the implementation work.

This mirrors a recent commit that introduced the `Dest` enum and a simplified `Output` struct. These changes allow us to add new types of inputs and output more easily.

For example, `dd seek=1 of=fifo` will now work.

For example, `dd skip=1 if=fifo` will now work.

github-actions · 2023-02-18T21:55:07Z

GNU testsuite comparison:

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?
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!

tertsdiepraam

Excellent! Just one nit

tertsdiepraam · 2023-02-18T22:35:09Z

src/uu/dd/src/dd.rs

+        if settings.skip > 0 {
+            src.skip(settings.skip)?;
+        }


Would it make sense to extract this? It appears in all the new_* functions.

Do you mean to factor these three lines out of the new_* functions into a new helper function and call that function in each of the three places? If so, I don't think it offers much benefit but I can do that if you it seems better to you.

Or do you mean to move this functionality out to the calling code so that it appears after the call to new_*()? I think I tried a design like that when I was first working on this pull request, but I no longer remember if it worked :).

Or do you mean to move this functionality out to the calling code so that it appears after the call to new_*()?

I meant this one, but it's not that important. Let's just merge this :)

jfinkels force-pushed the dd-fifo-seek branch from a389752 to 058d2c2 Compare November 20, 2022 19:36

tertsdiepraam reviewed Nov 21, 2022

View reviewed changes

src/uu/dd/src/dd.rs Outdated Show resolved Hide resolved

tertsdiepraam reviewed Nov 21, 2022

View reviewed changes

src/uu/dd/src/dd.rs Outdated Show resolved Hide resolved

jfinkels commented Nov 27, 2022

View reviewed changes

jfinkels mentioned this pull request Nov 27, 2022

dd: open stdin from file descriptor when possible #4189

Merged

sylvestre force-pushed the dd-fifo-seek branch from 7838177 to 641a49c Compare November 29, 2022 21:54

jfinkels force-pushed the dd-fifo-seek branch from db0ef95 to d121c76 Compare December 2, 2022 02:54

sylvestre requested a review from tertsdiepraam December 3, 2022 09:21

jfinkels force-pushed the dd-fifo-seek branch from d121c76 to 52b6ac0 Compare December 4, 2022 05:04

jfinkels added the help wanted label Dec 4, 2022

jfinkels force-pushed the dd-fifo-seek branch from 52b6ac0 to ba95ef4 Compare December 4, 2022 15:44

jfinkels force-pushed the dd-fifo-seek branch from ec77151 to c09dbb1 Compare December 6, 2022 03:58

tertsdiepraam approved these changes Dec 7, 2022

View reviewed changes

jfinkels force-pushed the dd-fifo-seek branch from c09dbb1 to 568a225 Compare December 7, 2022 22:39

jfinkels added 3 commits February 18, 2023 21:24

dd: create Source enum and simpler Input struct

cf72c99

This mirrors a recent commit that introduced the `Dest` enum and a simplified `Output` struct. These changes allow us to add new types of inputs and output more easily.

dd: add support for seeking in output FIFOs

9c3f810

For example, `dd seek=1 of=fifo` will now work.

dd: add support for skipping in input FIFO

c52647a

For example, `dd skip=1 if=fifo` will now work.

sylvestre force-pushed the dd-fifo-seek branch from 568a225 to c52647a Compare February 18, 2023 20:24

sylvestre requested a review from tertsdiepraam February 18, 2023 20:24

tertsdiepraam approved these changes Feb 18, 2023

View reviewed changes

tertsdiepraam merged commit 60d2df5 into uutils:main Feb 22, 2023

jfinkels mentioned this pull request Jul 4, 2023

Get remaining GNU test cases from ERROR to at least FAIL #3320

Closed

11 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dd: allow skipping and seeking in FIFOs #4164

dd: allow skipping and seeking in FIFOs #4164

jfinkels commented Nov 19, 2022 •

edited

Loading

github-actions bot commented Nov 19, 2022

jfinkels commented Nov 19, 2022

github-actions bot commented Nov 19, 2022

github-actions bot commented Nov 19, 2022

github-actions bot commented Nov 20, 2022

github-actions bot commented Nov 20, 2022

github-actions bot commented Nov 20, 2022

tertsdiepraam left a comment

jfinkels commented Nov 21, 2022

github-actions bot commented Nov 21, 2022

jfinkels Nov 27, 2022

tertsdiepraam Dec 7, 2022

jfinkels Dec 7, 2022

tertsdiepraam Dec 13, 2022 •

edited

Loading

jfinkels commented Nov 27, 2022

github-actions bot commented Nov 30, 2022

github-actions bot commented Dec 2, 2022

github-actions bot commented Dec 4, 2022

github-actions bot commented Dec 4, 2022

github-actions bot commented Dec 6, 2022

github-actions bot commented Dec 6, 2022

tertsdiepraam left a comment

tertsdiepraam Dec 7, 2022

jfinkels commented Dec 7, 2022

github-actions bot commented Dec 7, 2022

jfinkels commented Dec 8, 2022

github-actions bot commented Feb 18, 2023

tertsdiepraam left a comment

tertsdiepraam Feb 18, 2023

jfinkels Feb 22, 2023

tertsdiepraam Feb 22, 2023 •

edited

Loading

dd: allow skipping and seeking in FIFOs #4164

dd: allow skipping and seeking in FIFOs #4164

Conversation

jfinkels commented Nov 19, 2022 • edited Loading

github-actions bot commented Nov 19, 2022

jfinkels commented Nov 19, 2022

github-actions bot commented Nov 19, 2022

github-actions bot commented Nov 19, 2022

github-actions bot commented Nov 20, 2022

github-actions bot commented Nov 20, 2022

github-actions bot commented Nov 20, 2022

tertsdiepraam left a comment

Choose a reason for hiding this comment

jfinkels commented Nov 21, 2022

github-actions bot commented Nov 21, 2022

jfinkels Nov 27, 2022

Choose a reason for hiding this comment

tertsdiepraam Dec 7, 2022

Choose a reason for hiding this comment

jfinkels Dec 7, 2022

Choose a reason for hiding this comment

tertsdiepraam Dec 13, 2022 • edited Loading

Choose a reason for hiding this comment

jfinkels commented Nov 27, 2022

github-actions bot commented Nov 30, 2022

github-actions bot commented Dec 2, 2022

github-actions bot commented Dec 4, 2022

github-actions bot commented Dec 4, 2022

github-actions bot commented Dec 6, 2022

github-actions bot commented Dec 6, 2022

tertsdiepraam left a comment

Choose a reason for hiding this comment

tertsdiepraam Dec 7, 2022

Choose a reason for hiding this comment

jfinkels commented Dec 7, 2022

github-actions bot commented Dec 7, 2022

jfinkels commented Dec 8, 2022

github-actions bot commented Feb 18, 2023

tertsdiepraam left a comment

Choose a reason for hiding this comment

tertsdiepraam Feb 18, 2023

Choose a reason for hiding this comment

jfinkels Feb 22, 2023

Choose a reason for hiding this comment

tertsdiepraam Feb 22, 2023 • edited Loading

Choose a reason for hiding this comment

jfinkels commented Nov 19, 2022 •

edited

Loading

tertsdiepraam Dec 13, 2022 •

edited

Loading

tertsdiepraam Feb 22, 2023 •

edited

Loading