fix filter behaviour, remove string allocation when reading lines

ClementTsang · ClementTsang · Apr 10, 2023 · Mar 9, 2023 · Mar 11, 2023 · Mar 14, 2023
commit c53a32ef5b92dfa2b6ce39b32ba192ab4649b95a
diff --git 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
@@ -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<String, Option<IoData>>;
+
+/// 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<Filter>, mount_filter: &Option<Filter>,
+) -> 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<Filter>, mount_filter: &Option<Filter>) -> Vec<usize> {
+        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
@@ -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<Filter>, &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<Filter>, &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<StorageSystemInformation> {
     // 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
@@ -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<IoHarvest> {
     let mut io_hash: HashMap<String, Option<IoData>> = 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<IoHarvest> {
     Ok(io_hash)
 }
 
+/// Returns the disk usage of the mounted (and for now, physical) disks.
 pub fn get_disk_usage(
     disk_filter: &Option<Filter>, mount_filter: &Option<Filter>,
 ) -> anyhow::Result<Vec<DiskHarvest>> {
@@ -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
@@ -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<IoStats> {
+    fn from_str(s: &str) -> anyhow::Result<IoCounters> {
         fn next_part<'a>(iter: &mut impl Iterator<Item = &'a str>) -> 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::<u64>()? * 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<impl Iterator<Item = anyhow::Result<IoStats>>> {
+pub fn io_stats() -> anyhow::Result<Vec<anyhow::Result<IoCounters>>> {
     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)
 }