cleanup(scap,sinsp): Clean up proc callback handling code #1471

gnosek · 2023-11-04T23:29:12Z

What type of PR is this?

Uncomment one (or more) /kind <> lines:

/kind bug

/kind cleanup

/kind design

/kind documentation

/kind failing-test

/kind feature

Any specific area of the project related to this PR?

Uncomment one (or more) /area <> lines:

/area API-version

/area build

/area CI

/area driver-kmod

/area driver-bpf

/area driver-modern-bpf

/area libscap-engine-bpf

/area libscap-engine-gvisor

/area libscap-engine-kmod

/area libscap-engine-modern-bpf

/area libscap-engine-nodriver

/area libscap-engine-noop

/area libscap-engine-source-plugin

/area libscap-engine-savefile

/area libscap-engine-udig

/area libscap

/area libpman

/area libsinsp

/area tests

/area proposals

Does this PR require a change in the driver versions?

/version driver-API-version-major

/version driver-API-version-minor

/version driver-API-version-patch

/version driver-SCHEMA-version-major

/version driver-SCHEMA-version-minor

/version driver-SCHEMA-version-patch

What this PR does / why we need it:

This PR aims to simplify the infrastructure around the proc callback (a function called from scap for every new thread and fd discovered during the /proc scan). The end result is:

+274/-495: a bit of code removed, mostly in the dark corners of scap
reduced allocations during /proc scan (we freed most of them immediately)
fewer special cases all over the place

Summary of changes:

every place that used to check for m_proc_callback now calls it unconditionally
internal hash table manipulation (the m_proc_callback==NULL case) is now simply a different (default) callback
scap_proc_read_thread is no longer a special case with the procinfo param, just uses its own temporary proclist
scap_get_proc_table is gone (no longer used by sinsp)
all scap_tinfo/scap_fdinfo heap allocations now happen in the proc callback, only when really needed

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

I'd call the diff surprisingly readable for a PR that touches half the world. There were fairly few incidental changes (except for a massive tinfo->foo -> tinfo.foo in scap_proc_add_from_proc).

Note: I may have broken something around filtering captures (the code is... interesting, with sinsp storing bits in scap_threadinfos) so I'll try to come up with some tests for it. For now, I'm just letting CI have its fun with the PR.

Does this PR introduce a user-facing change?:

BREAKING CHANGE: scap_get_proc_table is gone

gnosek · 2023-11-04T23:38:10Z

Yeah, we're completely bypassing capture filtering now. Will fix.

gnosek · 2023-11-05T07:04:12Z

I dropped the more aggressive changes for now, if the tests pass, I'll probably submit the rest as a separate PR.

Signed-off-by: Grzegorz Nosek <grzegorz.nosek@sysdig.com>

This is effectively a special-case single-entry proclist Signed-off-by: Grzegorz Nosek <grzegorz.nosek@sysdig.com>

Signed-off-by: Grzegorz Nosek <grzegorz.nosek@sysdig.com>

It was always unclear (to me) what the proclist is used for, exactly. It turns out it's just a holder for the callback which was ignored in the previous implementation and now is replaced by a completely different temporary proclist. Signed-off-by: Grzegorz Nosek <grzegorz.nosek@sysdig.com>

Signed-off-by: Grzegorz Nosek <grzegorz.nosek@sysdig.com>

We don't need a heap-allocated tinfo just to pass it to sinsp_threadinfo::init Signed-off-by: Grzegorz Nosek <grzegorz.nosek@sysdig.com>

Signed-off-by: Grzegorz Nosek <grzegorz.nosek@sysdig.com>

gnosek · 2023-11-10T10:22:16Z

I know i am getting annoying...

Nah, this sort of annoying I fully approve :D

FedeDP

/approve

poiana · 2023-11-10T10:30:30Z

LGTM label has been added.

Git tree hash: 73b4175fabdaa0defb245905a4a245a0c05d5109

incertum

/approve

poiana · 2023-11-10T18:23:54Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: FedeDP, gnosek, incertum

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:

~~OWNERS~~ [FedeDP,gnosek,incertum]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

poiana added release-note-none kind/cleanup dco-signoff: yes area/libscap area/libsinsp size/XXL approved labels Nov 4, 2023

poiana requested review from hbrueckner and jasondellaluce November 4, 2023 23:29

gnosek changed the title ~~cleanup(scap,sinsp): Clean up proc callback handling code~~ wip: cleanup(scap,sinsp): Clean up proc callback handling code Nov 4, 2023

poiana added the do-not-merge/work-in-progress label Nov 4, 2023

poiana added dco-signoff: no and removed dco-signoff: yes labels Nov 5, 2023

gnosek force-pushed the proc-callback-cleanup branch from 15df9bf to 6ddf820 Compare November 5, 2023 07:03

poiana added dco-signoff: yes and removed dco-signoff: no labels Nov 5, 2023

gnosek changed the title ~~wip: cleanup(scap,sinsp): Clean up proc callback handling code~~ cleanup(scap,sinsp): Clean up proc callback handling code Nov 5, 2023

poiana removed the do-not-merge/work-in-progress label Nov 5, 2023

Andreagit97 added this to the 0.14.0 milestone Nov 6, 2023

gnosek mentioned this pull request Nov 6, 2023

cleanup(sinsp,scap): switch to sinsp thread table for savefiles #1472

Merged

gnosek force-pushed the proc-callback-cleanup branch 2 times, most recently from 55a1823 to 27d66f7 Compare November 6, 2023 14:08

gnosek mentioned this pull request Nov 7, 2023

wip: DO NOT MERGE: Proc callback cleanup bisect #1474

Closed

gnosek force-pushed the proc-callback-cleanup branch 3 times, most recently from de0d670 to efcd9fa Compare November 7, 2023 17:17

poiana added release-note and removed release-note-none labels Nov 8, 2023

gnosek added 18 commits November 10, 2023 11:21

cleanup(scap): remove fdinfo alloc/free functions

29b5367

Signed-off-by: Grzegorz Nosek <grzegorz.nosek@sysdig.com>

cleanup(scap): extend proc_callback type for future expansion

be1dd84

Signed-off-by: Grzegorz Nosek <grzegorz.nosek@sysdig.com>

cleanup(scap): introduce default proc callback

2aab28a

Signed-off-by: Grzegorz Nosek <grzegorz.nosek@sysdig.com>

cleanup(scap): introduce helper for initializing the proclist

56c8f74

Signed-off-by: Grzegorz Nosek <grzegorz.nosek@sysdig.com>

cleanup(scap): use default proc callback instead of NULL checks

06e6c3c

Signed-off-by: Grzegorz Nosek <grzegorz.nosek@sysdig.com>

cleanup(scap): inline scap_add_fd_to_proc_table

1147ab9

Signed-off-by: Grzegorz Nosek <grzegorz.nosek@sysdig.com>

cleanup(scap): remove some unused includes

2b05f1f

Signed-off-by: Grzegorz Nosek <grzegorz.nosek@sysdig.com>

cleanup(scap): flatten scap_proc_scan_vtable a bit

b37a50a

Signed-off-by: Grzegorz Nosek <grzegorz.nosek@sysdig.com>

cleanup(scap): remove procinfo from scap_proc_add_from_proc

84c5ddb

This is effectively a special-case single-entry proclist Signed-off-by: Grzegorz Nosek <grzegorz.nosek@sysdig.com>

cleanup(scap): remove proclist from scap_proc_read_thread

17c3be9

Signed-off-by: Grzegorz Nosek <grzegorz.nosek@sysdig.com>

cleanup(scap): remove scap_t from scap_proc_free

7aa434c

Signed-off-by: Grzegorz Nosek <grzegorz.nosek@sysdig.com>

cleanup(scap): remove scap_t from scap_fd_add

a0dfaad

Signed-off-by: Grzegorz Nosek <grzegorz.nosek@sysdig.com>

cleanup(scap): remove scap_proc_alloc

f38a596

Signed-off-by: Grzegorz Nosek <grzegorz.nosek@sysdig.com>

cleanup(scap,sinp): do not allocate scap_threadinfo for scap_proc_get

b5db5a0

We don't need a heap-allocated tinfo just to pass it to sinsp_threadinfo::init Signed-off-by: Grzegorz Nosek <grzegorz.nosek@sysdig.com>

cleanup(scap): remove scap_proc_free

db81cce

Signed-off-by: Grzegorz Nosek <grzegorz.nosek@sysdig.com>

cleanup(scap): remove unused parameter from scap_fd_add

14ef2c4

Signed-off-by: Grzegorz Nosek <grzegorz.nosek@sysdig.com>

cleanup(scap): move scap_alloc_proclist_info to scap_procs.c

d440f43

Signed-off-by: Grzegorz Nosek <grzegorz.nosek@sysdig.com>

gnosek force-pushed the proc-callback-cleanup branch from efcd9fa to d440f43 Compare November 10, 2023 10:21

FedeDP approved these changes Nov 10, 2023

View reviewed changes

poiana assigned FedeDP Nov 10, 2023

poiana added the lgtm label Nov 10, 2023

incertum approved these changes Nov 10, 2023

View reviewed changes

poiana assigned incertum Nov 10, 2023

poiana merged commit 3284fa8 into falcosecurity:master Nov 10, 2023
26 checks passed

gnosek deleted the proc-callback-cleanup branch November 10, 2023 19:48

LucaGuerra mentioned this pull request Mar 5, 2024

fix(engine/savefile): fix crash when reading proclist from scap #1726

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cleanup(scap,sinsp): Clean up proc callback handling code #1471

cleanup(scap,sinsp): Clean up proc callback handling code #1471

gnosek commented Nov 4, 2023 •

edited

gnosek commented Nov 4, 2023

gnosek commented Nov 5, 2023

gnosek commented Nov 10, 2023

FedeDP left a comment

poiana commented Nov 10, 2023

incertum left a comment

poiana commented Nov 10, 2023

cleanup(scap,sinsp): Clean up proc callback handling code #1471

cleanup(scap,sinsp): Clean up proc callback handling code #1471

Conversation

gnosek commented Nov 4, 2023 • edited

gnosek commented Nov 4, 2023

gnosek commented Nov 5, 2023

gnosek commented Nov 10, 2023

FedeDP left a comment

Choose a reason for hiding this comment

poiana commented Nov 10, 2023

incertum left a comment

Choose a reason for hiding this comment

poiana commented Nov 10, 2023

gnosek commented Nov 4, 2023 •

edited