[part 1/n] Introduce vtable-based dispatch #213
Conversation
gnosek
force-pushed
the
vtable
branch
2 times, most recently
from
1027917 to
c80689c
Compare
|
All in all, it looks quite good to me as an intermediate step! |
|
This is a bigger discussion I suppose, do you want to see the end result (or at least some future steps) before merging part 1 or do you trust me that we won't end up in worse shape than we already are? ;) |
|
This is looking good! |
I trust you 💙 |
Me too 💟 |
userspace/libscap/scap.h
Outdated
| // eventually, it will contain a pointer to a struct containing | ||
| // only the engine-specific bits | ||
| struct scap_engine_handle { | ||
| struct scap* m_handle; | ||
| }; |
There was a problem hiding this comment.
From the comment, it seems like this struct, that now contains a pointer to the global scap handler, will contain a single pointer to the engine-specific context. My question is, what's the advantage of having a structure with a single member instead of a single opaque pointer?
There was a problem hiding this comment.
IIRC it made some type safety easier (fewer casts between void* and a concrete T*). But I agree it's not too pretty so I'll try and see if just using a pointer works.
(note: in the public header, we don't really know what the type of the opaque handle is--that's the point of being an opaque handle :D)
|
This will make maintaining and extending libscap so much easier! 🚀 |
|
I believe this can go in as I find this to be a much better way of handling the system call providers we have. The only question I had was about the handle, but if you feel that doing it like proposed makes casting and type safety easier it's absolutely fine :) |
gnosek
changed the title
|
Wow! This is huge! Thank you @gnosek for all this work, I can't wait to see it merged 😍 🚀 |
This way we can have multiple implementations of platform-specific functions instead of `#ifdef`s all over the place Signed-off-by: Grzegorz Nosek <[email protected]>
Signed-off-by: Grzegorz Nosek <[email protected]>
Signed-off-by: Grzegorz Nosek <[email protected]>
Signed-off-by: Grzegorz Nosek <[email protected]>
Signed-off-by: Grzegorz Nosek <[email protected]>
Signed-off-by: Grzegorz Nosek <[email protected]>
Signed-off-by: Grzegorz Nosek <[email protected]>
Signed-off-by: Grzegorz Nosek <[email protected]>
Almost everything can be overridden, which we'll need once we switch eBPF to use it too. Signed-off-by: Grzegorz Nosek <[email protected]>
Signed-off-by: Grzegorz Nosek <[email protected]>
Signed-off-by: Grzegorz Nosek <[email protected]>
Signed-off-by: Grzegorz Nosek <[email protected]>
Signed-off-by: Grzegorz Nosek <[email protected]>
|
LGTM label has been added. Git tree hash: f9d603cd78d90942d5d21de20b45e2e46db008eb
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Andreagit97, FedeDP, gnosek 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 |
|
/retest |
|
/unhold |
|
/test build-libs |
What type of PR is this?
/kind cleanup
/kind design
Any specific area of the project related to this PR?
/area libscap
What this PR does / why we need it:
This is the first in a series of PRs that aim to encapsulate all engine-specific behavior of libscap behind a series of vtables, instead of
ifs and#ifdefs.The motivation is:
Special notes for your reviewer:
This PR (temporarily) makes things much uglier and is just the first one in a series. With this PR, the most important scap functions are routed through a vtable, if present. This PR creates separate implementations for the nodriver, source_plugin and udig engines. eBPF, kmod and savefile are still TODO, as are additional vtables, which are used mostly at initialization time.
Does this PR introduce a user-facing change?: