Skip to content
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 fix gnu test not-rewound.sh #6088

Merged
merged 1 commit into from
Mar 17, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions src/uu/dd/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -20,11 +20,9 @@ gcd = { workspace = true }
libc = { workspace = true }
uucore = { workspace = true, features = ["format", "quoting-style"] }

[target.'cfg(any(target_os = "linux"))'.dependencies]
nix = { workspace = true, features = ["fs"] }

[target.'cfg(any(target_os = "linux", target_os = "android"))'.dependencies]
signal-hook = { workspace = true }
nix = { workspace = true, features = ["fs"] }

[[bin]]
name = "dd"
Expand Down
59 changes: 52 additions & 7 deletions src/uu/dd/src/dd.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
// For the full copyright and license information, please view the LICENSE
// file that was distributed with this source code.

// spell-checker:ignore fname, ftype, tname, fpath, specfile, testfile, unspec, ifile, ofile, outfile, fullblock, urand, fileio, atoe, atoibm, behaviour, bmax, bremain, cflags, creat, ctable, ctty, datastructures, doesnt, etoa, fileout, fname, gnudd, iconvflags, iseek, nocache, noctty, noerror, nofollow, nolinks, nonblock, oconvflags, oseek, outfile, parseargs, rlen, rmax, rremain, rsofar, rstat, sigusr, wlen, wstat seekable oconv canonicalized fadvise Fadvise FADV DONTNEED ESPIPE bufferedoutput
// spell-checker:ignore fname, ftype, tname, fpath, specfile, testfile, unspec, ifile, ofile, outfile, fullblock, urand, fileio, atoe, atoibm, behaviour, bmax, bremain, cflags, creat, ctable, ctty, datastructures, doesnt, etoa, fileout, fname, gnudd, iconvflags, iseek, nocache, noctty, noerror, nofollow, nolinks, nonblock, oconvflags, oseek, outfile, parseargs, rlen, rmax, rremain, rsofar, rstat, sigusr, wlen, wstat seekable oconv canonicalized fadvise Fadvise FADV DONTNEED ESPIPE bufferedoutput, SETFL

mod blocks;
mod bufferedoutput;
Expand All @@ -16,8 +16,13 @@
use crate::bufferedoutput::BufferedOutput;
use blocks::conv_block_unblock_helper;
use datastructures::*;
#[cfg(any(target_os = "linux", target_os = "android"))]
use nix::fcntl::FcntlArg::F_SETFL;
#[cfg(any(target_os = "linux", target_os = "android"))]
use nix::fcntl::OFlag;
use parseargs::Parser;
use progress::{gen_prog_updater, ProgUpdate, ReadStat, StatusLevel, WriteStat};
use uucore::io::OwnedFileDescriptorOrHandle;

use std::cmp;
use std::env;
Expand All @@ -31,6 +36,8 @@
fs::FileTypeExt,
io::{AsRawFd, FromRawFd},
};
#[cfg(windows)]
use std::os::windows::{fs::MetadataExt, io::AsHandle};
use std::path::Path;
use std::sync::{
atomic::{AtomicBool, Ordering::Relaxed},
Expand Down Expand Up @@ -227,7 +234,7 @@
Err(e) => Err(e),
}
}
Self::File(f) => f.seek(io::SeekFrom::Start(n)),
Self::File(f) => f.seek(io::SeekFrom::Current(n.try_into().unwrap())),
#[cfg(unix)]
Self::Fifo(f) => io::copy(&mut f.take(n), &mut io::sink()),
}
Expand Down Expand Up @@ -283,7 +290,24 @@
/// Instantiate this struct with stdin as a source.
fn new_stdin(settings: &'a Settings) -> UResult<Self> {
#[cfg(not(unix))]
let mut src = Source::Stdin(io::stdin());
let mut src = {
let f = File::from(io::stdin().as_handle().try_clone_to_owned()?);
let is_file = if let Ok(metadata) = f.metadata() {
// this hack is needed as there is no other way on windows
// to differentiate between the case where `seek` works
// on a file handle or not. i.e. when the handle is no real
// file but a pipe, `seek` is still successful, but following
// `read`s are not affected by the seek.
metadata.creation_time() != 0
} else {
false
};
if is_file {
Source::File(f)
} else {
Source::Stdin(io::stdin())
}
};
#[cfg(unix)]
let mut src = Source::stdin_as_file();
if settings.skip > 0 {
Expand Down Expand Up @@ -557,7 +581,7 @@
return Ok(len);
}
}
f.seek(io::SeekFrom::Start(n))
f.seek(io::SeekFrom::Current(n.try_into().unwrap()))
}
#[cfg(unix)]
Self::Fifo(f) => {
Expand Down Expand Up @@ -699,6 +723,11 @@
if !settings.oconv.notrunc {
dst.set_len(settings.seek).ok();
}

Self::prepare_file(dst, settings)
}

