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

[postponed] bpf issue 515 (fix: param 1 (cmd) is an int not a int64_t) #1381

Closed
wants to merge 1 commit into from
Closed

Conversation

ecbadeaux
Copy link
Contributor

@ecbadeaux ecbadeaux commented Oct 3, 2023

What type of PR is this?

/kind documentation

Any specific area of the project related to this PR?

/area driver-bpf

/area driver-modern-bpf

/area tests

Does this PR require a change in the driver versions?

I am not sure.

What this PR does / why we need it:

Fixes bpf param 1 from int64_t to int.

Which issue(s) this PR fixes:

Params inconsistencies in our drivers #515 bpf

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

@poiana
Copy link
Contributor

poiana commented Oct 3, 2023

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: ecbadeaux
Once this PR has been reviewed and has the lgtm label, please assign lucaguerra for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

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

@poiana
Copy link
Contributor

poiana commented Oct 3, 2023

Welcome @ecbadeaux! It looks like this is your first PR to falcosecurity/libs 🎉

@poiana poiana added the size/S label Oct 3, 2023
@github-actions
Copy link

github-actions bot commented Oct 3, 2023

Please double check driver/API_VERSION file. See versioning.

/hold

@dwindsor
Copy link
Contributor

dwindsor commented Oct 3, 2023

Welcome @ecbadeaux! Thanks for this PR 🚀. The plan is to knock out issues in #515 as time allows.

Tagging @incertum @FedeDP @Andreagit97 for visibility.

@ecbadeaux ecbadeaux changed the title Resolve bpf issue 515 bpf issue 515 (fix: param 1 (cmd) is an int not a int64_t) Oct 3, 2023
Signed-off-by: Everett Badeaux <[email protected]>
@incertum
Copy link
Contributor

incertum commented Oct 4, 2023

@leogr @Andreagit97 and @FedeDP here we are again. We need to decide on a path forward to fix remaining issues in 515 for once. While we fixed few things without needing a new event type for example this change would need a new event.

Is there no way we can introduce versioning of scap files and now would mark breaking backwards compatibility?

@Andreagit97
Copy link
Member

ei @ecbadeaux thank you for this and this #1382! Unfortunately, as @incertum has correctly highlighted these patches are more complex than they seem... without making it too long, we cannot change the schema of current events (remove/add/change size of params) otherwise we would no longer able to read old scap-file (libs is able to read all old events if dumped to a file).

The only thing we can do is to add new events with a new format... at least right now... Probably is my fault because in issue #515 I've not highlighted this downside, I will update it ASAP splitting issues that don't require a new event from ones that can be easily addressable. For what concerns the ones that require a new event, as @incertum correctly said, we would like to address them all in one shot when the scap-file management is solved. Sorry again for the inconvenience!

@Andreagit97
Copy link
Member

I will update it ASAP splitting issues that don't require a new event from ones that can be easily addressable.

Done -> #515 (comment)

@incertum incertum changed the title bpf issue 515 (fix: param 1 (cmd) is an int not a int64_t) [postponed] bpf issue 515 (fix: param 1 (cmd) is an int not a int64_t) Oct 4, 2023
@incertum incertum added this to the TBD milestone Oct 4, 2023
@ecbadeaux ecbadeaux closed this by deleting the head repository Nov 1, 2023
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