Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ebpf: create function prototypes in all headers #3133

Merged
merged 2 commits into from
May 25, 2023
Merged

ebpf: create function prototypes in all headers #3133

merged 2 commits into from
May 25, 2023

Conversation

rafaeldtinoco
Copy link
Contributor

@rafaeldtinoco rafaeldtinoco commented May 22, 2023

commit f2feed0
Author: Rafael David Tinoco rafaeldtinoco@gmail.com
Date: Mon May 22 14:09:18 2023

ebpf: start using eBPF own macros

Use already provided macros for accessing types and fields:

1. allows better type casting instead of relying on (void *).
2. allows multiple fields accessible in a single step.

Note: This change was made possible after non CO-RE code was dropped.

Fixes: #898

commit ec16789
Author: Rafael David Tinoco rafaeldtinoco@gmail.com
Date: Mon May 22 06:37:03 2023

ebpf: create function prototypes in all headers

Fixes: #2576

Copy link
Contributor

@AlonZivony AlonZivony left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great :)

@AlonZivony
Copy link
Contributor

AlonZivony commented May 22, 2023

@rafaeldtinoco, don't you think we should move the functions documentation to the header file?

@@ -7,6 +7,12 @@

#include <maps.h>

// PROTOTYPES

#define sai static __always_inline
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

personally I'm not in favor of adding such a macro which I think makes the code weird and less readable

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AlonZivony what's your take on this ? I'm ok with both and I can revert to what @yanivagman is saying.

My point: Just tried to reduce the line width for readability. I can use "static __always_inline" in all places again, but then the arguments for the prototypes get very far on the right on the editor's screen.

I usually tend to think an 80 cols approach is optimal, and 100 cols is “acceptable”. Pushing farther than 90, or 100 cols, a line, is way too much IMO.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am for the change, as we declare all functions like this and I don't really think its important to see it all.
It does make the code less readable for people outside of the project. I don't think it is that hard though.

I do think that sai is not so clear, as the initial letters makes sense only after reading the macro.
Maybe change it to something like def or func, to make it clear that its the standard way to declare functions in our eBPF code?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, makes sense for me. I'll try to rename it and we can all agree on the next change (or not, lets see).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

statfunc it is then. Looks much better indeed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yanivagman let me know if you're +1 with the rename, please.

@rafaeldtinoco
Copy link
Contributor Author

@rafaeldtinoco, don't you think we should move the functions documentation to the header file?

Hum, yep, possibly. Maybe "the next change" ? I think it might be good.

@rafaeldtinoco
Copy link
Contributor Author

Integration tests are intermittent. Not only in this PR, I keep failing, from time to time:

image

Its likely related to timeouts when pulling alpine latest image.

Use already provided macros for accessing types and fields:

1. allows better type casting instead of relying on (void *).
2. allows multiple fields accessible in a single step.

Note: This change was made possible after non CO-RE code was dropped.

Fixes: #898
@geyslan
Copy link
Member

geyslan commented May 22, 2023

Integration tests are intermittent. Not only in this PR, I keep failing, from time to time:

Its likely related to timeouts when pulling alpine latest image.

This is already being fixed via #2688, by using timeout per command.

image

@rafaeldtinoco
Copy link
Contributor Author

rafaeldtinoco commented May 22, 2023

Alright, tests have passed, and E2E tests have also passed. I believe this is good. Waiting for another check.

@rafaeldtinoco
Copy link
Contributor Author

I've tested this change with E2E tests as well. Thanks Jose!

@rafaeldtinoco rafaeldtinoco merged commit 99560b7 into aquasecurity:main May 25, 2023
@rafaeldtinoco rafaeldtinoco deleted the create-func-prototypes branch May 25, 2023 22:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants