From 73c3d741a7889b21afe3f3d3af941eafca882b2c Mon Sep 17 00:00:00 2001 From: topjohnwu Date: Tue, 5 Mar 2024 01:48:35 -0800 Subject: [PATCH] Reorganize some code --- native/src/base/cxx_extern.rs | 5 +- native/src/base/lib.rs | 2 + native/src/base/logging.rs | 210 +-------------------------- native/src/base/result.rs | 195 +++++++++++++++++++++++++ native/src/core/resetprop/persist.rs | 10 +- 5 files changed, 210 insertions(+), 212 deletions(-) create mode 100644 native/src/base/result.rs diff --git a/native/src/base/cxx_extern.rs b/native/src/base/cxx_extern.rs index 7c64a455f..6ddae9124 100644 --- a/native/src/base/cxx_extern.rs +++ b/native/src/base/cxx_extern.rs @@ -7,11 +7,10 @@ use cfg_if::cfg_if; use cxx::private::c_char; use libc::mode_t; -use crate::logging::CxxResultExt; pub(crate) use crate::xwrap::*; use crate::{ - clone_attr, cstr, fclone_attr, fd_path, map_fd, map_file, slice_from_ptr, Directory, FsPath, - Utf8CStr, Utf8CStrBufRef, + clone_attr, cstr, fclone_attr, fd_path, map_fd, map_file, slice_from_ptr, CxxResultExt, + Directory, FsPath, Utf8CStr, Utf8CStrBufRef, }; pub(crate) fn fd_path_for_cxx(fd: RawFd, buf: &mut [u8]) -> isize { diff --git a/native/src/base/lib.rs b/native/src/base/lib.rs index 04ea52638..39c9d8d11 100644 --- a/native/src/base/lib.rs +++ b/native/src/base/lib.rs @@ -11,12 +11,14 @@ use cxx_extern::*; pub use files::*; pub use logging::*; pub use misc::*; +pub use result::*; mod cstr; mod cxx_extern; mod files; mod logging; mod misc; +mod result; mod xwrap; #[cxx::bridge] diff --git a/native/src/base/logging.rs b/native/src/base/logging.rs index e7b36606d..868795b46 100644 --- a/native/src/base/logging.rs +++ b/native/src/base/logging.rs @@ -1,27 +1,14 @@ +use std::fmt; +use std::fmt::Arguments; +use std::io::{stderr, stdout, Write}; +use std::process::exit; + use num_derive::{FromPrimitive, ToPrimitive}; use num_traits::FromPrimitive; -use std::fmt; -use std::fmt::{Arguments, Display}; -use std::io::{stderr, stdout, Write}; -use std::panic::Location; -use std::process::exit; use crate::ffi::LogLevelCxx; use crate::{Utf8CStr, Utf8CStrBufArr}; -// Error handling and logging throughout the Rust codebase in Magisk: -// -// All errors should be logged and consumed as soon as possible and converted into LoggedError. -// For `Result` with errors that implement the `Display` trait, use the `?` operator to -// log and convert to LoggedResult. -// -// To log an error with more information, use `ResultExt::log_with_msg()`. -// -// The "cxx" method variants in `CxxResultExt` are only used for C++ interop and -// should not be used directly in any Rust code. -// -// For general logging, use the !(...) macros. - // Ugly hack to avoid using enum #[allow(non_snake_case, non_upper_case_globals)] mod LogFlag { @@ -50,7 +37,7 @@ pub static mut LOGGER: Logger = Logger { }; type LogWriter = fn(level: LogLevel, msg: &Utf8CStr); -type Formatter<'a> = &'a mut dyn fmt::Write; +pub(crate) type Formatter<'a> = &'a mut dyn fmt::Write; #[derive(Copy, Clone)] pub struct Logger { @@ -171,188 +158,3 @@ macro_rules! debug { macro_rules! debug { ($($args:tt)+) => {}; } - -#[derive(Default)] -pub struct LoggedError {} - -// Automatically handle all printable errors -impl From for LoggedError { - #[cfg(not(debug_assertions))] - fn from(e: T) -> Self { - log_with_args(LogLevel::Error, format_args_nl!("{:#}", e)); - LoggedError::default() - } - - #[track_caller] - #[cfg(debug_assertions)] - fn from(e: T) -> Self { - let caller = Location::caller(); - log_with_args( - LogLevel::Error, - format_args_nl!("[{}:{}] {:#}", caller.file(), caller.line(), e), - ); - LoggedError::default() - } -} - -pub type LoggedResult = Result; - -#[macro_export] -macro_rules! log_err { - ($msg:literal $(,)?) => {{ - $crate::log_with_args($crate::LogLevel::Error, format_args_nl!($msg)); - $crate::LoggedError::default() - }}; - ($err:expr $(,)?) => {{ - $crate::log_with_args($crate::LogLevel::Error, format_args_nl!("{}", $err)); - $crate::LoggedError::default() - }}; - ($($args:tt)+) => {{ - $crate::log_with_args($crate::LogLevel::Error, format_args_nl!($($args)+)); - $crate::LoggedError::default() - }}; -} - -pub trait ResultExt { - fn log(self) -> LoggedResult; - fn log_with_msg fmt::Result>(self, f: F) -> LoggedResult; -} - -pub trait ResultNoLog { - fn no_log(self) -> LoggedResult; -} - -// Internal C++ bridging logging routines -pub(crate) trait CxxResultExt { - fn log_cxx(self) -> LoggedResult; - fn log_cxx_with_msg fmt::Result>(self, f: F) -> LoggedResult; -} - -trait LogImpl { - fn log_impl(self, level: LogLevel, caller: Option<&'static Location>) -> LoggedResult; - fn log_with_msg_impl fmt::Result>( - self, - level: LogLevel, - caller: Option<&'static Location>, - f: F, - ) -> LoggedResult; -} - -impl ResultNoLog for Result { - fn no_log(self) -> LoggedResult { - match self { - Ok(v) => Ok(v), - Err(_) => Err(LoggedError::default()), - } - } -} - -impl ResultNoLog for Option { - fn no_log(self) -> LoggedResult { - match self { - Some(v) => Ok(v), - None => Err(LoggedError::default()), - } - } -} - -impl> CxxResultExt for R { - fn log_cxx(self) -> LoggedResult { - self.log_impl(LogLevel::ErrorCxx, None) - } - - fn log_cxx_with_msg fmt::Result>(self, f: F) -> LoggedResult { - self.log_with_msg_impl(LogLevel::ErrorCxx, None, f) - } -} - -impl> ResultExt for R { - #[cfg(not(debug_assertions))] - fn log(self) -> LoggedResult { - self.log_impl(LogLevel::Error, None) - } - - #[track_caller] - #[cfg(debug_assertions)] - fn log(self) -> LoggedResult { - self.log_impl(LogLevel::Error, Some(Location::caller())) - } - - #[cfg(not(debug_assertions))] - fn log_with_msg fmt::Result>(self, f: F) -> LoggedResult { - self.log_with_msg_impl(LogLevel::Error, None, f) - } - - #[track_caller] - #[cfg(debug_assertions)] - fn log_with_msg fmt::Result>(self, f: F) -> LoggedResult { - self.log_with_msg_impl(LogLevel::Error, Some(Location::caller()), f) - } -} - -impl LogImpl for LoggedResult { - fn log_impl(self, _: LogLevel, _: Option<&'static Location>) -> LoggedResult { - self - } - - fn log_with_msg_impl 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()) - } - } - } -} - -impl LogImpl for Result { - fn log_impl(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, - format_args_nl!("[{}:{}] {:#}", caller.file(), caller.line(), e), - ); - } else { - log_with_args(level, format_args_nl!("{:#}", e)); - } - Err(LoggedError::default()) - } - } - } - - fn log_with_msg_impl 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()) - } - } - } -} diff --git a/native/src/base/result.rs b/native/src/base/result.rs new file mode 100644 index 000000000..0d32d4f63 --- /dev/null +++ b/native/src/base/result.rs @@ -0,0 +1,195 @@ +use std::fmt; +use std::fmt::Display; +use std::panic::Location; + +use crate::logging::Formatter; +use crate::{log_with_args, log_with_formatter, LogLevel}; + +// Error handling throughout the Rust codebase in Magisk: +// +// All errors should be logged and consumed as soon as possible and converted into LoggedError. +// For `Result` with errors that implement the `Display` trait, use the `?` operator to +// log and convert to LoggedResult. +// +// To log an error with more information, use `ResultExt::log_with_msg()`. +// +// The "cxx" method variants in `CxxResultExt` are only used for C++ interop and +// should not be used directly in any Rust code. + +#[derive(Default)] +pub struct LoggedError {} +pub type LoggedResult = Result; + +#[macro_export] +macro_rules! log_err { + ($($args:tt)+) => {{ + $crate::log_with_args($crate::LogLevel::Error, format_args_nl!($($args)+)); + $crate::LoggedError::default() + }}; +} + +// Any result or option can be silenced +pub trait SilentResultExt { + fn silent(self) -> LoggedResult; +} + +impl SilentResultExt for Result { + fn silent(self) -> LoggedResult { + match self { + Ok(v) => Ok(v), + Err(_) => Err(LoggedError::default()), + } + } +} + +impl SilentResultExt for Option { + fn silent(self) -> LoggedResult { + match self { + Some(v) => Ok(v), + None => Err(LoggedError::default()), + } + } +} + +// Public API for logging results +pub trait ResultExt { + fn log(self) -> LoggedResult; + fn log_with_msg fmt::Result>(self, f: F) -> LoggedResult; +} + +// Internal C++ bridging logging routines +pub(crate) trait CxxResultExt { + fn log_cxx(self) -> LoggedResult; + fn log_cxx_with_msg fmt::Result>(self, f: F) -> LoggedResult; +} + +trait Loggable { + fn do_log(self, level: LogLevel, caller: Option<&'static Location>) -> LoggedResult; + fn do_log_msg fmt::Result>( + self, + level: LogLevel, + caller: Option<&'static Location>, + f: F, + ) -> LoggedResult; +} + +impl> CxxResultExt for R { + fn log_cxx(self) -> LoggedResult { + self.do_log(LogLevel::ErrorCxx, None) + } + + fn log_cxx_with_msg fmt::Result>(self, f: F) -> LoggedResult { + self.do_log_msg(LogLevel::ErrorCxx, None, f) + } +} + +impl> ResultExt for R { + #[cfg(not(debug_assertions))] + fn log(self) -> LoggedResult { + self.do_log(LogLevel::Error, None) + } + + #[track_caller] + #[cfg(debug_assertions)] + fn log(self) -> LoggedResult { + self.do_log(LogLevel::Error, Some(Location::caller())) + } + + #[cfg(not(debug_assertions))] + fn log_with_msg fmt::Result>(self, f: F) -> LoggedResult { + self.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) + } +} + +impl Loggable for LoggedResult { + fn do_log(self, _: LogLevel, _: Option<&'static Location>) -> 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()) + } + } + } +} + +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, + format_args_nl!("[{}:{}] {:#}", caller.file(), caller.line(), e), + ); + } else { + log_with_args(level, format_args_nl!("{:#}", 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 { + #[cfg(not(debug_assertions))] + fn from(e: T) -> Self { + log_with_args(LogLevel::Error, format_args_nl!("{:#}", e)); + LoggedError::default() + } + + #[track_caller] + #[cfg(debug_assertions)] + fn from(e: T) -> Self { + let caller = Location::caller(); + log_with_args( + LogLevel::Error, + format_args_nl!("[{}:{}] {:#}", caller.file(), caller.line(), e), + ); + LoggedError::default() + } +} diff --git a/native/src/core/resetprop/persist.rs b/native/src/core/resetprop/persist.rs index fdcea05d8..51d4e61d2 100644 --- a/native/src/core/resetprop/persist.rs +++ b/native/src/core/resetprop/persist.rs @@ -12,7 +12,7 @@ use quick_protobuf::{BytesReader, MessageRead, MessageWrite, Writer}; use base::libc::{O_CLOEXEC, O_RDONLY}; use base::{ clone_attr, cstr, debug, libc::mkstemp, Directory, FsPath, FsPathBuf, LibcReturn, LoggedResult, - MappedFile, ResultNoLog, Utf8CStr, Utf8CStrBufArr, WalkResult, + MappedFile, SilentResultExt, Utf8CStr, Utf8CStrBufArr, WalkResult, }; use crate::ffi::{prop_cb_exec, PropCb}; @@ -67,7 +67,7 @@ impl PropExt for PersistentProperties { } fn find(&mut self, name: &Utf8CStr) -> LoggedResult<&mut PersistentPropertyRecord> { - let idx = self.find_index(name).no_log()?; + let idx = self.find_index(name).silent()?; Ok(&mut self[idx]) } } @@ -81,7 +81,7 @@ fn file_get_prop(name: &Utf8CStr) -> LoggedResult { let path = FsPathBuf::new(&mut buf) .join(PERSIST_PROP_DIR!()) .join(name); - let mut file = path.open(O_RDONLY | O_CLOEXEC).no_log()?; + let mut file = path.open(O_RDONLY | O_CLOEXEC).silent()?; debug!("resetprop: read prop from [{}]", path); let mut s = String::new(); file.read_to_string(&mut s)?; @@ -108,7 +108,7 @@ fn file_set_prop(name: &Utf8CStr, value: Option<&Utf8CStr>) -> LoggedResult<()> debug!("resetprop: write prop to [{}]", tmp); tmp.rename_to(path)? } else { - path.remove().no_log()?; + path.remove().silent()?; debug!("resetprop: unlink [{}]", path); } Ok(()) @@ -199,7 +199,7 @@ pub unsafe fn persist_delete_prop(name: &Utf8CStr) -> bool { fn inner(name: &Utf8CStr) -> LoggedResult<()> { if check_proto() { let mut props = proto_read_props()?; - let idx = props.find_index(name).no_log()?; + let idx = props.find_index(name).silent()?; props.remove(idx); proto_write_props(&props) } else {