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

Add helpers for passing flags #154

Merged

Conversation

grantseltzer
Copy link
Member

@grantseltzer grantseltzer commented Apr 21, 2022

The bpf helper bpf_map_update_elem takes a flags parameter which we're not letting users specify. Instead of breaking the API, i've added another helper for it. Additionally I've added API for the bpf_map_lookup_elem_flags helper.

Signed-off-by: grantseltzer grantseltzer@gmail.com

kakkoyun
kakkoyun previously approved these changes Apr 27, 2022
Copy link
Contributor

@kakkoyun kakkoyun left a comment

LGTM 💯

libbpfgo.go Outdated
@@ -841,6 +861,14 @@ func (b *BPFMap) Update(key, value unsafe.Pointer) error {
return nil
}

func (b *BPFMap) UpdateFlags(key, value unsafe.Pointer, flags MapFlag) error {
Copy link
Collaborator

@rafaeldtinoco rafaeldtinoco Jun 24, 2022

Choose a reason for hiding this comment

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

The function name makes it look like it is updating the "flags" for a map, while actually we're just updating the map using flags argument. Should we change the method name ?

I think I'm ok with not breaking the API to have flags added, at least until we decide to break the API and unify those 2 (should we add a TODO at the code ?). Also, we could make Update() be a call to UpdateFlags() (with a new name) with a nil flag argument (something like libbpf does when deprecating functions).

Copy link
Collaborator

@rafaeldtinoco rafaeldtinoco left a comment

Code is good, I would re-think the function name, add a TODO and call one function from the other (unless you have better ideas, all ears).

Signed-off-by: grantseltzer <grantseltzer@gmail.com>
Signed-off-by: grantseltzer <grantseltzer@gmail.com>
@grantseltzer grantseltzer force-pushed the update-lookup-flags-helpers branch from 5737fd9 to 42e5405 Compare Jun 24, 2022
@grantseltzer
Copy link
Member Author

grantseltzer commented Jun 24, 2022

Good thinking, I've updated accordingly.

@rafaeldtinoco rafaeldtinoco self-requested a review Jun 24, 2022
Copy link
Collaborator

@rafaeldtinoco rafaeldtinoco left a comment

LGTM. Make sure to "squashing" instead of "merging" (so we have a single commit). Thanks for addressing the suggestions.

@grantseltzer grantseltzer merged commit 0046dd6 into aquasecurity:main Jun 24, 2022
1 check passed
@grantseltzer
Copy link
Member Author

grantseltzer commented Jun 24, 2022

I hate merging so much, that was an accident. It'l never happen again, I promise you!

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.

None yet

3 participants