utils: adds a basic thread/pid api implementation. - #6
Conversation
jodh-intel
left a comment
jodh-intel
left a comment
There was a problem hiding this comment.
Thanks, @dborquez - a few comments...
| let c_path = fs::canonicalize(proc_path)?; | ||
|
|
||
| // make the task path | ||
| let tid_path = fs::canonicalize(Path::new(&c_path).join("task"))?; |
There was a problem hiding this comment.
I think you can drop this second call to canonicalize since at this point, c_path is already canonical so it's safe to append our own path element to it.
| Err(_) => continue, | ||
| }; | ||
|
|
||
| let tid: u32 = tid_str.parse().expect("Not a valid tid was found"); |
There was a problem hiding this comment.
Can you replace the expect() call by ?
| let tid: u32 = tid_str.parse().expect("Not a valid tid was found"); | |
| let tid: u32 = tid_str.parse()?; |
Or,
| let tid: u32 = tid_str.parse().expect("Not a valid tid was found"); | |
| let tid: u32 = tid_str.parse().map_err(|e| anyhow!("Not valid tid found: {e}"))?; |
| return Err(anyhow!("Status information about the process was not found.")); | ||
| } | ||
|
|
||
| let cpu_id: u32 = vect_proc_pid_stat[CPU_POS_STAT].parse().expect("Not a valid cpu id."); |
There was a problem hiding this comment.
Again, can you return the error and not call expect() to ensure we don't panic.
| let tid_path = Path::new("/proc").join(tid_str.clone()).join("stat"); | ||
|
|
||
| if !tid_path.exists() { | ||
| return Err(anyhow!("Tid stat was not found.")); |
There was a problem hiding this comment.
Nit: We're using a combination of "tid" and "Tid", so please can you choose one of those (or "TID") and use it consistently everywhere :)
| } | ||
|
|
||
| let cpu_id: u32 = vect_proc_pid_stat[CPU_POS_STAT].parse().expect("Not a valid cpu id."); | ||
| tids.insert(tid,cpu_id); |
There was a problem hiding this comment.
Can you either run rustfmt manually on the changes, or set up your editor to do this automatically as this doesn't appear to have been formatted (missing space).
313c1fd to
4558cf7
Compare
c3b7b41 to
16d3cf3
Compare
jodh-intel
left a comment
jodh-intel
left a comment
There was a problem hiding this comment.
Thanks, @dborquez - just a couple of comments...
| let src = std::fs::canonicalize(proc_path) | ||
| .map_err(|e| anyhow!("Invalid /proc/pid path: {proc_path}: {e}"))?; | ||
|
|
||
| if !src.metadata().unwrap().is_dir() { |
There was a problem hiding this comment.
We can't have any unwrap() or expect() calls in the "real" code so can you replace that with ? here?
See: https://github.com/kata-containers/kata-containers/blob/main/docs/code-pr-advice.md#unsafe-code
16d3cf3 to
4a06519
Compare
| Err(_) => continue, | ||
| }; | ||
|
|
||
| let comm_path = Path::new("/proc").join(tid_str.clone()).join("comm"); |
There was a problem hiding this comment.
I don't think this is correct as we should be looking at /proc/$cloud_hypervisor_pid/task/*/comm only.
4a06519 to
678989d
Compare
Draft version of the get_thread_ids that constructs the /proc/$pid/task and returns a tid-vcpu hashmap. Signed-off-by: David Esparza <david.esparza.borquez@intel.com>
678989d to
ecf269e
Compare
jodh-intel
left a comment
jodh-intel
left a comment
There was a problem hiding this comment.
Thanks, @dborquez. Just one nit.
I think you are now ready to add get_ch_vcpu_tids_by_path(), get_ch_vcpu_tids() and the tests into get_thread_ids() in src/runtime-rs/crates/hypervisor/src/ch/inner_hypervisor.rs.
Before you raise the PR, please test that you can start and stop a container using runtime-rs and CH.
|
|
||
| // Create a map of tids-vcpus from /proc/pid/task subdirectories | ||
| pub fn get_ch_vcpu_tids_by_path(proc_path: &str) -> Result<HashMap<u32, u32>> { | ||
| let mut tids = HashMap::new(); |
There was a problem hiding this comment.
Nit: I'd move this to just above the for loop since if the specified proc_path isn't valid, we've constructed this unecessarily.
2 participants
Draft version of the get_thread_ids that constructs the /proc/$pid/task and returns a tid-vcpu hashmap.