-
Notifications
You must be signed in to change notification settings - Fork 169
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
feat(driver): Add loongarch64 kernel module support #1803
Conversation
Signed-off-by: yzewei <[email protected]>
Please double check driver/SCHEMA_VERSION file. See versioning. /hold |
@FedeDP I'm not sure why the last PR was closed. |
@@ -99,14 +99,15 @@ void do___open_by_handle_atX_success(int *open_by_handle_fd, int *dirfd, char *f | |||
} | |||
} | |||
|
|||
#ifndef __loongarch64 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add a small comment around these compilation guards? Eg: "fstat
not available on loongarch64" is enough :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Of course, this is what I should do, and I will add them one by one in other sections.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps it would be even better to just check for ifdef __NR_fstat
?
Sorry, i came up with that right now 😆
Since it is self explaining, then, we can avoid adding the comment!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good idea!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This LGTM! I'd also update the [Supported arch matrix] in the readme: https://github.com/falcosecurity/libs?tab=readme-ov-file#drivers-officially-supported-architectures together with the ARCHS
label (at the beginning of the readme)
/milestone next-driver |
Signed-off-by: yzewei <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/approve
LGTM label has been added. Git tree hash: 38ee984291b5621cf9673e58a404cad5c32031dd
|
We can update the README later :) |
Good Job! Thanks a bunch! |
/cc @Andreagit97 |
@@ -47,8 +47,8 @@ else() | |||
ExternalProject_Add(protobuf | |||
PREFIX "${PROJECT_BINARY_DIR}/protobuf-prefix" | |||
DEPENDS zlib | |||
URL "https://github.com/protocolbuffers/protobuf/releases/download/v3.17.3/protobuf-cpp-3.17.3.tar.gz" | |||
URL_HASH "SHA256=51cec99f108b83422b7af1170afd7aeb2dd77d2bcbb7b6bad1f92509e9ccf8cb" | |||
URL "https://github.com/protocolbuffers/protobuf/releases/download/v3.20.3/protobuf-cpp-3.20.3.tar.gz" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps i already asked you; is this bump mandatory to build on loongarch64?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, the main reason for the replacement is
protobuf 3.17.3 version cannot be compiled with loongarch, because config.guess and config.sub lack loongarch definition, so it needs to be updated to a higher version
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Co-authored-by: Andrea Terzolo <[email protected]> Signed-off-by: yzewei <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/approve
LGTM label has been added. Git tree hash: 6c91bc4a01d8c9becbd9191239d6516e7fafd7a4
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Andreagit97, FedeDP, yzewei 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 |
/milestone 7.1.0+driver |
What type of PR is this?
/kind feature
Any specific area of the project related to this PR?
/area build
/area driver-kmod
Does this PR require a change in the driver versions?
/version driver-API-version-minor
What this PR does / why we need it:
Add loongarch64 kernel module support.
Temporarily does not involve bpf.
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
I'm not sure what problems are there at present. If there is a problem, I hope to point out.
Does this PR introduce a user-facing change?: