cleanup(libsinsp): re-order proc filtercheck fields + more unit testing #962
Conversation
| scap_const_sized_buffer empty_bytebuf = {.buf = nullptr, .size = 0}; | ||
|
|
||
| add_event_advance_ts(increasing_ts(), parent_tid, PPME_SYSCALL_CLONE_20_E, 0); | ||
| std::vector<std::string> cgroups = {"cpuset=/", "cpu=/user.slice", "cpuacct=/user.slice", "io=/user.slice", "memory=/user.slice/user-1000.slice/session-1.scope", "devices=/user.slice", "freezer=/", "net_cls=/", "perf_event=/", "net_prio=/", "hugetlb=/", "pids=/user.slice/user-1000.slice/session-1.scope", "rdma=/", "misc=/"}; | ||
| std::string cgroupsv = test_utils::to_null_delimited(cgroups); | ||
| std::vector<std::string> env = {"SHELL=/bin/bash", "PWD=/home/user", "HOME=/home/user"}; | ||
| std::string envv = test_utils::to_null_delimited(env); | ||
| std::vector<std::string> args = {"--help"}; | ||
| std::vector<std::string> args = {"-c", "'echo aGVsbG8K | base64 -d'"}; |
There was a problem hiding this comment.
is that change done to test multiple arguments to the binary?
|
I like this! 🚀 Thanks a lot! |
| @@ -329,27 +329,48 @@ class sinsp_filter_check_thread : public sinsp_filter_check | |||
| public: | |||
| enum check_type | |||
| { | |||
| TYPE_PID = 0, | |||
| TYPE_EXE, | |||
| TYPE_EXE = 0, | |||
There was a problem hiding this comment.
I am not sure: given this is a public enum, isn't this an API break?
I mean i know that nobody links libsinsp as a shared library, and we already broke API since 0.10.x release, but i am still unsure about this.
There was a problem hiding this comment.
Where is this used publicly? It looks to me that this is first mapped in order to the filterchecks defined as string and only used later, I believe I may be missing something
There was a problem hiding this comment.
It is not used at least by us, but it is exposed in a public header, right?
Perhaps i am wrong :)
There was a problem hiding this comment.
Also think it's a non-breaking change (definitely for Falco). We can add a release note with ! for sure, just in case.
|
@LucaGuerra one more thought: As you are fixing up the test framework, why don't we move the unit test updates from here to your PR? I'll then rebase and squash. |
|
/milestone 0.11.0 |
Signed-off-by: Melissa Kilby <[email protected]>
…ions Signed-off-by: Melissa Kilby <[email protected]>
Signed-off-by: Melissa Kilby <[email protected]>
Signed-off-by: Melissa Kilby <[email protected]>
incertum
force-pushed
the
re-order-proc-fields
branch
from
0dd7942 to
bd874cc
Compare
|
LGTM label has been added. Git tree hash: a8c3be1b1749812e6633421ab6e0acfcdd1e8f54
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: FedeDP, incertum, LucaGuerra 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?
What this PR does / why we need it:
Follow up #938, re-order proc filterchecks definitions to provide a nicer end user experience when reading the supported fields on the website and also improve clarity for developers via grouping related fields together. In addition, adjusted wording slightly for consistency and easier understanding.
While these changes are minor, they require careful review. Therefore, also extended the unit test for spawn_process.
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?: