refactor(userspace): re-implement plugin loader in C and split it in its own package #392

jasondellaluce · 2022-06-13T15:35:34Z

What type of PR is this?

/kind cleanup

/kind design

Any specific area of the project related to this PR?

/area libscap-engine-source-plugin

/area libsinsp

What this PR does / why we need it:

Which issue(s) this PR fixes:

This PR refactors the plugin definitions and loading logic. The plugin types, definitions, and loading logic are now all under an ad-hoc userspace/plugin directory on which both libscap and libsinsp dependencies. The goals and benefits are:

Better separate responsibilities: So far the plugin definitions were scattered across libsinsp and libscap, and all their definitions were placed inside the libscap's `plugin_info.h"
Logic is all in the same place: The plugin types and the plugin loader are now resident in a single code location, which is going to become the only reference for the official plugin system definitions. The class-based libsinsp code responsible of managing plugins now wraps around the new C definitions of userspace/plugin
Split plugin-related code: The old plugin_info.h file has been split into plugin_types.h and plugin_api.h, so that consumers can only import the definitions they need
Reusability in external projects: The new design allows the official plugin definitions and loader to be re-used in other external projects. An example in which this could be required is the Plugin SDK Go, that can potentially implement a plugin loader written in Go based on top of the official definition of libs. Automated tools, such as the upcoming plugin distribution system in falcoctl, could be built on top of this code.
Bug fixes in plugin loader: The old plugin loader logic that was bundled in libsinsp contained some flaws. For instance, it considered symbols such as plugin_init or plugin_destroy as optional. The new plugin loader also prompts more explicit error messages, such as when the semver checks fail.

Special notes for your reviewer:

~400 LOC are caused by moving plugin_info.h and splitting it into plugin/plugin_types.h and plugin/plugin_api.h.

Does this PR introduce a user-facing change?:

NONE

jasondellaluce · 2022-06-13T15:58:13Z

falcosecurity/plugins#121 is a good example where the plugin loader code might be needed in an external project and must not be replicated to guarantee consistency.

Molter73

I'm not super familiar with the plugins system, but the change looks cool! I left a few comments on the C code, they are mostly nitpicks, so don't worry too much about them.

userspace/plugin/plugin_loader.c

FedeDP · 2022-06-14T08:15:46Z

userspace/plugin/plugin_loader.c

+    if(a->api.s == NULL) \
+    { \
+        strcpy(e, "symbol not implemented: "); \
+        strncat(e, #s, PLUGIN_MAX_ERRLEN - 1); \


Here we are not considering "symbol not implemented: " length when accounting for max size.
I don't think this will ever be an issue, still it would be great to deal with it given that we are using strncat :)

Just changed this thanks to the suggestions of @Molter73!

FedeDP

This looks so cool!
Thank you!
Did you consider adding a library around it?

jasondellaluce · 2022-06-14T08:38:04Z

This looks so cool! Thank you! Did you consider adding a library around it?

Thanks Fede! You mean building a .a lib out of this? This can be an idea! I didn't want to impact the CMake setup in this first PR (also to avoid interfering with the huge ongoing libscap refactoring), but I think this can be a good idea.

jasondellaluce · 2022-06-14T08:46:31Z

/retest

userspace/plugin/plugin_loader.c

jasondellaluce · 2022-06-14T09:34:16Z

Thanks @Molter73 for the deep review! I appreciate it 🙏🏼

leogr

This is cool.

👍 for me :)

poiana · 2022-06-17T16:39:13Z

LGTM label has been added.

Git tree hash: ed493b873af12985ceeeb97ee45b6bdf4e2d5980

…ide of sinsp and scap Signed-off-by: Jason Dellaluce <jasondellaluce@gmail.com>

…l directory Signed-off-by: Jason Dellaluce <jasondellaluce@gmail.com>

Signed-off-by: Jason Dellaluce <jasondellaluce@gmail.com>

Signed-off-by: Jason Dellaluce <jasondellaluce@gmail.com> Co-authored-by: Mauro Ezequiel Moltrasio <moltrasiom@hotmail.com>

Signed-off-by: Jason Dellaluce <jasondellaluce@gmail.com>

jasondellaluce · 2022-07-14T11:20:55Z

/retest

FedeDP

/approve

poiana · 2022-07-14T12:06:10Z

LGTM label has been added.

Git tree hash: 5ea9263eaee5d65b445c072761767e003bea50fc

poiana · 2022-07-14T12:06:10Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: FedeDP, jasondellaluce

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]

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

leogr

Again 👍

poiana added release-note-none kind/cleanup dco-signoff: yes kind/design area/libscap-engine-source-plugin area/libsinsp labels Jun 13, 2022

poiana requested review from Andreagit97 and ldegio June 13, 2022 15:35

poiana added the size/XXL label Jun 13, 2022

Molter73 reviewed Jun 13, 2022

View reviewed changes

userspace/plugin/plugin_loader.c Outdated Show resolved Hide resolved

userspace/plugin/plugin_loader.c Outdated Show resolved Hide resolved

userspace/plugin/plugin_loader.c Outdated Show resolved Hide resolved

userspace/plugin/plugin_loader.c Outdated Show resolved Hide resolved

FedeDP reviewed Jun 14, 2022

View reviewed changes

Molter73 reviewed Jun 14, 2022

View reviewed changes

userspace/plugin/plugin_loader.c Outdated Show resolved Hide resolved

jasondellaluce mentioned this pull request Jun 14, 2022

new: introduce plugin loader code falcosecurity/plugin-sdk-go#56

Merged

