cleanup(sinsp)!: clarify get_thread_ref vs find_thread#2694
cleanup(sinsp)!: clarify get_thread_ref vs find_thread#2694poiana merged 2 commits intofalcosecurity:masterfrom
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2694 +/- ##
==========================================
- Coverage 77.62% 77.62% -0.01%
==========================================
Files 294 294
Lines 31971 31947 -24
Branches 4721 4712 -9
==========================================
- Hits 24817 24798 -19
+ Misses 7154 7149 -5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
When query_os_if_not_found is false, get_thread_ref does the same thing as find_thread. Remove this indirection and call find_thread directly, whenever possible. Signed-off-by: Grzegorz Nosek <grzegorz.nosek@sysdig.com>
All the remaining uses of get_thread_ref want a proc lookup so remove the now redundant parameter. The method is renamed to get_thread to force a compile error when updating to a newer libs version, due to the two bool parameters with defaults that could be confused and end up with different semantics (the value of query_os_if_not_found would now become the value of lookup_only). Signed-off-by: Grzegorz Nosek <grzegorz.nosek@sysdig.com>
4536d7f to
6b06550
Compare
|
/milestone 0.23.0 |
|
Note: I hoped this would let me do some bigger cleanups later. Sadly, I underestimated the level to which all the components are entangled and I will have to keep the usergroup_manager reference inside the thread_manager no matter what, so this PR is not really a prerequisite to anything else and now stands on its own. I still feel it's nice to have just one way to do a non-/proc thread lookup rather than two though :) |
ekoops
left a comment
There was a problem hiding this comment.
Great improvement! In the future, we could also think about switching the name of these two: sinsp_thread_manager::get_thread() and sinsp_thread_manager::find_thread(). sinsp_thread_manager::find_thread() is just a simple getter, while sinsp_thread_manager::get_thread() can do proc scanning, so their current name are a bit misleading.
Anyway,
/approve
|
LGTM label has been added. DetailsGit tree hash: 1562c85d8d2737d8fe5d8200dfb7b26c7731cf02 |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ekoops, gnosek The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What type of PR is this?
/kind cleanup
Any specific area of the project related to this PR?
/area libsinsp
Does this PR require a change in the driver versions?
What this PR does / why we need it:
get_thread_refpotentially looks up a thread in /proc, which comes with extra dependencies (usergroup_manager, transitively via threadinfo), but in the majority of cases, it does not (query_os_if_not_found is usually false). In this case, it's equivalent tofind_thread, so replace these uses withfind_threadand makeget_thread_refalways do a /proc lookup (if needed).Also, rename
get_thread_reftoget_thread, to force a compilation error (otherwise thequery_os_if_not_foundparameter could be mistaken forlookup_onlyif the latter had a default specified).Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?: