cleanup: remove some extra code #2186
Conversation
| if(res < 0) { | ||
| evt->set_errorcode(-(int32_t)res); | ||
| } | ||
| if(evt->has_return_value()) { |
There was a problem hiding this comment.
if i don't miss something all the syscall exit event should have at least the return value so i don't see why we need the additional manual check on the parameter names
There was a problem hiding this comment.
I encountered this code right yesterday and was wondering exactly the same 🤣
There was a problem hiding this comment.
BTW this logic was broken since there are all these possible values "uid", "gid", "res_or_fd", "euid", "egid"
|
LGTM label has been added. Git tree hash: b5d930a73bbd6992bceb57cbce9658d16d2aa4c0
|
|
Uh but tests are failing of course! |
Signed-off-by: Andrea Terzolo <[email protected]>
Andreagit97
force-pushed
the
Andreagit97/cleanup
branch
from
8c8aa15 to
9845c6e
Compare
| @@ -167,3 +167,26 @@ TEST(event_table, check_usage_of_EC_UNKNOWN_flag) { | |||
| } | |||
| } | |||
| } | |||
|
|
|||
| TEST(event_table, check_exit_param_names) { | |||
There was a problem hiding this comment.
I don't get this test; do we really need to enforce the param name from now on, given your changes?
There was a problem hiding this comment.
We should enforce the rule that the first parameter in the exit event is the return value of the syscall. Unfortunately, I've not found a great way to do that so for now we just monitor the parameters' names.
There was a problem hiding this comment.
it's a best effort check, if the name matches we are quite confident that the first parameter is a return value
There was a problem hiding this comment.
this already cached the weird case of `PPME_GENERIC_X'
There was a problem hiding this comment.
Ah i see, makes sense.
But then we are not using the check on param name at runtime anymore, so this is just to avoid that in the future a new syscall exit event is added whose first param is not a return code.
There was a problem hiding this comment.
exactly, today we are quite confident the rule is valid, this test tries to prevent future issues
Perf diff from master - unit testsHeap diff from master - unit testsHeap diff from master - scap fileBenchmarks diff from master |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2186 +/- ##
=======================================
Coverage 75.19% 75.20%
=======================================
Files 259 259
Lines 33899 33890 -9
Branches 5799 5801 +2
=======================================
- Hits 25492 25487 -5
+ Misses 8407 8403 -4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
LGTM label has been added. Git tree hash: 2bd540684cc5cf9789f005658d56e5243aa4aa64
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Andreagit97, FedeDP The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs 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?
No
What this PR does / why we need it:
if i don't miss something all the syscall exit event should have at least the return value so i don't see why we need the additional manual check on the parameter names
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?: