Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bug: add bindings to grab ppid in some cases on macos #825

Merged
merged 1 commit into from
Oct 11, 2022

Conversation

ClementTsang
Copy link
Owner

@ClementTsang ClementTsang commented Oct 11, 2022

Description

A description of the change and what it does. If relevant (such as any change that modifies the UI), please provide screenshots of the change:

The sysinfo crate grabs data in a way that sometimes will not work based on permissions on macOS, leading to some missing data - one case this comes up is parent PIDs, particularly noticeable in tree mode. Unless you run the program with sudo, if the parent PID is 1, the current sysinfo crate will fail to grab the parent PID for a process.

This can be circumvented by taking a page from htop's book and use kinfo_procs (see here). This is actually what heim uses, and we can leverage some of its implementation over here to grab ppid in cases that sysinfo fails. In the future, we can further leverage this to get even more data (e.g. for missing processes).

We can see this demonstrated here - left is the new implementation, right is the old. We see how the left one correctly shows the tree structure since it can actually detect the PPIDs, while the right fails to do this correctly since it cannot find the PPID for some of the processes and ends up promoting those processes to orphans.

image

Issue

If applicable, what issue does this address?

Closes: #

Testing

If relevant, please state how this was tested. All changes must be tested to work:

If this is a code change, please also indicate which platforms were tested:

  • Windows
  • macOS
  • Linux

Checklist

If relevant, ensure the following have been met:

  • Areas your change affects have been linted using rustfmt (cargo fmt)
  • The change has been tested and doesn't appear to cause any unintended breakage
  • Documentation has been added/updated if needed (README.md, help menu, etc.)
  • The pull request passes the provided CI pipeline
  • There are no merge conflicts
  • If relevant, new tests were added (don't worry too much about coverage)

@codecov-commenter
Copy link

Codecov Report

Base: 23.57% // Head: 23.57% // No change to project coverage 👍

Coverage data is based on head (5459b02) compared to base (e7b31df).
Patch coverage: 0.00% of modified lines in pull request are covered.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #825   +/-   ##
=======================================
  Coverage   23.57%   23.57%           
=======================================
  Files          60       60           
  Lines       13291    13291           
=======================================
  Hits         3133     3133           
  Misses      10158    10158           
Impacted Files Coverage Δ
src/app/data_farmer.rs 8.36% <0.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@ClementTsang ClementTsang merged commit 1e5f0ea into master Oct 11, 2022
@ClementTsang ClementTsang deleted the macos_process_bindings branch October 11, 2022 23:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants