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

Simplify GetMapsIDsByName #384

Merged
merged 8 commits into from
Oct 23, 2023
Merged

Conversation

aymericDD
Copy link
Contributor

The id is incremented by C.bpf_map_get_next_id so no need to define the startId and nextId. I encounter some weird behavior in my project when I was using map with a BPF_MAP_TYPE_HASH type.

@aymericDD aymericDD marked this pull request as ready for review October 10, 2023 14:24
@geyslan
Copy link
Member

geyslan commented Oct 10, 2023

Hello @aymericDD. Could you please explain this weird behaviour so we can provide more context for this change.

@geyslan geyslan self-assigned this Oct 10, 2023
@aymericDD
Copy link
Contributor Author

When I was using a BPF_MAP_TYPE_HASH with a struct key and struct value, the map was not returned in the list of IDs. I take a look at the bpftool and I notice the ID was not re-incremented manually.

@aymericDD
Copy link
Contributor Author

aymericDD commented Oct 11, 2023

I added a unit test to cover this case. I don't have a local dev environment because I have a Macbook pro M1 and vagrant with virtualbox is not supported for now with the ARM architecture.

map-low.go Outdated Show resolved Hide resolved
It retrieves the next available map ID after the given startID.
map-low.go Outdated Show resolved Hide resolved
map-low.go Outdated Show resolved Hide resolved
map-low.go Outdated Show resolved Hide resolved
Remove custom error to reduce maintenance toil. Just return the syscall error is enough and does not ask to create custom error for each kind of syscall errors.
map-low.go Show resolved Hide resolved
The startId was not a pointer, so its value was never updated for the caller of the GetMapsIDsByName function.
@geyslan
Copy link
Member

geyslan commented Oct 23, 2023

@aymericDD is it waiting review?

@aymericDD
Copy link
Contributor Author

Yes 👍

Copy link
Member

@geyslan geyslan left a comment

Choose a reason for hiding this comment

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

LGTM

@geyslan geyslan merged commit 6f549ef into aquasecurity:main Oct 23, 2023
14 checks passed
@geyslan
Copy link
Member

geyslan commented Oct 23, 2023

@aymericDD thanks again for your contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants