From 0401f527e5ec18bafa351c1104b12b5690765b61 Mon Sep 17 00:00:00 2001 From: Clement Tsang <34804052+ClementTsang@users.noreply.github.com> Date: Mon, 22 Jul 2024 00:52:38 -0400 Subject: [PATCH] refactor: migrate data collection and some others away from BottomError (#1497) --- src/canvas.rs | 3 +- src/data_collection.rs | 3 +- src/data_collection/cpu/sysinfo.rs | 8 +- src/data_collection/disks/freebsd.rs | 6 +- src/data_collection/error.rs | 30 ++++ src/data_collection/processes.rs | 5 +- src/data_collection/processes/linux.rs | 161 +++++++++--------- src/data_collection/processes/unix.rs | 4 +- .../processes/unix/process_ext.rs | 7 +- .../processes/unix/user_table.rs | 8 +- src/data_collection/processes/windows.rs | 3 +- src/main.rs | 7 +- src/utils/error.rs | 9 - 13 files changed, 132 insertions(+), 122 deletions(-) create mode 100644 src/data_collection/error.rs diff --git a/src/canvas.rs b/src/canvas.rs index 547a3e13..35096ee5 100644 --- a/src/canvas.rs +++ b/src/canvas.rs @@ -23,7 +23,6 @@ use crate::{ }, constants::*, options::OptionError, - utils::error, }; #[derive(Debug)] @@ -205,7 +204,7 @@ impl Painter { pub fn draw_data( &mut self, terminal: &mut Terminal, app_state: &mut App, - ) -> error::Result<()> { + ) -> Result<(), std::io::Error> { use BottomWidgetType::*; terminal.draw(|f| { diff --git a/src/data_collection.rs b/src/data_collection.rs index 45c8fe0a..d37931f9 100644 --- a/src/data_collection.rs +++ b/src/data_collection.rs @@ -8,6 +8,7 @@ pub mod batteries; pub mod cpu; pub mod disks; +pub mod error; pub mod memory; pub mod network; pub mod processes; @@ -367,7 +368,7 @@ impl DataCollector { #[cfg(target_family = "unix")] { - self.data.load_avg = cpu::get_load_avg().ok(); + self.data.load_avg = Some(cpu::get_load_avg()); } } } diff --git a/src/data_collection/cpu/sysinfo.rs b/src/data_collection/cpu/sysinfo.rs index 707822f9..72a8a558 100644 --- a/src/data_collection/cpu/sysinfo.rs +++ b/src/data_collection/cpu/sysinfo.rs @@ -6,9 +6,9 @@ use std::collections::VecDeque; use sysinfo::{LoadAvg, System}; use super::{CpuData, CpuDataType, CpuHarvest}; -use crate::data_collection::cpu::LoadAvgHarvest; +use crate::data_collection::{cpu::LoadAvgHarvest, error::CollectionResult}; -pub fn get_cpu_data_list(sys: &System, show_average_cpu: bool) -> crate::error::Result { +pub fn get_cpu_data_list(sys: &System, show_average_cpu: bool) -> CollectionResult { let mut cpu_deque: VecDeque<_> = sys .cpus() .iter() @@ -31,10 +31,10 @@ pub fn get_cpu_data_list(sys: &System, show_average_cpu: bool) -> crate::error:: Ok(Vec::from(cpu_deque)) } -pub fn get_load_avg() -> crate::error::Result { +pub fn get_load_avg() -> LoadAvgHarvest { // The API for sysinfo apparently wants you to call it like this, rather than // using a &System. let LoadAvg { one, five, fifteen } = sysinfo::System::load_average(); - Ok([one as f32, five as f32, fifteen as f32]) + [one as f32, five as f32, fifteen as f32] } diff --git a/src/data_collection/disks/freebsd.rs b/src/data_collection/disks/freebsd.rs index 095c5b12..11629e16 100644 --- a/src/data_collection/disks/freebsd.rs +++ b/src/data_collection/disks/freebsd.rs @@ -7,7 +7,7 @@ use serde::Deserialize; use super::{keep_disk_entry, DiskHarvest, IoHarvest}; use crate::{ - data_collection::{deserialize_xo, disks::IoData, DataCollector}, + data_collection::{deserialize_xo, disks::IoData, error::CollectionResult, DataCollector}, utils::error, }; @@ -27,7 +27,7 @@ struct FileSystem { mounted_on: String, } -pub fn get_io_usage() -> error::Result { +pub fn get_io_usage() -> CollectionResult { // TODO: Should this (and other I/O collectors) fail fast? In general, should // collection ever fail fast? #[allow(unused_mut)] @@ -59,7 +59,7 @@ pub fn get_io_usage() -> error::Result { Ok(io_harvest) } -pub fn get_disk_usage(collector: &DataCollector) -> error::Result> { +pub fn get_disk_usage(collector: &DataCollector) -> CollectionResult> { let disk_filter = &collector.filters.disk_filter; let mount_filter = &collector.filters.mount_filter; let vec_disks: Vec = get_disk_info().map(|storage_system_information| { diff --git a/src/data_collection/error.rs b/src/data_collection/error.rs new file mode 100644 index 00000000..83fe2d0e --- /dev/null +++ b/src/data_collection/error.rs @@ -0,0 +1,30 @@ +use anyhow::anyhow; + +/// An error to do with data collection. +#[derive(Debug)] +pub enum CollectionError { + /// A general error to propagate back up. A wrapper around [`anyhow::Error`]. + General(anyhow::Error), + + /// The collection is unsupported. + Unsupported, +} + +impl CollectionError { + // pub(crate) fn general>(error: E) -> Self { + // Self::General(error.into()) + // } + + pub(crate) fn from_str(msg: &'static str) -> Self { + Self::General(anyhow!(msg)) + } +} + +/// A [`Result`] with the error type being a [`DataCollectionError`]. +pub(crate) type CollectionResult = Result; + +impl From for CollectionError { + fn from(err: std::io::Error) -> Self { + CollectionError::General(err.into()) + } +} diff --git a/src/data_collection/processes.rs b/src/data_collection/processes.rs index 7359ede2..03854049 100644 --- a/src/data_collection/processes.rs +++ b/src/data_collection/processes.rs @@ -33,8 +33,7 @@ cfg_if! { use std::{borrow::Cow, time::Duration}; -use super::DataCollector; -use crate::utils::error; +use super::{error::CollectionResult, DataCollector}; cfg_if! { if #[cfg(target_family = "windows")] { @@ -131,7 +130,7 @@ impl ProcessHarvest { } impl DataCollector { - pub(crate) fn get_processes(&mut self) -> error::Result> { + pub(crate) fn get_processes(&mut self) -> CollectionResult> { cfg_if! { if #[cfg(target_os = "linux")] { let time_diff = self.data.collection_time diff --git a/src/data_collection/processes/linux.rs b/src/data_collection/processes/linux.rs index 87a3a21d..6de841f1 100644 --- a/src/data_collection/processes/linux.rs +++ b/src/data_collection/processes/linux.rs @@ -13,10 +13,7 @@ use process::*; use sysinfo::ProcessStatus; use super::{Pid, ProcessHarvest, UserTable}; -use crate::{ - data_collection::DataCollector, - utils::error::{self, BottomError}, -}; +use crate::data_collection::{error::CollectionResult, DataCollector}; /// Maximum character length of a `/proc//stat`` process name. /// If it's equal or greater, then we instead refer to the command for the name. @@ -64,7 +61,9 @@ struct CpuUsage { cpu_fraction: f64, } -fn cpu_usage_calculation(prev_idle: &mut f64, prev_non_idle: &mut f64) -> error::Result { +fn cpu_usage_calculation( + prev_idle: &mut f64, prev_non_idle: &mut f64, +) -> CollectionResult { let (idle, non_idle) = { // From SO answer: https://stackoverflow.com/a/23376195 let first_line = { @@ -132,7 +131,7 @@ fn get_linux_cpu_usage( fn read_proc( prev_proc: &PrevProcDetails, process: Process, args: ReadProcArgs, user_table: &mut UserTable, -) -> error::Result<(ProcessHarvest, u64)> { +) -> CollectionResult<(ProcessHarvest, u64)> { let Process { pid: _, uid, @@ -298,7 +297,7 @@ pub(crate) struct ReadProcArgs { pub(crate) fn linux_process_data( collector: &mut DataCollector, time_difference_in_secs: u64, -) -> error::Result> { +) -> CollectionResult> { let total_memory = collector.total_memory(); let prev_proc = PrevProc { prev_idle: &mut collector.prev_idle, @@ -323,88 +322,82 @@ pub(crate) fn linux_process_data( // TODO: [PROC THREADS] Add threads - if let Ok(CpuUsage { + let CpuUsage { mut cpu_usage, cpu_fraction, - }) = cpu_usage_calculation(prev_idle, prev_non_idle) - { - if unnormalized_cpu { - let num_processors = collector.sys.system.cpus().len() as f64; + } = cpu_usage_calculation(prev_idle, prev_non_idle)?; - // Note we *divide* here because the later calculation divides `cpu_usage` - in - // effect, multiplying over the number of cores. - cpu_usage /= num_processors; - } + if unnormalized_cpu { + let num_processors = collector.sys.system.cpus().len() as f64; - let mut pids_to_clear: HashSet = pid_mapping.keys().cloned().collect(); - - let pids = fs::read_dir("/proc")?.flatten().filter_map(|dir| { - if is_str_numeric(dir.file_name().to_string_lossy().trim()) { - Some(dir.path()) - } else { - None - } - }); - - let args = ReadProcArgs { - use_current_cpu_total, - cpu_usage, - cpu_fraction, - total_memory, - time_difference_in_secs, - uptime: sysinfo::System::uptime(), - }; - - let process_vector: Vec = pids - .filter_map(|pid_path| { - if let Ok(process) = Process::from_path(pid_path) { - let pid = process.pid; - let prev_proc_details = pid_mapping.entry(pid).or_default(); - - #[allow(unused_mut)] - if let Ok((mut process_harvest, new_process_times)) = - read_proc(prev_proc_details, process, args, user_table) - { - #[cfg(feature = "gpu")] - if let Some(gpus) = &collector.gpu_pids { - gpus.iter().for_each(|gpu| { - // add mem/util for all gpus to pid - if let Some((mem, util)) = gpu.get(&(pid as u32)) { - process_harvest.gpu_mem += mem; - process_harvest.gpu_util += util; - } - }); - if let Some(gpu_total_mem) = &collector.gpus_total_mem { - process_harvest.gpu_mem_percent = (process_harvest.gpu_mem as f64 - / *gpu_total_mem as f64 - * 100.0) - as f32; - } - } - - prev_proc_details.cpu_time = new_process_times; - prev_proc_details.total_read_bytes = process_harvest.total_read_bytes; - prev_proc_details.total_write_bytes = process_harvest.total_write_bytes; - - pids_to_clear.remove(&pid); - return Some(process_harvest); - } - } - - None - }) - .collect(); - - pids_to_clear.iter().for_each(|pid| { - pid_mapping.remove(pid); - }); - - Ok(process_vector) - } else { - Err(BottomError::GenericError( - "Could not calculate CPU usage.".to_string(), - )) + // Note we *divide* here because the later calculation divides `cpu_usage` - in + // effect, multiplying over the number of cores. + cpu_usage /= num_processors; } + + let mut pids_to_clear: HashSet = pid_mapping.keys().cloned().collect(); + + let pids = fs::read_dir("/proc")?.flatten().filter_map(|dir| { + if is_str_numeric(dir.file_name().to_string_lossy().trim()) { + Some(dir.path()) + } else { + None + } + }); + + let args = ReadProcArgs { + use_current_cpu_total, + cpu_usage, + cpu_fraction, + total_memory, + time_difference_in_secs, + uptime: sysinfo::System::uptime(), + }; + + let process_vector: Vec = pids + .filter_map(|pid_path| { + if let Ok(process) = Process::from_path(pid_path) { + let pid = process.pid; + let prev_proc_details = pid_mapping.entry(pid).or_default(); + + #[allow(unused_mut)] + if let Ok((mut process_harvest, new_process_times)) = + read_proc(prev_proc_details, process, args, user_table) + { + #[cfg(feature = "gpu")] + if let Some(gpus) = &collector.gpu_pids { + gpus.iter().for_each(|gpu| { + // add mem/util for all gpus to pid + if let Some((mem, util)) = gpu.get(&(pid as u32)) { + process_harvest.gpu_mem += mem; + process_harvest.gpu_util += util; + } + }); + if let Some(gpu_total_mem) = &collector.gpus_total_mem { + process_harvest.gpu_mem_percent = + (process_harvest.gpu_mem as f64 / *gpu_total_mem as f64 * 100.0) + as f32; + } + } + + prev_proc_details.cpu_time = new_process_times; + prev_proc_details.total_read_bytes = process_harvest.total_read_bytes; + prev_proc_details.total_write_bytes = process_harvest.total_write_bytes; + + pids_to_clear.remove(&pid); + return Some(process_harvest); + } + } + + None + }) + .collect(); + + pids_to_clear.iter().for_each(|pid| { + pid_mapping.remove(pid); + }); + + Ok(process_vector) } #[cfg(test)] diff --git a/src/data_collection/processes/unix.rs b/src/data_collection/processes/unix.rs index ea1a3701..2656a583 100644 --- a/src/data_collection/processes/unix.rs +++ b/src/data_collection/processes/unix.rs @@ -13,9 +13,9 @@ cfg_if! { use super::ProcessHarvest; use crate::data_collection::{DataCollector, processes::*}; - use crate::utils::error; + use crate::data_collection::error::CollectionResult; - pub fn sysinfo_process_data(collector: &mut DataCollector) -> error::Result> { + pub fn sysinfo_process_data(collector: &mut DataCollector) -> CollectionResult> { let sys = &collector.sys.system; let use_current_cpu_total = collector.use_current_cpu_total; let unnormalized_cpu = collector.unnormalized_cpu; diff --git a/src/data_collection/processes/unix/process_ext.rs b/src/data_collection/processes/unix/process_ext.rs index eeef07ce..953549eb 100644 --- a/src/data_collection/processes/unix/process_ext.rs +++ b/src/data_collection/processes/unix/process_ext.rs @@ -6,16 +6,13 @@ use hashbrown::HashMap; use sysinfo::{ProcessStatus, System}; use super::ProcessHarvest; -use crate::{ - data_collection::{processes::UserTable, Pid}, - utils::error, -}; +use crate::data_collection::{error::CollectionResult, processes::UserTable, Pid}; pub(crate) trait UnixProcessExt { fn sysinfo_process_data( sys: &System, use_current_cpu_total: bool, unnormalized_cpu: bool, total_memory: u64, user_table: &mut UserTable, - ) -> error::Result> { + ) -> CollectionResult> { let mut process_vector: Vec = Vec::new(); let process_hashmap = sys.processes(); let cpu_usage = sys.global_cpu_info().cpu_usage() as f64 / 100.0; diff --git a/src/data_collection/processes/unix/user_table.rs b/src/data_collection/processes/unix/user_table.rs index ce0c61ff..50edaba2 100644 --- a/src/data_collection/processes/unix/user_table.rs +++ b/src/data_collection/processes/unix/user_table.rs @@ -1,6 +1,6 @@ use hashbrown::HashMap; -use crate::utils::error::{self, BottomError}; +use crate::data_collection::error::{CollectionError, CollectionResult}; #[derive(Debug, Default)] pub struct UserTable { @@ -8,7 +8,7 @@ pub struct UserTable { } impl UserTable { - pub fn get_uid_to_username_mapping(&mut self, uid: libc::uid_t) -> error::Result { + pub fn get_uid_to_username_mapping(&mut self, uid: libc::uid_t) -> CollectionResult { if let Some(user) = self.uid_user_mapping.get(&uid) { Ok(user.clone()) } else { @@ -17,12 +17,12 @@ impl UserTable { let passwd = unsafe { libc::getpwuid(uid) }; if passwd.is_null() { - Err(BottomError::GenericError("passwd is inaccessible".into())) + Err(CollectionError::from_str("passwd is inaccessible")) } else { // SAFETY: We return early if passwd is null. let username = unsafe { std::ffi::CStr::from_ptr((*passwd).pw_name) } .to_str() - .map_err(|err| BottomError::GenericError(err.to_string()))? + .map_err(|err| CollectionError::General(err.into()))? .to_string(); self.uid_user_mapping.insert(uid, username.clone()); diff --git a/src/data_collection/processes/windows.rs b/src/data_collection/processes/windows.rs index 8d59f515..ba6be2b8 100644 --- a/src/data_collection/processes/windows.rs +++ b/src/data_collection/processes/windows.rs @@ -3,12 +3,13 @@ use std::time::Duration; use super::ProcessHarvest; +use crate::data_collection::error::CollectionResult; use crate::data_collection::DataCollector; // TODO: There's a lot of shared code with this and the unix impl. pub fn sysinfo_process_data( collector: &mut DataCollector, -) -> crate::utils::error::Result> { +) -> CollectionResult> { let sys = &collector.sys.system; let users = &collector.sys.users; let use_current_cpu_total = collector.use_current_cpu_total; diff --git a/src/main.rs b/src/main.rs index cb30aed9..66805bd7 100644 --- a/src/main.rs +++ b/src/main.rs @@ -52,7 +52,6 @@ use data_conversion::*; use event::{handle_key_event_or_break, handle_mouse_event, BottomEvent, CollectionThreadEvent}; use options::{args, get_color_scheme, get_config_path, get_or_create_config, init_app}; use tui::{backend::CrosstermBackend, Terminal}; -use utils::error; #[allow(unused_imports)] use utils::logging::*; @@ -64,10 +63,10 @@ use utils::logging::*; fn try_drawing( terminal: &mut Terminal>, app: &mut App, painter: &mut canvas::Painter, -) -> error::Result<()> { +) -> anyhow::Result<()> { if let Err(err) = painter.draw_data(terminal, app) { cleanup_terminal(terminal)?; - Err(err) + Err(err.into()) } else { Ok(()) } @@ -76,7 +75,7 @@ fn try_drawing( /// Clean up the terminal before returning it to the user. fn cleanup_terminal( terminal: &mut Terminal>, -) -> error::Result<()> { +) -> anyhow::Result<()> { disable_raw_mode()?; execute!( terminal.backend_mut(), diff --git a/src/utils/error.rs b/src/utils/error.rs index ba8a92fa..5d96f762 100644 --- a/src/utils/error.rs +++ b/src/utils/error.rs @@ -8,16 +8,7 @@ pub type Result = result::Result; /// An error that can occur while Bottom runs. #[derive(Debug, Error, PartialEq, Eq)] pub enum BottomError { - /// An error when there is an IO exception. - #[error("IO exception, {0}")] - InvalidIo(String), /// An error to represent generic errors. #[error("Error, {0}")] GenericError(String), } - -impl From for BottomError { - fn from(err: std::io::Error) -> Self { - BottomError::InvalidIo(err.to_string()) - } -}