Fix error logging on the C++ side

topjohnwu · Oct 13, 2023 · 36de628 · 36de628
1 parent 51e3788
commit 36de628
Show file tree

Hide file tree

Showing 7 changed files with 48 additions and 26 deletions.
diff --git a/native/src/Cargo.lock b/native/src/Cargo.lock
diff --git a/native/src/base/Cargo.toml b/native/src/base/Cargo.toml
@@ -17,3 +17,4 @@ thiserror = { workspace = true }
 argh = { workspace = true }
 bytemuck = { workspace = true }
 num-traits = { workspace = true }
+num-derive = { workspace = true }
diff --git a/native/src/base/lib.rs b/native/src/base/lib.rs
@@ -4,6 +4,7 @@
 #![feature(utf8_chunks)]
 
 pub use libc;
+use num_traits::FromPrimitive;
 
 pub use cstr::*;
 use cxx_extern::*;
@@ -21,7 +22,9 @@ mod xwrap;
 #[cxx::bridge]
 pub mod ffi {
     #[derive(Copy, Clone)]
-    pub enum LogLevel {
+    #[repr(i32)]
+    #[cxx_name = "LogLevel"]
+    pub(crate) enum LogLevelCxx {
         ErrorCxx,
         Error,
         Warn,
@@ -36,9 +39,10 @@ pub mod ffi {
 
     extern "Rust" {
         #[cxx_name = "log_with_rs"]
-        fn log_from_cxx(level: LogLevel, msg: &[u8]);
+        fn log_from_cxx(level: LogLevelCxx, msg: &[u8]);
+        #[cxx_name = "set_log_level_state"]
+        fn set_log_level_state_cxx(level: LogLevelCxx, enabled: bool);
         fn exit_on_error(b: bool);
-        fn set_log_level_state(level: LogLevel, enabled: bool);
         fn cmdline_logging();
     }
 
@@ -54,3 +58,9 @@ pub mod ffi {
         fn enable_selinux();
     }
 }
+
+fn set_log_level_state_cxx(level: ffi::LogLevelCxx, enabled: bool) {
+    if let Some(level) = LogLevel::from_i32(level.repr) {
+        set_log_level_state(level, enabled)
+    }
+}
diff --git a/native/src/base/logging.rs b/native/src/base/logging.rs
@@ -1,10 +1,12 @@
+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::LogLevel;
+use crate::ffi::LogLevelCxx;
 use crate::{Utf8CStr, Utf8CStrBufArr};
 
 // Error handling and logging throughout the Rust codebase in Magisk:
@@ -30,6 +32,16 @@ mod LogFlag {
     pub const ExitOnError: u32 = 1 << 4;
 }
 
+#[derive(Copy, Clone, FromPrimitive, ToPrimitive)]
+#[repr(i32)]
+pub enum LogLevel {
+    ErrorCxx = LogLevelCxx::ErrorCxx.repr,
+    Error = LogLevelCxx::Error.repr,
+    Warn = LogLevelCxx::Warn.repr,
+    Info = LogLevelCxx::Info.repr,
+    Debug = LogLevelCxx::Debug.repr,
+}
+
 // We don't need to care about thread safety, because all
 // logger changes will only happen on the main thread.
 pub static mut LOGGER: Logger = Logger {
@@ -63,7 +75,6 @@ impl LogLevel {
             LogLevel::Warn => LogFlag::DisableWarn,
             LogLevel::Info => LogFlag::DisableInfo,
             LogLevel::Debug => LogFlag::DisableDebug,
-            _ => 0,
         }
     }
 }
@@ -85,15 +96,17 @@ fn log_with_writer<F: FnOnce(LogWriter)>(level: LogLevel, f: F) {
         return;
     }
     f(logger.write);
-    if level == LogLevel::ErrorCxx && (logger.flags & LogFlag::ExitOnError) != 0 {
+    if matches!(level, LogLevel::ErrorCxx) && (logger.flags & LogFlag::ExitOnError) != 0 {
         exit(1);
     }
 }
 
-pub fn log_from_cxx(level: LogLevel, msg: &[u8]) {
-    // SAFETY: The null termination is handled on the C++ side
-    let msg = unsafe { Utf8CStr::from_bytes_unchecked(msg) };
-    log_with_writer(level, |write| write(level, msg));
+pub fn log_from_cxx(level: LogLevelCxx, msg: &[u8]) {
+    if let Some(level) = LogLevel::from_i32(level.repr) {
+        // SAFETY: The null termination is handled on the C++ side
+        let msg = unsafe { Utf8CStr::from_bytes_unchecked(msg) };
+        log_with_writer(level, |write| write(level, msg));
+    }
 }
 
 pub fn log_with_formatter<F: FnOnce(Formatter) -> fmt::Result>(level: LogLevel, f: F) {
@@ -110,7 +123,7 @@ pub fn log_with_args(level: LogLevel, args: Arguments) {
 
 pub fn cmdline_logging() {
     fn cmdline_write(level: LogLevel, msg: &Utf8CStr) {
-        if level == LogLevel::Info {
+        if matches!(level, LogLevel::Info) {
             stdout().write_all(msg.as_bytes()).ok();
         } else {
             stderr().write_all(msg.as_bytes()).ok();
@@ -129,29 +142,29 @@ pub fn cmdline_logging() {
 #[macro_export]
 macro_rules! error {
     ($($args:tt)+) => {
-        $crate::log_with_args($crate::ffi::LogLevel::Error, format_args_nl!($($args)+))
+        $crate::log_with_args($crate::LogLevel::Error, format_args_nl!($($args)+))
     }
 }
 
 #[macro_export]
 macro_rules! warn {
     ($($args:tt)+) => {
-        $crate::log_with_args($crate::ffi::LogLevel::Warn, format_args_nl!($($args)+))
+        $crate::log_with_args($crate::LogLevel::Warn, format_args_nl!($($args)+))
     }
 }
 
 #[macro_export]
 macro_rules! info {
     ($($args:tt)+) => {
-        $crate::log_with_args($crate::ffi::LogLevel::Info, format_args_nl!($($args)+))
+        $crate::log_with_args($crate::LogLevel::Info, format_args_nl!($($args)+))
     }
 }
 
 #[cfg(debug_assertions)]
 #[macro_export]
 macro_rules! debug {
     ($($args:tt)+) => {
-        $crate::log_with_args($crate::ffi::LogLevel::Debug, format_args_nl!($($args)+))
+        $crate::log_with_args($crate::LogLevel::Debug, format_args_nl!($($args)+))
     }
 }
 
@@ -189,15 +202,15 @@ pub type LoggedResult<T> = Result<T, LoggedError>;
 #[macro_export]
 macro_rules! log_err {
     ($msg:literal $(,)?) => {{
-        $crate::log_with_args($crate::ffi::LogLevel::Error, format_args_nl!($msg));
+        $crate::log_with_args($crate::LogLevel::Error, format_args_nl!($msg));
         $crate::LoggedError::default()
     }};
     ($err:expr $(,)?) => {{
-        $crate::log_with_args($crate::ffi::LogLevel::Error, format_args_nl!("{}", $err));
+        $crate::log_with_args($crate::LogLevel::Error, format_args_nl!("{}", $err));
         $crate::LoggedError::default()
     }};
     ($($args:tt)+) => {{
-        $crate::log_with_args($crate::ffi::LogLevel::Error, format_args_nl!($($args)+));
+        $crate::log_with_args($crate::LogLevel::Error, format_args_nl!($($args)+));
         $crate::LoggedError::default()
     }};
 }

diff --git a/native/src/base/xwrap.rs b/native/src/base/xwrap.rs
@@ -26,19 +26,19 @@ fn error_str() -> &'static str {
 
 macro_rules! error_cxx {
     ($($args:tt)+) => {
-        ($crate::log_with_args($crate::ffi::LogLevel::ErrorCxx, format_args_nl!($($args)+)))
+        ($crate::log_with_args($crate::LogLevel::ErrorCxx, format_args_nl!($($args)+)))
     }
 }
 
 macro_rules! perror {
     ($fmt:expr) => {
-        $crate::log_with_formatter($crate::ffi::LogLevel::ErrorCxx, |w| {
+        $crate::log_with_formatter($crate::LogLevel::ErrorCxx, |w| {
             w.write_str($fmt)?;
             w.write_fmt(format_args_nl!(" failed with {}: {}", $crate::errno(), error_str()))
         })
     };
     ($fmt:expr, $($args:tt)*) => {
-        $crate::log_with_formatter($crate::ffi::LogLevel::ErrorCxx, |w| {
+        $crate::log_with_formatter($crate::LogLevel::ErrorCxx, |w| {
             w.write_fmt(format_args!($fmt, $($args)*))?;
             w.write_fmt(format_args_nl!(" failed with {}: {}", $crate::errno(), error_str()))
         })

diff --git a/native/src/core/logging.rs b/native/src/core/logging.rs
@@ -11,7 +11,6 @@ use bytemuck::{bytes_of, bytes_of_mut, write_zeroes, Pod, Zeroable};
 use num_derive::{FromPrimitive, ToPrimitive};
 use num_traits::FromPrimitive;
 
-use base::ffi::LogLevel;
 use base::libc::{
     clock_gettime, getpid, gettid, localtime_r, pipe2, pthread_sigmask, sigaddset, sigset_t,
     sigtimedwait, timespec, tm, CLOCK_REALTIME, O_CLOEXEC, PIPE_BUF, SIGPIPE, SIG_BLOCK,
@@ -50,11 +49,10 @@ extern "C" {
 
 fn level_to_prio(level: LogLevel) -> i32 {
     match level {
-        LogLevel::Error => ALogPriority::ANDROID_LOG_ERROR as i32,
+        LogLevel::Error | LogLevel::ErrorCxx => ALogPriority::ANDROID_LOG_ERROR as i32,
         LogLevel::Warn => ALogPriority::ANDROID_LOG_WARN as i32,
         LogLevel::Info => ALogPriority::ANDROID_LOG_INFO as i32,
         LogLevel::Debug => ALogPriority::ANDROID_LOG_DEBUG as i32,
-        _ => 0,
     }
 }
 

diff --git a/native/src/init/logging.rs b/native/src/init/logging.rs
@@ -3,12 +3,11 @@ use std::fs::File;
 use std::io::{IoSlice, Write};
 use std::sync::OnceLock;
 
-use base::ffi::LogLevel;
 use base::libc::{
     close, makedev, mknod, open, syscall, unlink, SYS_dup3, O_CLOEXEC, O_RDWR, STDERR_FILENO,
     STDIN_FILENO, STDOUT_FILENO, S_IFCHR,
 };
-use base::{cstr, exit_on_error, raw_cstr, Logger, Utf8CStr, LOGGER};
+use base::{cstr, exit_on_error, raw_cstr, LogLevel, Logger, Utf8CStr, LOGGER};
 
 static KMSG: OnceLock<File> = OnceLock::new();