fn prepare_file(dst: File, settings: &'a Settings) -> UResult<Self> {
let density = if settings.oconv.sparse {
Density::Sparse
} else {
Expand All @@ -710,6 +739,24 @@
Ok(Self { dst, settings })
}

/// Instantiate this struct with file descriptor as a destination.
///
/// This is useful e.g. for the case when the file descriptor was
/// already opened by the system (stdout) and has a state
/// (current position) that shall be used.
fn new_file_from_stdout(settings: &'a Settings) -> UResult<Self> {
let fx = OwnedFileDescriptorOrHandle::from(io::stdout())?;
#[cfg(any(target_os = "linux", target_os = "android"))]
if let Some(libc_flags) = make_linux_oflags(&settings.oflags) {
nix::fcntl::fcntl(
fx.as_raw().as_raw_fd(),
F_SETFL(OFlag::from_bits_retain(libc_flags)),
)?;

Check warning on line 754 in src/uu/dd/src/dd.rs

View check run for this annotation

Codecov / codecov/patch

src/uu/dd/src/dd.rs#L754

Added line #L754 was not covered by tests
}

Self::prepare_file(fx.into_file(), settings)
}

/// Instantiate this struct with the given named pipe as a destination.
#[cfg(unix)]
fn new_fifo(filename: &Path, settings: &'a Settings) -> UResult<Self> {
Expand Down Expand Up @@ -1287,9 +1334,7 @@
#[cfg(unix)]
Some(ref outfile) if is_fifo(outfile) => Output::new_fifo(Path::new(&outfile), &settings)?,
Some(ref outfile) => Output::new_file(Path::new(&outfile), &settings)?,
None if is_stdout_redirected_to_seekable_file() => {
Output::new_file(Path::new(&stdout_canonicalized()), &settings)?
}
None if is_stdout_redirected_to_seekable_file() => Output::new_file_from_stdout(&settings)?,
None => Output::new_stdout(&settings)?,
};
dd_copy(i, o).map_err_context(|| "IO error".to_string())
Expand Down
1 change: 1 addition & 0 deletions src/uucore/src/lib/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ pub use uucore_procs::*;
// * cross-platform modules
pub use crate::mods::display;
pub use crate::mods::error;
pub use crate::mods::io;
pub use crate::mods::line_ending;
pub use crate::mods::os;
pub use crate::mods::panic;
Expand Down
1 change: 1 addition & 0 deletions src/uucore/src/lib/mods.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

pub mod display;
pub mod error;
pub mod io;
pub mod line_ending;
pub mod os;
pub mod panic;
Expand Down
98 changes: 98 additions & 0 deletions src/uucore/src/lib/mods/io.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,98 @@
// This file is part of the uutils coreutils package.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you please document it?
As it will be part of the API like:
https://docs.rs/uucore/0.0.24/uucore/error/index.html

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thats cool.
I added some documentation.
Thanks.

//
// For the full copyright and license information, please view the LICENSE
// file that was distributed with this source code.

/// Encapsulates differences between OSs regarding the access to
/// file handles / descriptors.
/// This is useful when dealing with lower level stdin/stdout access.
///
/// In detail:
/// On unix like OSs, file _descriptors_ are used in this context.
/// On windows OSs, file _handles_ are used.
///
/// Even though they are distinct classes, they share common functionality.
/// Access to this common functionality is provided in `OwnedFileDescriptorOrHandle`.

#[cfg(not(windows))]
use std::os::fd::{AsFd, OwnedFd};
#[cfg(windows)]
use std::os::windows::io::{AsHandle, OwnedHandle};
use std::{
fs::{File, OpenOptions},
io,
path::Path,
process::Stdio,
};

#[cfg(windows)]
type NativeType = OwnedHandle;
#[cfg(not(windows))]
type NativeType = OwnedFd;

/// abstraction wrapper for native file handle / file descriptor
pub struct OwnedFileDescriptorOrHandle {
fx: NativeType,
}

impl OwnedFileDescriptorOrHandle {
/// create from underlying native type
pub fn new(x: NativeType) -> Self {
Self { fx: x }
}

/// create by opening a file
pub fn open_file(options: &OpenOptions, path: &Path) -> io::Result<Self> {
let f = options.open(path)?;
Self::from(f)
}

/// conversion from borrowed native type
///
/// e.g. `std::io::stdout()`, `std::fs::File`, ...
#[cfg(windows)]
pub fn from<T: AsHandle>(t: T) -> io::Result<Self> {
Ok(Self {
fx: t.as_handle().try_clone_to_owned()?,
})
}

/// conversion from borrowed native type
///
/// e.g. `std::io::stdout()`, `std::fs::File`, ...
#[cfg(not(windows))]
pub fn from<T: AsFd>(t: T) -> io::Result<Self> {
Ok(Self {
fx: t.as_fd().try_clone_to_owned()?,
})
}

/// instantiates a corresponding `File`
pub fn into_file(self) -> File {
File::from(self.fx)
}

/// instantiates a corresponding `Stdio`
pub fn into_stdio(self) -> Stdio {
Stdio::from(self.fx)
}

/// clones self. useful when needing another
/// owned reference to same file
pub fn try_clone(&self) -> io::Result<Self> {
self.fx.try_clone().map(Self::new)
}

/// provides native type to be used with
/// OS specific functions without abstraction
pub fn as_raw(&self) -> &NativeType {
&self.fx
}
}

/// instantiates a corresponding `Stdio`
impl From<OwnedFileDescriptorOrHandle> for Stdio {
fn from(value: OwnedFileDescriptorOrHandle) -> Self {
value.into_stdio()
}
}
50 changes: 49 additions & 1 deletion tests/by-util/test_dd.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
//
// For the full copyright and license information, please view the LICENSE
// file that was distributed with this source code.
// spell-checker:ignore fname, tname, fpath, specfile, testfile, unspec, ifile, ofile, outfile, fullblock, urand, fileio, atoe, atoibm, availible, behaviour, bmax, bremain, btotal, cflags, creat, ctable, ctty, datastructures, doesnt, etoa, fileout, fname, gnudd, iconvflags, iseek, nocache, noctty, noerror, nofollow, nolinks, nonblock, oconvflags, oseek, outfile, parseargs, rlen, rmax, rposition, rremain, rsofar, rstat, sigusr, sigval, wlen, wstat abcdefghijklm abcdefghi nabcde nabcdefg abcdefg fifoname
// spell-checker:ignore fname, tname, fpath, specfile, testfile, unspec, ifile, ofile, outfile, fullblock, urand, fileio, atoe, atoibm, availible, behaviour, bmax, bremain, btotal, cflags, creat, ctable, ctty, datastructures, doesnt, etoa, fileout, fname, gnudd, iconvflags, iseek, nocache, noctty, noerror, nofollow, nolinks, nonblock, oconvflags, oseek, outfile, parseargs, rlen, rmax, rposition, rremain, rsofar, rstat, sigusr, sigval, wlen, wstat abcdefghijklm abcdefghi nabcde nabcdefg abcdefg fifoname seekable

#[cfg(unix)]
use crate::common::util::run_ucmd_as_root_with_stdin_stdout;
Expand All @@ -11,6 +11,7 @@ use crate::common::util::TestScenario;
use crate::common::util::{UCommand, TESTS_BINARY};

use regex::Regex;
use uucore::io::OwnedFileDescriptorOrHandle;

use std::fs::{File, OpenOptions};
use std::io::{BufReader, Read, Write};
Expand Down Expand Up @@ -1713,3 +1714,50 @@ fn test_reading_partial_blocks_from_fifo_unbuffered() {
let expected = b"0+2 records in\n0+2 records out\n4 bytes copied";
assert!(output.stderr.starts_with(expected));
}

#[test]
fn test_stdin_stdout_not_rewound_even_when_connected_to_seekable_file() {
use std::process::Stdio;

let ts = TestScenario::new(util_name!());
let at = &ts.fixtures;

at.write("in", "abcde");

let stdin = OwnedFileDescriptorOrHandle::open_file(
OpenOptions::new().read(true),
at.plus("in").as_path(),
)
.unwrap();
let stdout = OwnedFileDescriptorOrHandle::open_file(
OpenOptions::new().create(true).write(true),
at.plus("out").as_path(),
)
.unwrap();
let stderr = OwnedFileDescriptorOrHandle::open_file(
OpenOptions::new().create(true).write(true),
at.plus("err").as_path(),
)
.unwrap();

ts.ucmd()
.args(&["bs=1", "skip=1", "count=1"])
.set_stdin(Stdio::from(stdin.try_clone().unwrap()))
.set_stdout(Stdio::from(stdout.try_clone().unwrap()))
.set_stderr(Stdio::from(stderr.try_clone().unwrap()))
.succeeds();

ts.ucmd()
.args(&["bs=1", "skip=1"])
.set_stdin(stdin)
.set_stdout(stdout)
.set_stderr(stderr)
.succeeds();

let err_file_content = std::fs::read_to_string(at.plus_as_string("err")).unwrap();
println!("stderr:\n{}", err_file_content);

let out_file_content = std::fs::read_to_string(at.plus_as_string("out")).unwrap();
println!("stdout:\n{}", out_file_content);
assert_eq!(out_file_content, "bde");
}
Loading