diff --git a/CHANGELOG.md b/CHANGELOG.md index f226b9882..48b9fba52 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - [#1021](https://github.com/ClementTsang/bottom/pull/1021): Fix selected text background colour being wrong if only the foreground colour was set. - [#1037](https://github.com/ClementTsang/bottom/pull/1037): Fix `is_list_ignored` accepting all results if set to `false`. +- [#1064](https://github.com/ClementTsang/bottom/pull/1064): Disk name/mount filter was fixed to not just drop entries if one filter wasn't set. ## Features @@ -25,6 +26,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - [#1036](https://github.com/ClementTsang/bottom/pull/1036): Migrate away from heim for memory information; Linux platforms will also now try to use `MemAvailable` to determine used memory if supported. - [#1041](https://github.com/ClementTsang/bottom/pull/1041): Migrate away from heim for network information. +- [#1064](https://github.com/ClementTsang/bottom/pull/1064): Removed the Heim dependency. ## Other diff --git a/src/app/data_harvester/disks.rs b/src/app/data_harvester/disks.rs index 30e651db0..4292a57d6 100644 --- a/src/app/data_harvester/disks.rs +++ b/src/app/data_harvester/disks.rs @@ -2,6 +2,8 @@ use std::collections::HashMap; +use crate::app::filter::Filter; + cfg_if::cfg_if! { if #[cfg(target_os = "freebsd")] { pub mod freebsd; @@ -37,3 +39,110 @@ pub struct IoData { } pub type IoHarvest = HashMap>; + +/// Whether to keep the current disk entry given the filters, disk name, and disk mount. +/// Precedence ordering in the case where name and mount filters disagree, "allow" +/// takes precedence over "deny". +/// +/// For implementation, we do this as follows: +/// +/// 1. Is the entry allowed through any filter? That is, does it match an entry in a +/// filter where `is_list_ignored` is `false`? If so, we always keep this entry. +/// 2. Is the entry denied through any filter? That is, does it match an entry in a +/// filter where `is_list_ignored` is `true`? If so, we always deny this entry. +/// 3. Anything else is allowed. +pub(self) fn keep_disk_entry( + disk_name: &str, mount_point: &str, disk_filter: &Option, mount_filter: &Option, +) -> bool { + match (disk_filter, mount_filter) { + (Some(d), Some(m)) => match (d.is_list_ignored, m.is_list_ignored) { + (true, true) => !(d.has_match(disk_name) || m.has_match(mount_point)), + (true, false) => { + if m.has_match(mount_point) { + true + } else { + d.keep_entry(disk_name) + } + } + (false, true) => { + if d.has_match(disk_name) { + true + } else { + m.keep_entry(mount_point) + } + } + (false, false) => d.has_match(disk_name) || m.has_match(mount_point), + }, + (Some(d), None) => d.keep_entry(disk_name), + (None, Some(m)) => m.keep_entry(mount_point), + (None, None) => true, + } +} + +#[cfg(test)] +mod test { + use regex::Regex; + + use crate::app::filter::Filter; + + use super::keep_disk_entry; + + fn run_filter(disk_filter: &Option, mount_filter: &Option) -> Vec { + let targets = [ + ("/dev/nvme0n1p1", "/boot"), + ("/dev/nvme0n1p2", "/"), + ("/dev/nvme0n1p3", "/home"), + ("/dev/sda1", "/mnt/test"), + ("/dev/sda2", "/mnt/boot"), + ]; + + targets + .into_iter() + .enumerate() + .filter_map(|(itx, (name, mount))| { + if keep_disk_entry(name, mount, disk_filter, mount_filter) { + Some(itx) + } else { + None + } + }) + .collect() + } + + #[test] + fn test_keeping_disk_entry() { + let disk_ignore = Some(Filter { + is_list_ignored: true, + list: vec![Regex::new("nvme").unwrap()], + }); + + let disk_keep = Some(Filter { + is_list_ignored: false, + list: vec![Regex::new("nvme").unwrap()], + }); + + let mount_ignore = Some(Filter { + is_list_ignored: true, + list: vec![Regex::new("boot").unwrap()], + }); + + let mount_keep = Some(Filter { + is_list_ignored: false, + list: vec![Regex::new("boot").unwrap()], + }); + + assert_eq!(run_filter(&None, &None), vec![0, 1, 2, 3, 4]); + + assert_eq!(run_filter(&disk_ignore, &None), vec![3, 4]); + assert_eq!(run_filter(&disk_keep, &None), vec![0, 1, 2]); + + assert_eq!(run_filter(&None, &mount_ignore), vec![1, 2, 3]); + assert_eq!(run_filter(&None, &mount_keep), vec![0, 4]); + + assert_eq!(run_filter(&disk_ignore, &mount_ignore), vec![3]); + assert_eq!(run_filter(&disk_keep, &mount_ignore), vec![0, 1, 2, 3]); + + assert_eq!(run_filter(&disk_ignore, &mount_keep), vec![0, 3, 4]); + assert_eq!(run_filter(&disk_keep, &mount_keep), vec![0, 1, 2, 4]); + } +} diff --git a/src/app/data_harvester/disks/freebsd.rs b/src/app/data_harvester/disks/freebsd.rs index 1cf47882a..9238fc42d 100644 --- a/src/app/data_harvester/disks/freebsd.rs +++ b/src/app/data_harvester/disks/freebsd.rs @@ -4,14 +4,8 @@ use std::io; use serde::Deserialize; -use crate::{ - app::Filter, - data_harvester::{ - deserialize_xo, - disks::{DiskHarvest, IoHarvest}, - }, - utils::error, -}; +use super::{keep_disk_entry, DiskHarvest, IoData, IoHarvest}; +use crate::{app::Filter, data_harvester::deserialize_xo, utils::error}; #[derive(Deserialize, Debug, Default)] #[serde(rename_all = "kebab-case")] @@ -49,21 +43,7 @@ pub fn get_disk_usage( .filesystem .into_iter() .filter_map(|disk| { - // Precedence ordering in the case where name and mount filters disagree, "allow" - // takes precedence over "deny". - // - // For implementation, we do this as follows: - // - // 1. Is the entry allowed through any filter? That is, does it match an entry in a - // filter where `is_list_ignored` is `false`? If so, we always keep this entry. - // 2. Is the entry denied through any filter? That is, does it match an entry in a - // filter where `is_list_ignored` is `true`? If so, we always deny this entry. - // 3. Anything else is allowed. - let filter_check_map = - [(disk_filter, &disk.name), (mount_filter, &disk.mounted_on)]; - if matches_allow_list(filter_check_map.as_slice()) - || !matches_ignore_list(filter_check_map.as_slice()) - { + if keep_disk_entry(&disk.name, &disk.mounted_on, disk_filter, mount_filter) { Some(DiskHarvest { free_space: disk.available_blocks * 1024, used_space: disk.used_blocks * 1024, @@ -81,20 +61,6 @@ pub fn get_disk_usage( Ok(vec_disks) } -fn matches_allow_list(filter_check_map: &[(&Option, &String)]) -> bool { - filter_check_map.iter().any(|(filter, text)| match filter { - Some(f) if !f.is_list_ignored => f.list.iter().any(|r| r.is_match(text)), - Some(_) | None => false, - }) -} - -fn matches_ignore_list(filter_check_map: &[(&Option, &String)]) -> bool { - filter_check_map.iter().any(|(filter, text)| match filter { - Some(f) if f.is_list_ignored => f.list.iter().any(|r| r.is_match(text)), - Some(_) | None => false, - }) -} - fn get_disk_info() -> io::Result { // TODO: Ideally we don't have to shell out to a new program. let output = std::process::Command::new("df") diff --git a/src/app/data_harvester/disks/unix.rs b/src/app/data_harvester/disks/unix.rs index bc5ff3638..f57e1e297 100644 --- a/src/app/data_harvester/disks/unix.rs +++ b/src/app/data_harvester/disks/unix.rs @@ -18,13 +18,14 @@ cfg_if::cfg_if! { } } +use super::{keep_disk_entry, DiskHarvest, IoData, IoHarvest}; use crate::app::Filter; -use crate::data_harvester::disks::{DiskHarvest, IoData, IoHarvest}; +/// Returns the I/O usage of certain mount points. pub fn get_io_usage() -> anyhow::Result { let mut io_hash: HashMap> = HashMap::new(); - for io in io_stats()?.flatten() { + for io in io_stats()?.into_iter().flatten() { let mount_point = io.device_name().to_string_lossy(); io_hash.insert( @@ -39,6 +40,7 @@ pub fn get_io_usage() -> anyhow::Result { Ok(io_hash) } +/// Returns the disk usage of the mounted (and for now, physical) disks. pub fn get_disk_usage( disk_filter: &Option, mount_filter: &Option, ) -> anyhow::Result> { @@ -55,43 +57,7 @@ pub fn get_disk_usage( // 2. Is the entry denied through any filter? That is, does it match an entry in a filter where `is_list_ignored` is `true`? If so, we always deny this entry. // 3. Anything else is allowed. - let filter_check_map = [(disk_filter, &name), (mount_filter, &mount_point)]; - - // This represents case 1. That is, if there is a match in an allowing list - if there is, then - // immediately allow it! - let matches_allow_list = filter_check_map.iter().any(|(filter, text)| { - if let Some(filter) = filter { - if !filter.is_list_ignored { - for r in &filter.list { - if r.is_match(text) { - return true; - } - } - } - } - false - }); - - let to_keep = if matches_allow_list { - true - } else { - // If it doesn't match an allow list, then check if it is denied. - // That is, if it matches in a reject filter, then reject. Otherwise, we always keep it. - !filter_check_map.iter().any(|(filter, text)| { - if let Some(filter) = filter { - if filter.is_list_ignored { - for r in &filter.list { - if r.is_match(text) { - return true; - } - } - } - } - false - }) - }; - - if to_keep { + if keep_disk_entry(&name, &mount_point, disk_filter, mount_filter) { // The usage line can fail in some cases (for example, if you use Void Linux + LUKS, // see https://github.com/ClementTsang/bottom/issues/419 for details). if let Ok(usage) = partition.usage() { diff --git a/src/app/data_harvester/disks/unix/linux/io_counters.rs b/src/app/data_harvester/disks/unix/linux/io_counters.rs index c77bf3f52..ad788dcf1 100644 --- a/src/app/data_harvester/disks/unix/linux/io_counters.rs +++ b/src/app/data_harvester/disks/unix/linux/io_counters.rs @@ -23,7 +23,7 @@ const DISK_SECTOR_SIZE: u64 = 512; #[allow(dead_code)] #[derive(Debug, Default)] -pub struct IoStats { +pub struct IoCounters { name: String, read_count: u64, read_merged_count: u64, @@ -35,7 +35,7 @@ pub struct IoStats { write_time_secs: u64, } -impl IoStats { +impl IoCounters { pub(crate) fn device_name(&self) -> &OsStr { OsStr::new(&self.name) } @@ -49,7 +49,7 @@ impl IoStats { } } -impl FromStr for IoStats { +impl FromStr for IoCounters { type Err = anyhow::Error; /// Converts a `&str` to an [`IoStats`]. @@ -60,7 +60,7 @@ impl FromStr for IoStats { /// /// https://www.kernel.org/doc/Documentation/iostats.txt /// https://www.kernel.org/doc/Documentation/ABI/testing/procfs-diskstats - fn from_str(s: &str) -> anyhow::Result { + fn from_str(s: &str) -> anyhow::Result { fn next_part<'a>(iter: &mut impl Iterator) -> Result<&'a str, io::Error> { iter.next() .ok_or_else(|| io::Error::from(io::ErrorKind::InvalidData)) @@ -81,7 +81,7 @@ impl FromStr for IoStats { let write_bytes = next_part(&mut parts)?.parse::()? * DISK_SECTOR_SIZE; let write_time_secs = next_part(&mut parts)?.parse()?; - Ok(IoStats { + Ok(IoCounters { name, read_count, read_merged_count, @@ -96,13 +96,21 @@ impl FromStr for IoStats { } /// Returns an iterator of disk I/O stats. Pulls data from `/proc/diskstats`. -pub fn io_stats() -> anyhow::Result>> { +pub fn io_stats() -> anyhow::Result>> { const PROC_DISKSTATS: &str = "/proc/diskstats"; - Ok(BufReader::new(File::open(PROC_DISKSTATS)?) - .lines() - .map(|line| match line { - Ok(line) => IoStats::from_str(&line), - Err(err) => Err(err.into()), - })) + let mut results = vec![]; + let mut reader = BufReader::new(File::open(PROC_DISKSTATS)?); + let mut line = String::new(); + + while let Ok(bytes) = reader.read_line(&mut line) { + if bytes > 0 { + results.push(IoCounters::from_str(&line)); + line.clear(); + } else { + break; + } + } + + Ok(results) } diff --git a/src/app/data_harvester/disks/unix/linux/partition.rs b/src/app/data_harvester/disks/unix/linux/partition.rs index cf6471d02..f9b6faf04 100644 --- a/src/app/data_harvester/disks/unix/linux/partition.rs +++ b/src/app/data_harvester/disks/unix/linux/partition.rs @@ -23,16 +23,19 @@ pub(crate) struct Partition { impl Partition { /// Returns the device name, if there is one. + #[inline] pub fn device(&self) -> Option<&str> { self.device.as_deref() } /// Returns the mount point for this partition. + #[inline] pub fn mount_point(&self) -> &Path { self.mount_point.as_path() } /// Returns the [`FileSystem`] of this partition. + #[inline] pub fn fs_type(&self) -> &FileSystem { &self.fs_type } @@ -139,29 +142,51 @@ impl FromStr for Partition { } #[allow(dead_code)] -/// Returns all partitions. +/// Returns a [`Vec`] containing all partitions. pub(crate) fn partitions() -> anyhow::Result> { const PROC_MOUNTS: &str = "/proc/mounts"; - let mounts = BufReader::new(File::open(PROC_MOUNTS)?).lines(); - Ok(mounts - .filter_map(|line| match line { - Ok(line) => Partition::from_str(&line).ok(), - Err(_) => None, - }) - .collect()) + let mut results = vec![]; + let mut reader = BufReader::new(File::open(PROC_MOUNTS)?); + let mut line = String::new(); + + while let Ok(bytes) = reader.read_line(&mut line) { + if bytes > 0 { + if let Ok(partition) = Partition::from_str(&line) { + results.push(partition); + } + + line.clear(); + } else { + break; + } + } + + Ok(results) } -/// Returns all physical partitions. +/// Returns a [`Vec`] containing all *physical* partitions. This is defined by +/// [`FileSystem::is_physical()`]. pub(crate) fn partitions_physical() -> anyhow::Result> { const PROC_MOUNTS: &str = "/proc/mounts"; - let mounts = BufReader::new(File::open(PROC_MOUNTS)?).lines(); - Ok(mounts - .filter_map(|line| match line { - Ok(line) => Partition::from_str(&line).ok(), - Err(_) => None, - }) - .filter(|partition| partition.fs_type().is_physical()) - .collect()) + let mut results = vec![]; + let mut reader = BufReader::new(File::open(PROC_MOUNTS)?); + let mut line = String::new(); + + while let Ok(bytes) = reader.read_line(&mut line) { + if bytes > 0 { + if let Ok(partition) = Partition::from_str(&line) { + if partition.fs_type().is_physical() { + results.push(partition); + } + } + + line.clear(); + } else { + break; + } + } + + Ok(results) } diff --git a/src/app/data_harvester/disks/windows.rs b/src/app/data_harvester/disks/windows.rs index 79646dce3..fa3542a8a 100644 --- a/src/app/data_harvester/disks/windows.rs +++ b/src/app/data_harvester/disks/windows.rs @@ -2,20 +2,13 @@ use sysinfo::{DiskExt, System, SystemExt}; -use crate::{ - app::{ - data_harvester::disks::{DiskHarvest, IoHarvest}, - filter::Filter, - }, - utils::error, -}; +use super::{keep_disk_entry, DiskHarvest, IoData, IoHarvest}; +use crate::{app::filter::Filter, utils::error}; -// TODO: Unify this bit with Unix impl. pub fn get_io_usage() -> error::Result { Ok(IoHarvest::default()) } -// TODO: Unify this bit with Unix impl. pub(crate) fn get_disk_usage( sys: &System, disk_filter: &Option, mount_filter: &Option, ) -> Vec { @@ -42,7 +35,7 @@ pub(crate) fn get_disk_usage( .into_string() .unwrap_or_else(|_| "Mount unavailable".to_string()); - if keep_entry(&name, &mount_point, disk_filter, mount_filter) { + if keep_disk_entry(&name, &mount_point, disk_filter, mount_filter) { let free_space = disk.available_space(); let total_space = disk.total_space(); let used_space = total_space - free_space; @@ -60,52 +53,3 @@ pub(crate) fn get_disk_usage( }) .collect() } - -fn keep_entry( - name: &str, mount_point: &str, disk_filter: &Option, mount_filter: &Option, -) -> bool { - // Precedence ordering in the case where name and mount filters disagree, "allow" takes precedence over "deny". - // - // For implementation, we do this as follows: - // 1. Is the entry allowed through any filter? That is, does it match an entry in a filter where `is_list_ignored` is `false`? If so, we always keep this entry. - // 2. Is the entry denied through any filter? That is, does it match an entry in a filter where `is_list_ignored` is `true`? If so, we always deny this entry. - // 3. Anything else is allowed. - - let filter_check_map = [(disk_filter, &name), (mount_filter, &mount_point)]; - - // This represents case 1. That is, if there is a match in an allowing list - if there is, then - // immediately allow it! - let matches_allow_list = filter_check_map.iter().any(|(filter, text)| { - if let Some(filter) = filter { - if !filter.is_list_ignored { - for r in &filter.list { - if r.is_match(text) { - return true; - } - } - } - } - false - }); - - let to_keep = if matches_allow_list { - true - } else { - // If it doesn't match an allow list, then check if it is denied. - // That is, if it matches in a reject filter, then reject. Otherwise, we always keep it. - !filter_check_map.iter().any(|(filter, text)| { - if let Some(filter) = filter { - if filter.is_list_ignored { - for r in &filter.list { - if r.is_match(text) { - return true; - } - } - } - } - false - }) - }; - - to_keep -} diff --git a/src/app/filter.rs b/src/app/filter.rs index 10629d949..22c08e29d 100644 --- a/src/app/filter.rs +++ b/src/app/filter.rs @@ -9,7 +9,7 @@ impl Filter { /// Whether the filter should keep the entry or reject it. #[inline] pub(crate) fn keep_entry(&self, value: &str) -> bool { - if self.list.iter().any(|regex| regex.is_match(value)) { + if self.has_match(value) { // If a match is found, then if we wanted to ignore if we match, return false. If we want // to keep if we match, return true. Thus, return the inverse of `is_list_ignored`. !self.is_list_ignored @@ -17,6 +17,12 @@ impl Filter { self.is_list_ignored } } + + /// Whether there is a filter that matches the result. + #[inline] + pub(crate) fn has_match(&self, value: &str) -> bool { + self.list.iter().any(|regex| regex.is_match(value)) + } } #[cfg(test)]