Skip to content

Commit

Permalink
Updates based on code review by @nastevens
Browse files Browse the repository at this point in the history
This addresses the following feeback:

Your code looks really good, and very Rustic. I only have a few comments:
· In ProcessRecord you use isize for the pid and ppid. These aren’t really sizes, so I’d go with i32
· Line 102: since all you’re doing is panic!() on Err, you can just use .unwrap()
· Line 107: you could use “filter_map” instead of filter and then “collect” the results. This eliminates the need for “mut records” on line 98
· Line 126: use “filter_map”
· I’d add Cargo support
· You could consider using a map type containing vector types for storing the ppid->pid translation. That would improve performance since the record list wouldn’t getting iterated over multiple times. Unfortunately Rust doesn’t have a multimap yet (see rust-lang/rfcs#784), otherwise you could just use that!

Using a map type for the lookup is not supported with this but will
be evaluated.
  • Loading branch information
posborne committed Mar 4, 2015
1 parent c28b00b commit 5823d36
Showing 1 changed file with 20 additions and 26 deletions.
46 changes: 20 additions & 26 deletions pstree.rs
Original file line number Diff line number Diff line change
Expand Up @@ -39,8 +39,8 @@ use std::old_io::BufferedReader;
#[derive(Clone,Debug)]
struct ProcessRecord {
name: String,
pid: isize,
ppid: isize,
pid: i32,
ppid: i32,
}

#[derive(Clone,Debug)]
Expand All @@ -65,8 +65,8 @@ impl ProcessTreeNode {
// Given a status file path, return a hashmap with the following form:
// pid -> ProcessRecord
fn get_process_record(status_path: &Path) -> Option<ProcessRecord> {
let mut pid : Option<isize> = None;
let mut ppid : Option<isize> = None;
let mut pid : Option<i32> = None;
let mut ppid : Option<i32> = None;
let mut name : Option<String> = None;

let mut status_file = BufferedReader::new(File::open(status_path));
Expand Down Expand Up @@ -95,41 +95,35 @@ fn get_process_record(status_path: &Path) -> Option<ProcessRecord> {

// build a simple struct (ProcessRecord) for each process
fn get_process_records() -> Vec<ProcessRecord> {
let mut records : Vec<ProcessRecord> = Vec::new();
let proc_directory = Path::new("/proc");

// find potential process directories under /proc
let proc_directory_contents = match fs::readdir(&proc_directory) {
Err(why) => panic!("{}", why.desc),
Ok(res) => res
};

for entry in proc_directory_contents.iter().filter(|entry| entry.is_dir()) {
let status_path = entry.join("status");
if status_path.exists() {
match get_process_record(&status_path) {
Some(record) => {
records.push(record)
},
None => (),
let proc_directory_contents = fs::readdir(&proc_directory).unwrap();
proc_directory_contents.iter().filter_map(|entry| {
if entry.is_dir() {
let status_path = entry.join("status");
if status_path.exists() {
return get_process_record(&status_path)
}
}
}
records
None
}).collect()
}

fn populate_node(node : &mut ProcessTreeNode, records: &Vec<ProcessRecord>) {
// populate the node by finding its children... recursively
let pid = node.record.pid; // avoid binding node as immutable in closure
node.children.extend(
records.iter()
.filter(|record| record.ppid == pid)
.map(|record| {
records.iter().filter_map(|record| {
if record.ppid == pid {
let mut child = ProcessTreeNode::new(record);
populate_node(&mut child, records);
child
})
);
Some(child)
} else {
None
}
})
)
}

fn build_process_tree() -> ProcessTree {
Expand Down

0 comments on commit 5823d36

Please sign in to comment.