From 50be73d99f498c27652648337e5c83978d8ce259 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Orhun=20Parmaks=C4=B1z?= Date: Wed, 19 Oct 2022 00:13:59 +0300 Subject: [PATCH 1/6] refactor: add automatic conversion for `nix::Errno` --- src/uu/sync/src/sync.rs | 28 +++------------------------- src/uucore/Cargo.toml | 8 ++++---- src/uucore/src/lib/mods/error.rs | 11 +++++++++++ 3 files changed, 18 insertions(+), 29 deletions(-) diff --git a/src/uu/sync/src/sync.rs b/src/uu/sync/src/sync.rs index 5db957178..eeff191a4 100644 --- a/src/uu/sync/src/sync.rs +++ b/src/uu/sync/src/sync.rs @@ -11,14 +11,12 @@ extern crate libc; use clap::{crate_version, Arg, ArgAction, Command}; #[cfg(any(target_os = "linux", target_os = "android"))] -use nix::errno::Errno; -#[cfg(any(target_os = "linux", target_os = "android"))] use nix::fcntl::{open, OFlag}; #[cfg(any(target_os = "linux", target_os = "android"))] use nix::sys::stat::Mode; use std::path::Path; use uucore::display::Quotable; -use uucore::error::{UResult, USimpleError}; +use uucore::error::{FromIo, UResult, USimpleError}; use uucore::format_usage; static ABOUT: &str = "Synchronize cached writes to persistent storage"; @@ -183,28 +181,8 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> { // Use the Nix open to be able to set the NONBLOCK flags for fifo files #[cfg(any(target_os = "linux", target_os = "android"))] { - match open(Path::new(&f), OFlag::O_NONBLOCK, Mode::empty()) { - Ok(_) => {} - Err(e) => { - if e == Errno::ENOENT { - return Err(USimpleError::new( - 1, - format!("cannot stat {}: No such file or directory", f.quote()), - )); - } - if e == Errno::EACCES { - if Path::new(&f).is_dir() { - return Err(USimpleError::new( - 1, - format!("error opening {}: Permission denied", f.quote()), - )); - } else { - // ignore the issue - // ./target/debug/coreutils sync --data file - } - } - } - } + open(Path::new(&f), OFlag::O_NONBLOCK, Mode::empty()) + .map_err_context(|| format!("cannot stat {}", f.quote()))?; } #[cfg(not(any(target_os = "linux", target_os = "android")))] { diff --git a/src/uucore/Cargo.toml b/src/uucore/Cargo.toml index af78e3f94..240678b69 100644 --- a/src/uucore/Cargo.toml +++ b/src/uucore/Cargo.toml @@ -38,7 +38,7 @@ os_display = "0.1.3" [target.'cfg(unix)'.dependencies] walkdir = { version="2.3.2", optional=true } -nix = { version = "0.25", optional = true, default-features = false, features = ["fs", "uio", "zerocopy"] } +nix = { version = "0.25", default-features = false, features = ["fs", "uio", "zerocopy"] } [dev-dependencies] clap = "4.0" @@ -53,8 +53,8 @@ default = [] # * non-default features encoding = ["data-encoding", "data-encoding-macro", "z85", "thiserror"] entries = ["libc"] -fs = ["libc", "nix", "winapi-util"] -fsext = ["libc", "nix", "time"] +fs = ["libc", "winapi-util"] +fsext = ["libc", "time"] lines = [] memo = ["itertools"] mode = ["libc"] @@ -65,4 +65,4 @@ signals = [] utf8 = [] utmpx = ["time", "time/macros", "libc", "dns-lookup"] wide = [] -pipes = ["nix"] +pipes = [] diff --git a/src/uucore/src/lib/mods/error.rs b/src/uucore/src/lib/mods/error.rs index 4cc0b1519..7377c021b 100644 --- a/src/uucore/src/lib/mods/error.rs +++ b/src/uucore/src/lib/mods/error.rs @@ -508,6 +508,17 @@ impl From for Box { } } +impl FromIo> for Result { + fn map_err_context(self, context: impl FnOnce() -> String) -> UResult { + self.map_err(|e| { + Box::new(UIoError { + context: Some((context)()), + inner: std::io::Error::from_raw_os_error(e as i32), + }) as Box + }) + } +} + /// Shorthand to construct [`UIoError`]-instances. /// /// This macro serves as a convenience call to quickly construct instances of From df8ba875167e2a95b801e7929c167f8016ffa9e3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Orhun=20Parmaks=C4=B1z?= Date: Sat, 22 Oct 2022 17:20:57 +0300 Subject: [PATCH 2/6] feat: add more implementations for converting `nix::Error` --- src/uucore/src/lib/mods/error.rs | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/src/uucore/src/lib/mods/error.rs b/src/uucore/src/lib/mods/error.rs index 7377c021b..67fed9a46 100644 --- a/src/uucore/src/lib/mods/error.rs +++ b/src/uucore/src/lib/mods/error.rs @@ -519,6 +519,31 @@ impl FromIo> for Result { } } +impl FromIo> for nix::Error { + fn map_err_context(self, context: impl FnOnce() -> String) -> UResult { + Err(Box::new(UIoError { + context: Some((context)()), + inner: std::io::Error::from_raw_os_error(self as i32), + }) as Box) + } +} + +impl From for UIoError { + fn from(f: nix::Error) -> Self { + Self { + context: None, + inner: std::io::Error::from_raw_os_error(f as i32), + } + } +} + +impl From for Box { + fn from(f: nix::Error) -> Self { + let u_error: UIoError = f.into(); + Box::new(u_error) as Self + } +} + /// Shorthand to construct [`UIoError`]-instances. /// /// This macro serves as a convenience call to quickly construct instances of From 990bb4224d3d4f58e14c999b3d7ffae06e8ef304 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Orhun=20Parmaks=C4=B1z?= Date: Sat, 22 Oct 2022 17:45:40 +0300 Subject: [PATCH 3/6] test: add test for `nix::Error` conversions --- src/uucore/src/lib/mods/error.rs | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/src/uucore/src/lib/mods/error.rs b/src/uucore/src/lib/mods/error.rs index 67fed9a46..9e1bbc09b 100644 --- a/src/uucore/src/lib/mods/error.rs +++ b/src/uucore/src/lib/mods/error.rs @@ -729,3 +729,29 @@ impl Display for ClapErrorWrapper { Ok(()) } } + +#[cfg(test)] +mod tests { + use super::*; + use nix::errno::Errno; + use std::io::ErrorKind; + + #[test] + fn test_nix_error_conversion() { + for (nix_error, expected_error_kind) in [ + (Errno::EACCES, ErrorKind::PermissionDenied), + (Errno::ENOENT, ErrorKind::NotFound), + (Errno::EEXIST, ErrorKind::AlreadyExists), + ] { + let error = UIoError::from(nix_error); + assert_eq!(expected_error_kind, error.inner.kind()); + } + assert_eq!( + "test: Permission denied", + Err::<(), nix::Error>(Errno::EACCES) + .map_err_context(|| String::from("test")) + .unwrap_err() + .to_string() + ) + } +} From c19c19e4db56678035d4aca4053128dfbcae7ed4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Orhun=20Parmaks=C4=B1z?= Date: Sat, 22 Oct 2022 21:26:33 +0300 Subject: [PATCH 4/6] docs: add usage example for `nix::Errno` conversion --- src/uucore/src/lib/mods/error.rs | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/src/uucore/src/lib/mods/error.rs b/src/uucore/src/lib/mods/error.rs index 9e1bbc09b..de53f2ef3 100644 --- a/src/uucore/src/lib/mods/error.rs +++ b/src/uucore/src/lib/mods/error.rs @@ -508,6 +508,20 @@ impl From for Box { } } +/// Enables the conversion from [`Result`] to [`UResult`]. +/// +/// # Examples +/// +/// ``` +/// use uucore::error::FromIo; +/// use nix::errno::Errno; +/// +/// let nix_err = Err::<(), nix::Error>(Errno::EACCES); +/// let uio_result = nix_err.map_err_context(|| String::from("fix me please!")); +/// +/// // prints "fix me please!: Permission denied" +/// println!("{}", uio_result.unwrap_err()); +/// ``` impl FromIo> for Result { fn map_err_context(self, context: impl FnOnce() -> String) -> UResult { self.map_err(|e| { From 81ea9521ce0fdb8bc2eb28668c44f944e79da9d4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Orhun=20Parmaks=C4=B1z?= Date: Sat, 22 Oct 2022 21:34:45 +0300 Subject: [PATCH 5/6] fix: conditionally enable `nix` error conversions --- src/uu/sync/src/sync.rs | 4 +++- src/uucore/src/lib/mods/error.rs | 13 +++++++++---- 2 files changed, 12 insertions(+), 5 deletions(-) diff --git a/src/uu/sync/src/sync.rs b/src/uu/sync/src/sync.rs index a0139dcdf..08ba6b1d4 100644 --- a/src/uu/sync/src/sync.rs +++ b/src/uu/sync/src/sync.rs @@ -16,7 +16,9 @@ use nix::fcntl::{open, OFlag}; use nix::sys::stat::Mode; use std::path::Path; use uucore::display::Quotable; -use uucore::error::{FromIo, UResult, USimpleError}; +#[cfg(any(target_os = "linux", target_os = "android"))] +use uucore::error::FromIo; +use uucore::error::{UResult, USimpleError}; use uucore::format_usage; static ABOUT: &str = "Synchronize cached writes to persistent storage"; diff --git a/src/uucore/src/lib/mods/error.rs b/src/uucore/src/lib/mods/error.rs index de53f2ef3..5f6f21b77 100644 --- a/src/uucore/src/lib/mods/error.rs +++ b/src/uucore/src/lib/mods/error.rs @@ -522,6 +522,7 @@ impl From for Box { /// // prints "fix me please!: Permission denied" /// println!("{}", uio_result.unwrap_err()); /// ``` +#[cfg(any(target_os = "linux", target_os = "android"))] impl FromIo> for Result { fn map_err_context(self, context: impl FnOnce() -> String) -> UResult { self.map_err(|e| { @@ -533,6 +534,7 @@ impl FromIo> for Result { } } +#[cfg(any(target_os = "linux", target_os = "android"))] impl FromIo> for nix::Error { fn map_err_context(self, context: impl FnOnce() -> String) -> UResult { Err(Box::new(UIoError { @@ -542,6 +544,7 @@ impl FromIo> for nix::Error { } } +#[cfg(any(target_os = "linux", target_os = "android"))] impl From for UIoError { fn from(f: nix::Error) -> Self { Self { @@ -551,6 +554,7 @@ impl From for UIoError { } } +#[cfg(any(target_os = "linux", target_os = "android"))] impl From for Box { fn from(f: nix::Error) -> Self { let u_error: UIoError = f.into(); @@ -746,12 +750,13 @@ impl Display for ClapErrorWrapper { #[cfg(test)] mod tests { - use super::*; - use nix::errno::Errno; - use std::io::ErrorKind; - #[test] + #[cfg(any(target_os = "linux", target_os = "android"))] fn test_nix_error_conversion() { + use super::{FromIo, UIoError}; + use nix::errno::Errno; + use std::io::ErrorKind; + for (nix_error, expected_error_kind) in [ (Errno::EACCES, ErrorKind::PermissionDenied), (Errno::ENOENT, ErrorKind::NotFound), From be49eb68f30e7f215b194369c029a6bb5aabf40b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Orhun=20Parmaks=C4=B1z?= Date: Fri, 4 Nov 2022 13:35:54 +0300 Subject: [PATCH 6/6] fix: address test failures --- src/uu/sync/src/sync.rs | 11 +++++++++-- tests/by-util/test_sync.rs | 4 ++-- 2 files changed, 11 insertions(+), 4 deletions(-) diff --git a/src/uu/sync/src/sync.rs b/src/uu/sync/src/sync.rs index 08ba6b1d4..0fa81218a 100644 --- a/src/uu/sync/src/sync.rs +++ b/src/uu/sync/src/sync.rs @@ -11,6 +11,8 @@ extern crate libc; use clap::{crate_version, Arg, ArgAction, Command}; #[cfg(any(target_os = "linux", target_os = "android"))] +use nix::errno::Errno; +#[cfg(any(target_os = "linux", target_os = "android"))] use nix::fcntl::{open, OFlag}; #[cfg(any(target_os = "linux", target_os = "android"))] use nix::sys::stat::Mode; @@ -170,9 +172,14 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> { // Use the Nix open to be able to set the NONBLOCK flags for fifo files #[cfg(any(target_os = "linux", target_os = "android"))] { - open(Path::new(&f), OFlag::O_NONBLOCK, Mode::empty()) - .map_err_context(|| format!("cannot stat {}", f.quote()))?; + let path = Path::new(&f); + if let Err(e) = open(path, OFlag::O_NONBLOCK, Mode::empty()) { + if e != Errno::EACCES || (e == Errno::EACCES && path.is_dir()) { + return e.map_err_context(|| format!("cannot stat {}", f.quote()))?; + } + } } + #[cfg(not(any(target_os = "linux", target_os = "android")))] { if !Path::new(&f).exists() { diff --git a/tests/by-util/test_sync.rs b/tests/by-util/test_sync.rs index 7f2cd4b66..4bf2629c4 100644 --- a/tests/by-util/test_sync.rs +++ b/tests/by-util/test_sync.rs @@ -64,9 +64,9 @@ fn test_sync_no_permission_dir() { ts.ccmd("chmod").arg("0").arg(dir).succeeds(); let result = ts.ucmd().arg("--data").arg(dir).fails(); - result.stderr_contains("sync: error opening 'foo': Permission denied"); + result.stderr_contains("sync: cannot stat 'foo': Permission denied"); let result = ts.ucmd().arg(dir).fails(); - result.stderr_contains("sync: error opening 'foo': Permission denied"); + result.stderr_contains("sync: cannot stat 'foo': Permission denied"); } #[cfg(not(target_os = "windows"))]