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
Contributor

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

Choose a reason for hiding this comment

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

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
Contributor

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
Contributor

@rafaeldtinoco rafaeldtinoco left a comment

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

Good thinking, I've updated accordingly.

@rafaeldtinoco rafaeldtinoco self-requested a review June 24, 2022 16:17
Copy link
Contributor

@rafaeldtinoco rafaeldtinoco left a comment

Choose a reason for hiding this comment

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

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
@grantseltzer
Copy link
Contributor Author

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