diff --git a/CHANGELOG.md b/CHANGELOG.md index b62a4d67..b7dd8274 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,6 +26,10 @@ That said, these are more guidelines rather than hardset rules, though the proje - [#1793](https://github.com/ClementTsang/bottom/pull/1793): Add support for threads in Linux. +### Bug Fixes + +- [#1800](https://github.com/ClementTsang/bottom/pull/1800): Fix colon at end of process name in Linux. + ## [0.11.1] - 2025-08-15 ### Bug Fixes diff --git a/src/collection/processes/linux/mod.rs b/src/collection/processes/linux/mod.rs index 3619147f..9c349c39 100644 --- a/src/collection/processes/linux/mod.rs +++ b/src/collection/processes/linux/mod.rs @@ -137,7 +137,7 @@ fn read_proc( thread_parent: Option, ) -> CollectionResult<(ProcessHarvest, u64)> { let Process { - pid: _, + pid: _pid, uid, stat, io, @@ -221,39 +221,56 @@ fn read_proc( }; let (command, name) = { - let truncated_name = stat.comm; + let comm = stat.comm; if let Some(cmdline) = cmdline { if cmdline.is_empty() { - (concat_string!("[", truncated_name, "]"), truncated_name) + (concat_string!("[", comm, "]"), comm) } else { - let name = if truncated_name.len() >= MAX_STAT_NAME_LEN { - let first_part = match cmdline.split_once(' ') { - Some((first, _)) => first, - None => &cmdline, - }; + // If the comm fits then we'll default to whatever is set. + // If it doesn't, we need to do some magic to determine what it's + // supposed to be. + // + // We follow something similar to how htop does it to identify a valid name based on the cmdline. + // - https://github.com/htop-dev/htop/blob/bcb18ef82269c68d54a160290e5f8b2e939674ec/Process.c#L268 (kinda) + // - https://github.com/htop-dev/htop/blob/bcb18ef82269c68d54a160290e5f8b2e939674ec/Process.c#L573 + // + // Also note that cmdline is (for us) separated by \0. - // We're only interested in the executable part, not the file path (part of command), - // so strip everything but the command name if needed. - let command = match first_part.rsplit_once('/') { - Some((_, last)) => last, - None => first_part, - }; - - // TODO: Needed as some processes have stuff like "systemd-userwork: waiting..." - // command.trim_end_matches(':').to_string() - - command.to_string() + // TODO: We might want to re-evaluate if we want to do it like this, + // as it turns out I was dumb and sometimes comm != process name... + // + // What we should do is store: + // - basename (what we're kinda doing now, except we're gating on comm length) + // - command (full thing) + // - comm (as a separate thing) + // + // Stuff like htop also offers the option to "highlight" basename and comm in command. Might be neat? + let name = if comm.len() >= MAX_STAT_NAME_LEN { + name_from_cmdline(&cmdline) } else { - truncated_name + comm }; (cmdline, name) } } else { - (truncated_name.clone(), truncated_name) + (comm.clone(), comm) } }; + // We have moved command processing here. + // SAFETY: We are only replacing a single char (NUL) with another single char (space). + + let mut command = command; + let buf_mut = unsafe { command.as_mut_vec() }; + + for byte in buf_mut { + if *byte == 0 { + const SPACE: u8 = ' '.to_ascii_lowercase() as u8; + *byte = SPACE; + } + } + Ok(( ProcessHarvest { pid: process.pid, @@ -284,6 +301,22 @@ fn read_proc( )) } +fn name_from_cmdline(cmdline: &str) -> String { + let mut start = 0; + let mut end = cmdline.len(); + + for (i, c) in cmdline.chars().enumerate() { + if c == '/' { + start = i + 1; + } else if c == '\0' || c == ':' { + end = i; + break; + } + } + + cmdline[start..end].to_string() +} + pub(crate) struct PrevProc<'a> { pub prev_idle: &'a mut f64, pub prev_non_idle: &'a mut f64, @@ -502,4 +535,19 @@ mod tests { "Failed to properly calculate idle/non-idle for /proc/stat CPU with 10 values" ); } + + #[test] + fn test_name_from_cmdline() { + assert_eq!(name_from_cmdline("/usr/bin/btm"), "btm"); + assert_eq!(name_from_cmdline("/usr/bin/btm\0--asdf\0--asdf/gkj"), "btm"); + assert_eq!(name_from_cmdline("/usr/bin/btm:"), "btm"); + assert_eq!(name_from_cmdline("/usr/bin/b tm"), "b tm"); + assert_eq!(name_from_cmdline("/usr/bin/b tm:"), "b tm"); + assert_eq!(name_from_cmdline("/usr/bin/b tm\0--test"), "b tm"); + assert_eq!(name_from_cmdline("/usr/bin/b tm:\0--test"), "b tm"); + assert_eq!( + name_from_cmdline("/usr/bin/b t m:\0--\"test thing\""), + "b t m" + ); + } } diff --git a/src/collection/processes/linux/process.rs b/src/collection/processes/linux/process.rs index d17eb646..6d6f5b32 100644 --- a/src/collection/processes/linux/process.rs +++ b/src/collection/processes/linux/process.rs @@ -237,7 +237,7 @@ impl Process { ) -> anyhow::Result<(Process, Vec)> { buffer.clear(); - let fd = rustix::fs::openat( + let pid_dir = rustix::fs::openat( rustix::fs::CWD, pid_path.as_path(), OFlags::PATH | OFlags::DIRECTORY | OFlags::CLOEXEC, @@ -257,7 +257,7 @@ impl Process { .ok_or_else(|| anyhow!("PID for {pid_path:?} was not found"))?; let uid = { - let metadata = rustix::fs::fstat(&fd); + let metadata = rustix::fs::fstat(&pid_dir); match metadata { Ok(md) => Some(md.st_uid), Err(_) => None, @@ -271,10 +271,10 @@ impl Process { // Stat is pretty long, do this first to pre-allocate up-front. let stat = - open_at(&mut root, "stat", &fd).and_then(|file| Stat::from_file(file, buffer))?; + open_at(&mut root, "stat", &pid_dir).and_then(|file| Stat::from_file(file, buffer))?; reset(&mut root, buffer); - let cmdline = if cmdline(&mut root, &fd, buffer).is_ok() { + let cmdline = if cmdline(&mut root, &pid_dir, buffer).is_ok() { // The clone will give a string with the capacity of the length of buffer, don't worry. Some(buffer.clone()) } else { @@ -282,37 +282,13 @@ impl Process { }; reset(&mut root, buffer); - let io = open_at(&mut root, "io", &fd) + let io = open_at(&mut root, "io", &pid_dir) .and_then(|file| Io::from_file(file, buffer)) .ok(); reset(&mut root, buffer); - let threads = if get_threads { - root.push("task"); - - if let Ok(task) = std::fs::read_dir(root) { - let pid_str = pid.to_string(); - - task.flatten() - .filter_map(|thread_dir| { - let file_name = thread_dir.file_name(); - let file_name = file_name.to_string_lossy(); - let file_name = file_name.trim(); - - if is_str_numeric(file_name) && file_name != pid_str { - Some(thread_dir.path()) - } else { - None - } - }) - .collect::>() - } else { - Vec::new() - } - } else { - Vec::new() - }; + let threads = threads(&mut root, pid, get_threads); Ok(( Process { @@ -329,19 +305,7 @@ impl Process { #[inline] fn cmdline(root: &mut PathBuf, fd: &OwnedFd, buffer: &mut String) -> anyhow::Result<()> { - let _ = open_at(root, "cmdline", fd) - .map(|mut file| file.read_to_string(buffer)) - .inspect(|_| { - // SAFETY: We are only replacing a single char (NUL) with another single char (space). - let buf_mut = unsafe { buffer.as_mut_vec() }; - - for byte in buf_mut { - if *byte == 0 { - const SPACE: u8 = ' '.to_ascii_lowercase() as u8; - *byte = SPACE; - } - } - })?; + let _ = open_at(root, "cmdline", fd).map(|mut file| file.read_to_string(buffer))?; Ok(()) } @@ -356,3 +320,40 @@ fn open_at(root: &mut PathBuf, child: &str, fd: &OwnedFd) -> anyhow::Result Vec { + if get_threads { + root.push("task"); + + let Ok(task_dir) = rustix::fs::openat( + rustix::fs::CWD, + root.as_path(), + OFlags::RDONLY | OFlags::DIRECTORY | OFlags::CLOEXEC, + Mode::empty(), + ) else { + return Vec::new(); + }; + + if let Ok(task) = rustix::fs::Dir::read_from(task_dir) { + let pid_str = pid.to_string(); + + return task + .flatten() + .filter_map(|thread_dir| { + let file_name = thread_dir.file_name(); + let file_name = file_name.to_string_lossy(); + let file_name = file_name.trim(); + + if is_str_numeric(file_name) && file_name != pid_str { + Some(root.join(file_name).to_path_buf()) + } else { + None + } + }) + .collect::>(); + } + } + + Vec::new() +}