leogr previously approved these changes Jun 17, 2022

View reviewed changes

poiana assigned leogr Jun 17, 2022

poiana added the lgtm label Jun 17, 2022

jasondellaluce dismissed leogr’s stale review via 6a429da July 13, 2022 09:06

jasondellaluce force-pushed the refactor/plugin-defs-loader branch from 89dcd02 to 6a429da Compare July 13, 2022 09:06

poiana removed the lgtm label Jul 13, 2022

poiana requested a review from leogr July 13, 2022 09:06

jasondellaluce added 4 commits July 13, 2022 10:36

refactor(userspace): move plugin definitions in their own folder outs…

d8ac90a

…ide of sinsp and scap Signed-off-by: Jason Dellaluce <jasondellaluce@gmail.com>

refactor(userspace/libscap): adapt scap plugin definitions to externa…

bac27aa

…l directory Signed-off-by: Jason Dellaluce <jasondellaluce@gmail.com>

new(userspace/plugin): port C version of the plugin loader

8bcbb83

Signed-off-by: Jason Dellaluce <jasondellaluce@gmail.com>

refactor(userspace/plugin): rename source files

172a6f0

Signed-off-by: Jason Dellaluce <jasondellaluce@gmail.com>

jasondellaluce and others added 6 commits July 13, 2022 10:36

refactor(userspace/libsinsp): use new plugin loader in libsinsp

2ae2d54

Signed-off-by: Jason Dellaluce <jasondellaluce@gmail.com>

fix(userspace/libsinsp): fix compilation issues

edff05b

Signed-off-by: Jason Dellaluce <jasondellaluce@gmail.com>

update(userspace/plugin): make SYM_REQCHECK macro safer

6caf074

Signed-off-by: Jason Dellaluce <jasondellaluce@gmail.com> Co-authored-by: Mauro Ezequiel Moltrasio <moltrasiom@hotmail.com>

update(userspace/plugin): improve code stability and readability

db7f6ab

Signed-off-by: Jason Dellaluce <jasondellaluce@gmail.com> Co-authored-by: Mauro Ezequiel Moltrasio <moltrasiom@hotmail.com>

fix(userspace/plugin): use right size limit in snprintf

4f42fc3

Signed-off-by: Jason Dellaluce <jasondellaluce@gmail.com> Co-authored-by: Mauro Ezequiel Moltrasio <moltrasiom@hotmail.com>

fix(userspace/plugin): set get_last_err as required symbol

cfbc993

Signed-off-by: Jason Dellaluce <jasondellaluce@gmail.com>

jasondellaluce force-pushed the refactor/plugin-defs-loader branch from 6a429da to 47bf2b8 Compare July 13, 2022 10:36

chore(userspace/plugin): fix compilation issues

08b3ae0

Signed-off-by: Jason Dellaluce <jasondellaluce@gmail.com>

jasondellaluce force-pushed the refactor/plugin-defs-loader branch from 47bf2b8 to 08b3ae0 Compare July 13, 2022 10:45

FedeDP approved these changes Jul 14, 2022

View reviewed changes

poiana assigned FedeDP Jul 14, 2022

poiana added the lgtm label Jul 14, 2022

poiana added the approved label Jul 14, 2022

leogr approved these changes Jul 14, 2022

View reviewed changes

poiana merged commit 5b9c3ca into falcosecurity:master Jul 14, 2022

jasondellaluce mentioned this pull request Jul 14, 2022

update: bump libs version to 5b9c3ca9ae55800e774e1681a0c5e7d54ca89263 falcosecurity/plugin-sdk-go#63

Merged

jasondellaluce mentioned this pull request Aug 3, 2022

update: bump libs version to b4c198773bf05486e122f6d3f7f63be125242413 falcosecurity/falco#2152

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(userspace): re-implement plugin loader in C and split it in its own package #392

refactor(userspace): re-implement plugin loader in C and split it in its own package #392

jasondellaluce commented Jun 13, 2022 •

edited

Loading

jasondellaluce commented Jun 13, 2022

Molter73 left a comment

FedeDP Jun 14, 2022

jasondellaluce Jun 14, 2022

FedeDP left a comment

jasondellaluce commented Jun 14, 2022

jasondellaluce commented Jun 14, 2022

jasondellaluce commented Jun 14, 2022

leogr left a comment

poiana commented Jun 17, 2022

jasondellaluce commented Jul 14, 2022

FedeDP left a comment

poiana commented Jul 14, 2022

poiana commented Jul 14, 2022

leogr left a comment

refactor(userspace): re-implement plugin loader in C and split it in its own package #392

refactor(userspace): re-implement plugin loader in C and split it in its own package #392

Conversation

jasondellaluce commented Jun 13, 2022 • edited Loading

jasondellaluce commented Jun 13, 2022

Molter73 left a comment

Choose a reason for hiding this comment

FedeDP Jun 14, 2022

Choose a reason for hiding this comment

jasondellaluce Jun 14, 2022

Choose a reason for hiding this comment

FedeDP left a comment

Choose a reason for hiding this comment

jasondellaluce commented Jun 14, 2022

jasondellaluce commented Jun 14, 2022

jasondellaluce commented Jun 14, 2022

leogr left a comment

Choose a reason for hiding this comment

poiana commented Jun 17, 2022

jasondellaluce commented Jul 14, 2022

FedeDP left a comment

Choose a reason for hiding this comment

poiana commented Jul 14, 2022

poiana commented Jul 14, 2022

leogr left a comment

Choose a reason for hiding this comment

jasondellaluce commented Jun 13, 2022 •

edited

Loading