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 crud support for BPF_MAP_TYPE_BLOOM_FILTER #778

Merged
merged 1 commit into from
May 8, 2024

Conversation

barthr
Copy link
Contributor

@barthr barthr commented May 7, 2024

This PR fixes issue: #777

@danielocfb
Copy link
Collaborator

Seems fine to me, thanks! Would you be able to add a test for this, by any chance? Also, can you please squash commits into a single one?

@barthr
Copy link
Contributor Author

barthr commented May 7, 2024

Yes will do!

@barthr barthr force-pushed the master branch 2 times, most recently from 705a0e2 to 6eeeb30 Compare May 7, 2024 20:20
@barthr
Copy link
Contributor Author

barthr commented May 7, 2024

So couple of things, when I started working on the tests I noticed that there is no way to do a proper lookup using the current lookup functionality. So I've added another function which should be used when u are using a bloom filter. I made it conform the spec as per (https://docs.kernel.org/bpf/map_bloom_filter.html). I also added some small refactoring. The PR turned out a little bit bigger than expected. Let me know what you think, I've only recently (this week) started to learn rust so it's all quite new to me.

@barthr barthr changed the title Update map_key to return NULL when using BloomFilter Add crud support for BPF_MAP_TYPE_BLOOM_FILTER May 7, 2024
Copy link
Collaborator

@danielocfb danielocfb left a comment

Choose a reason for hiding this comment

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

I am unsure how exactly I feel about lookup_bloom_filter, but I suppose there is precedent, I don't see a better way, and the alternative of using lookup seems too....dodgy. So why not. Thanks!
Can you please take a look at the one suggestion I left?

libbpf-rs/src/map.rs Outdated Show resolved Hide resolved
@barthr
Copy link
Contributor Author

barthr commented May 7, 2024

I get your view. I tried looking into staying as close as possible to existing calls and existing API's however it would make the lookup code a bit ugly and violating the principle of least suprises.

So that is why I opted for the specific method for the bloom filter map. However, there might also be solutions which use a more polymorphism approach. Like a trait with a default lookup method with custom overrides for certain map types. Another approach might be to have a lookup method which can take an extra value Argument and returns an int. I think for now this is out of scope.

Unfortunately, the libbpf api is a bit funky since the same methods mean different things on different types, that is the downside of making everything a map I suppose.

I will look at your comments tomorrow.

Update map_key documentation to include bloom filter

Add bloom filter test

Add specific lookup for bloom filter

Finalize PR

Improve error message
@danielocfb danielocfb merged commit 946feb5 into libbpf:master May 8, 2024
12 checks passed
danielocfb pushed a commit that referenced this pull request May 8, 2024
Add a CHANGELOG entry for pull request #778, which addressed some issues
when working with bloom filter maps.

Signed-off-by: Daniel Müller <deso@posteo.net>
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