dd: create Dest enum and simpler Output struct #4134
Conversation
jfinkels
force-pushed
the
dd-output-enums
branch
from
0ed8767 to
dfc5e31
Compare
There was a problem hiding this comment.
Cool! I like this already without the fifo variant because it feels much simpler than the current solution. I would only suggest to make the sparse option a bit more explicit. For example:
enum Dest {
Stdout(Stdout),
File { handle: File, sparse: bool },
}
// Or
enum FileWriteMode {
Dense,
Sparse,
}
enum Dest {
Stdout(Stdout),
File(File, FileWriteMode),
}(There must be a better name than FileWriteMode, but I can't think of one right now)
jfinkels
force-pushed
the
dd-output-enums
branch
from
dfc5e31 to
6f9194c
Compare
tertsdiepraam
left a comment
tertsdiepraam
left a comment
There was a problem hiding this comment.
I'd like to see Input get a similar makeover (not in this PR though). Are you planning to do that? if not then I could pick that up.
I did that on my local machine but I ran into an issue and didn't include it here. The issue is that two of the tests at the bottom of the I will have a similar need to have a |
jfinkels
force-pushed
the
dd-output-enums
branch
from
6f9194c to
a2f54b5
Compare
|
GNU testsuite comparison: |
sylvestre
force-pushed
the
dd-output-enums
branch
from
a2f54b5 to
4621557
Compare
|
GNU testsuite comparison: |
That's not great, I think we should try to rewrite those tests (if that's possible) Edit: It's just two small tests, IMO those shouldn't block this change and can be removed, although it would be even better if you can fine some other way to write them of course. |
I've found a way to re-write the tests, so I'm working on that now. I think it might be simplest to just produce a separate pull request that gives the |
2 participants
This change uses an enum rather than a trait to represent the possible data sources for
dd. This is a refactor only, it does not change any behavior ofdd. The purpose of this change is to prepare for a future pull request that will introduce a new enum variantDest::Fifoto represent writing to a named pipe on Unix systems. The wayddworks for named pipes is different from the way it works for regular files or stdout. For example, since a named pipe is not seekable, aseek=1option should actually cause a read of one block, rather than a seek as in a regular file, or a write as in stdout. This is all to try and gettests/dd/no-allocate.shin the GNU test suite passing.Anyway, you can ignore all of that stuff about named pipes to review this pull request, but it provides my motivation for this change. Hopefully this change makes the code easier to read as well.