From fd193c3caeb14e618df85af3dc0b29ec57d0425b Mon Sep 17 00:00:00 2001 From: topjohnwu Date: Thu, 21 Aug 2025 22:26:36 -0700 Subject: [PATCH] Simplify ResultExt implementation Also introduce OptionExt --- native/src/base/result.rs | 208 +++++++++++++++------------ native/src/boot/cpio.rs | 26 ++-- native/src/core/module.rs | 4 +- native/src/core/resetprop/persist.rs | 2 +- native/src/init/logging.rs | 2 +- 5 files changed, 131 insertions(+), 111 deletions(-) diff --git a/native/src/base/result.rs b/native/src/base/result.rs index 24867352b..287368c12 100644 --- a/native/src/base/result.rs +++ b/native/src/base/result.rs @@ -33,25 +33,19 @@ macro_rules! log_err { } // Any result or option can be silenced -pub trait SilentResultExt { +pub trait SilentLogExt { fn silent(self) -> LoggedResult; } -impl SilentResultExt for Result { +impl SilentLogExt for Result { fn silent(self) -> LoggedResult { - match self { - Ok(v) => Ok(v), - Err(_) => Err(LoggedError::default()), - } + self.map_err(|_| LoggedError::default()) } } -impl SilentResultExt for Option { +impl SilentLogExt for Option { fn silent(self) -> LoggedResult { - match self { - Some(v) => Ok(v), - None => Err(LoggedError::default()), - } + self.ok_or_else(LoggedError::default) } } @@ -67,142 +61,170 @@ pub(crate) trait CxxResultExt { fn log_cxx(self) -> LoggedResult; } -trait Loggable { - fn do_log(self, level: LogLevel, caller: Option<&'static Location>) -> LoggedResult; +// Public API for converting Option to LoggedResult +pub trait OptionExt { + fn ok_or_log(self) -> LoggedResult; + fn ok_or_log_msg fmt::Result>(self, f: F) -> LoggedResult; +} + +impl OptionExt for Option { + #[inline(always)] + fn ok_or_log(self) -> LoggedResult { + self.ok_or_else(LoggedError::default) + } + + #[cfg(not(debug_assertions))] + fn ok_or_log_msg fmt::Result>(self, f: F) -> LoggedResult { + self.ok_or_else(|| { + do_log_msg(LogLevel::Error, None, f); + LoggedError::default() + }) + } + + #[track_caller] + #[cfg(debug_assertions)] + fn ok_or_log_msg fmt::Result>(self, f: F) -> LoggedResult { + let caller = Some(Location::caller()); + self.ok_or_else(|| { + do_log_msg(LogLevel::Error, caller, f); + LoggedError::default() + }) + } +} + +trait Loggable { + fn do_log(self, level: LogLevel, caller: Option<&'static Location>) -> LoggedError; fn do_log_msg fmt::Result>( self, level: LogLevel, caller: Option<&'static Location>, f: F, - ) -> LoggedResult; + ) -> LoggedError; } -impl> CxxResultExt for R { +impl CxxResultExt for Result { fn log_cxx(self) -> LoggedResult { - self.do_log(LogLevel::ErrorCxx, None) + self.map_err(|e| e.do_log(LogLevel::ErrorCxx, None)) } } -impl> ResultExt for R { +impl ResultExt for Result { #[cfg(not(debug_assertions))] fn log(self) -> LoggedResult { - self.do_log(LogLevel::Error, None) + self.map_err(|e| e.do_log(LogLevel::Error, None)) } #[track_caller] #[cfg(debug_assertions)] fn log(self) -> LoggedResult { - self.do_log(LogLevel::Error, Some(Location::caller())) + let caller = Some(Location::caller()); + self.map_err(|e| e.do_log(LogLevel::Error, caller)) } #[cfg(not(debug_assertions))] fn log_with_msg fmt::Result>(self, f: F) -> LoggedResult { - self.do_log_msg(LogLevel::Error, None, f) + self.map_err(|e| e.do_log_msg(LogLevel::Error, None, f)) } #[track_caller] #[cfg(debug_assertions)] fn log_with_msg fmt::Result>(self, f: F) -> LoggedResult { - self.do_log_msg(LogLevel::Error, Some(Location::caller()), f) + let caller = Some(Location::caller()); + self.map_err(|e| e.do_log_msg(LogLevel::Error, caller, f)) } #[cfg(not(debug_assertions))] fn log_ok(self) { - self.log().ok(); + self.map_err(|e| e.do_log(LogLevel::Error, None)).ok(); } #[track_caller] #[cfg(debug_assertions)] fn log_ok(self) { - self.do_log(LogLevel::Error, Some(Location::caller())).ok(); + let caller = Some(Location::caller()); + self.map_err(|e| e.do_log(LogLevel::Error, caller)).ok(); } } -impl Loggable for LoggedResult { - fn do_log(self, _: LogLevel, _: Option<&'static Location>) -> LoggedResult { +impl ResultExt for LoggedResult { + fn log(self) -> LoggedResult { self } - fn do_log_msg fmt::Result>( - self, - level: LogLevel, - caller: Option<&'static Location>, - f: F, - ) -> LoggedResult { - match self { - Ok(v) => Ok(v), - Err(_) => { - log_with_formatter(level, |w| { - if let Some(caller) = caller { - write!(w, "[{}:{}] ", caller.file(), caller.line())?; - } - f(w)?; - w.write_char('\n') - }); - Err(LoggedError::default()) - } - } + #[cfg(not(debug_assertions))] + fn log_with_msg fmt::Result>(self, f: F) -> LoggedResult { + do_log_msg(LogLevel::Error, None, f); + self } + + #[track_caller] + #[cfg(debug_assertions)] + fn log_with_msg fmt::Result>(self, f: F) -> LoggedResult { + let caller = Some(Location::caller()); + do_log_msg(LogLevel::Error, caller, f); + self + } + + fn log_ok(self) {} } -impl Loggable for Result { - fn do_log(self, level: LogLevel, caller: Option<&'static Location>) -> LoggedResult { - match self { - Ok(v) => Ok(v), - Err(e) => { - if let Some(caller) = caller { - log_with_args!(level, "[{}:{}] {:#}", caller.file(), caller.line(), e); - } else { - log_with_args!(level, "{:#}", e); - } - Err(LoggedError::default()) - } - } - } - - fn do_log_msg fmt::Result>( - self, - level: LogLevel, - caller: Option<&'static Location>, - f: F, - ) -> LoggedResult { - match self { - Ok(v) => Ok(v), - Err(e) => { - log_with_formatter(level, |w| { - if let Some(caller) = caller { - write!(w, "[{}:{}] ", caller.file(), caller.line())?; - } - f(w)?; - writeln!(w, ": {e:#}") - }); - Err(LoggedError::default()) - } - } - } -} - -// Automatically convert all printable errors to LoggedError to support `?` operator -impl From for LoggedError { +// Allow converting Loggable errors to LoggedError to support `?` operator +impl From for LoggedError { #[cfg(not(debug_assertions))] fn from(e: T) -> Self { - log_with_args!(LogLevel::Error, "{:#}", e); - LoggedError::default() + e.do_log(LogLevel::Error, None) } #[track_caller] #[cfg(debug_assertions)] fn from(e: T) -> Self { - let caller = Location::caller(); - log_with_args!( - LogLevel::Error, - "[{}:{}] {:#}", - caller.file(), - caller.line(), - e - ); + let caller = Some(Location::caller()); + e.do_log(LogLevel::Error, caller) + } +} + +// Actual logging implementation + +// Make all printable objects Loggable +impl Loggable for T { + fn do_log(self, level: LogLevel, caller: Option<&'static Location>) -> LoggedError { + if let Some(caller) = caller { + log_with_args!(level, "[{}:{}] {:#}", caller.file(), caller.line(), self); + } else { + log_with_args!(level, "{:#}", self); + } LoggedError::default() } + + fn do_log_msg fmt::Result>( + self, + level: LogLevel, + caller: Option<&'static Location>, + f: F, + ) -> LoggedError { + log_with_formatter(level, |w| { + if let Some(caller) = caller { + write!(w, "[{}:{}] ", caller.file(), caller.line())?; + } + f(w)?; + writeln!(w, ": {self:#}") + }); + LoggedError::default() + } +} + +fn do_log_msg fmt::Result>( + level: LogLevel, + caller: Option<&'static Location>, + f: F, +) { + log_with_formatter(level, |w| { + if let Some(caller) = caller { + write!(w, "[{}:{}] ", caller.file(), caller.line())?; + } + f(w)?; + w.write_char('\n') + }); } // Check libc return value and map to Result diff --git a/native/src/boot/cpio.rs b/native/src/boot/cpio.rs index 4079c2ac2..51d2036af 100644 --- a/native/src/boot/cpio.rs +++ b/native/src/boot/cpio.rs @@ -20,8 +20,8 @@ use base::libc::{ dev_t, gid_t, major, makedev, minor, mknod, mode_t, uid_t, }; use base::{ - BytesExt, EarlyExitExt, LoggedError, LoggedResult, MappedFile, ResultExt, Utf8CStr, - Utf8CStrBuf, WriteExt, cstr, error, log_err, + BytesExt, EarlyExitExt, LoggedResult, MappedFile, OptionExt, ResultExt, Utf8CStr, Utf8CStrBuf, + WriteExt, cstr, log_err, }; use crate::check_env; @@ -328,10 +328,10 @@ impl Cpio { } fn extract_entry(&self, path: &str, out: &mut String) -> LoggedResult<()> { - let entry = self.entries.get(path).ok_or_else(|| { - error!("No such file"); - LoggedError::default() - })?; + let entry = self + .entries + .get(path) + .ok_or_log_msg(|w| w.write_str("No such file"))?; eprintln!("Extracting entry [{path}] to [{out}]"); let out = Utf8CStr::from_string(out); @@ -462,10 +462,10 @@ impl Cpio { } fn mv(&mut self, from: &str, to: &str) -> LoggedResult<()> { - let entry = self.entries.remove(&norm_path(from)).ok_or_else(|| { - error!("no such entry {}", from); - LoggedError::default() - })?; + let entry = self + .entries + .remove(&norm_path(from)) + .ok_or_log_msg(|w| w.write_fmt(format_args!("No such entry {from}")))?; self.entries.insert(norm_path(to), entry); eprintln!("Move [{from}] -> [{to}]"); Ok(()) @@ -813,10 +813,8 @@ fn x8u(x: &[u8; 8]) -> LoggedResult { let s = str::from_utf8(x).log_with_msg(|w| w.write_str("bad cpio header"))?; for c in s.chars() { ret = ret * 16 - + c.to_digit(16).ok_or_else(|| { - error!("bad cpio header"); - LoggedError::default() - })?; + + c.to_digit(16) + .ok_or_log_msg(|w| w.write_str("bad cpio header"))?; } Ok(ret) } diff --git a/native/src/core/module.rs b/native/src/core/module.rs index 40a131f7b..42aa00b52 100644 --- a/native/src/core/module.rs +++ b/native/src/core/module.rs @@ -4,8 +4,8 @@ use crate::ffi::{ModuleInfo, exec_module_scripts, exec_script, get_magisk_tmp, l use crate::mount::setup_module_mount; use base::{ DirEntry, Directory, FsPathBuilder, LibcReturn, LoggedResult, OsResultStatic, ResultExt, - SilentResultExt, Utf8CStr, Utf8CStrBuf, Utf8CString, WalkResult, clone_attr, cstr, debug, - error, info, libc, raw_cstr, warn, + SilentLogExt, Utf8CStr, Utf8CStrBuf, Utf8CString, WalkResult, clone_attr, cstr, debug, error, + info, libc, raw_cstr, warn, }; use libc::{AT_REMOVEDIR, MS_RDONLY, O_CLOEXEC, O_CREAT, O_RDONLY}; use std::collections::BTreeMap; diff --git a/native/src/core/resetprop/persist.rs b/native/src/core/resetprop/persist.rs index 89f106c27..a37d76d81 100644 --- a/native/src/core/resetprop/persist.rs +++ b/native/src/core/resetprop/persist.rs @@ -16,7 +16,7 @@ use crate::resetprop::proto::persistent_properties::{ use base::const_format::concatcp; use base::libc::{O_CLOEXEC, O_RDONLY}; use base::{ - Directory, FsPathBuilder, LibcReturn, LoggedResult, MappedFile, SilentResultExt, Utf8CStr, + Directory, FsPathBuilder, LibcReturn, LoggedResult, MappedFile, SilentLogExt, Utf8CStr, Utf8CStrBuf, WalkResult, clone_attr, cstr, debug, libc::mkstemp, }; diff --git a/native/src/init/logging.rs b/native/src/init/logging.rs index 997651aef..e53d1f4e2 100644 --- a/native/src/init/logging.rs +++ b/native/src/init/logging.rs @@ -1,5 +1,5 @@ use base::{ - LOGGER, LogLevel, Logger, SilentResultExt, Utf8CStr, cstr, + LOGGER, LogLevel, Logger, SilentLogExt, Utf8CStr, cstr, libc::{ O_CLOEXEC, O_RDWR, O_WRONLY, S_IFCHR, STDERR_FILENO, STDIN_FILENO, STDOUT_FILENO, SYS_dup3, makedev, mknod, syscall,