Add Map::lookup_batch and Map::lookup_and_delete_batch #996
Conversation
There was a problem hiding this comment.
Thanks for the pull request. Left a couple of comments. Can you please clean up the history? No fixup commits, no merges.
At the highest level, I don't know whether an Iterator is really the best abstraction here, because Rust's lack for streaming iterators means that we have to deep-copy each key/value we hand out. But it may still be the right trade-off, given the conveniences it affords.
libbpf-rs/src/map.rs
Outdated
| elem_flags: MapFlags, | ||
| flags: MapFlags, | ||
| delete: bool, | ||
| ) -> Result<BatchedMapIter<'_>> |
There was a problem hiding this comment.
Why does this function return a Result? I see no error cases.
libbpf-rs/src/map.rs
Outdated
| if self.map_type().is_bloom_filter() { | ||
| return Err(Error::with_invalid_data( | ||
| "lookup_bloom_filter() must be used for bloom filter maps", | ||
| )); | ||
| } | ||
| if self.map_type().is_percpu() { | ||
| return Err(Error::with_invalid_data(format!( | ||
| "lookup_percpu() must be used for per-cpu maps (type of the map is {:?})", | ||
| self.map_type(), | ||
| ))); | ||
| } |
There was a problem hiding this comment.
This code is now duplicated three times. Can you please share this functionality?
libbpf-rs/src/map.rs
Outdated
| } | ||
| }; | ||
|
|
||
| if ret == -14 || count == 0 { |
There was a problem hiding this comment.
-14? Symbolic constant anyone?
There was a problem hiding this comment.
Also, I think we need to set done here. My understanding is that errors should generally assumed to be persistent. The exception is ENOENT, which seems to indicate termination for some map types (in which case we still have count elements to yield) and EINTR, which I believe should just trigger a retry with the same input arguments.
There was a problem hiding this comment.
Also, can you use existing utility function for checking return value?
libbpf-rs/src/map.rs
Outdated
| }; | ||
|
|
||
| let key_size = match map.map_type() { | ||
| MapType::Hash | MapType::PercpuHash | MapType::LruHash | MapType::LruPercpuHash => { |
There was a problem hiding this comment.
Didn't all callers make sure that this is not invoked on Percpu maps? Perhaps move this accumulation of alternatives into a separate function is_hash_map or something like that, at which point it makes at least sense in isolation.
libbpf-rs/src/map.rs
Outdated
|
|
||
| let key_size = match map.map_type() { | ||
| MapType::Hash | MapType::PercpuHash | MapType::LruHash | MapType::LruPercpuHash => { | ||
| map.key_size().max(4) |
There was a problem hiding this comment.
What is 4? Please add a comment.
libbpf-rs/src/map.rs
Outdated
| self.keys.as_mut_ptr() as *mut c_void, | ||
| self.values.as_mut_ptr() as *mut c_void, | ||
| (&mut count) as *mut u32, | ||
| &self.batch_opts as *const libbpf_sys::bpf_map_batch_opts, |
There was a problem hiding this comment.
Can you please use .cast for pointer casts where possible?
| key_size: u32, | ||
| value_size: u32, |
There was a problem hiding this comment.
Nit: Perhaps it's preferable to pass in usize instead of having to cast at every usage site?
libbpf-rs/src/map.rs
Outdated
| } | ||
|
|
||
| impl Iterator for BatchedMapIter<'_> { | ||
| type Item = (Vec<Vec<u8>>, Vec<Vec<u8>>); |
There was a problem hiding this comment.
This return type seems rather convoluted to me, to be honest. For one, I don't see any reason to separate keys from values and report them differently; there is a 1:1 mapping, no? In my opinion it would be cleaner to just a single element ((key, value)) at a time and only perform the system call once the current batch is depleted.
* add Map::lookup_batch and Map::lookup_and_delete_batch * add lookup batch test to test behavior * introduce BatchedMapIter to iterate over Map in batches
|
Sorry for the messy rebase, but should be fine now I hope? I have incorporated all of your feedback, I think it looks much better now. I share your sentiment regarding the abstraction choice. If you have a better idea, let me know and I will try to implement it :) |
| self.next.as_mut_ptr().cast(), | ||
| self.keys.as_mut_ptr().cast(), | ||
| self.values.as_mut_ptr().cast(), | ||
| (&mut count) as *mut u32, |
There was a problem hiding this comment.
You are still casting unnecessarily.
| self.keys.as_mut_ptr().cast(), | ||
| self.values.as_mut_ptr().cast(), | ||
| (&mut count) as *mut u32, | ||
| &self.batch_opts as *const libbpf_sys::bpf_map_batch_opts, |
There was a problem hiding this comment.
Another unnecessary cast?
| if self.delete { | ||
| libbpf_sys::bpf_map_lookup_and_delete_batch( | ||
| self.map_fd.as_raw_fd(), | ||
| prev as _, |
There was a problem hiding this comment.
FWIW, I think this cast silently masks undefined behavior (mutation of a non-mut variable), which you could have easily spotted had you followed the suggestion of using cast thoroughly, because that's exactly what it's meant to catch.
Add a few fixes on top of pull request libbpf#996. Among others, remove an 'as' cast which may have been unsound, remove some other unnecessary ones, and don't repeat code needlessly. Signed-off-by: Daniel Müller <deso@posteo.net>
Add a few fixes on top of pull request libbpf#996. Among others, remove an 'as' cast which potentially could be the source of unsoundness, remove some other unnecessary ones, and don't repeat code needlessly. Signed-off-by: Daniel Müller <deso@posteo.net>
Add a few fixes on top of pull request libbpf#996. Among others, remove an 'as' cast which potentially could be the source of unsoundness, remove some other unnecessary ones, and don't repeat code needlessly. Signed-off-by: Daniel Müller <deso@posteo.net>
Add a few fixes on top of pull request #996. Among others, remove an 'as' cast which potentially could be the source of unsoundness, remove some other unnecessary ones, and don't repeat code needlessly. Signed-off-by: Daniel Müller <deso@posteo.net>
Hi all,
This adds the methods above to the
Maptrait (see #655) . Let me know what you think!Specifically, I was a bit unsure about the error handling in
BatchedMapIter. The libbpf docs mention that count is trustworthy for any non-EFAULT error. But I'm not sure if this is the right way to check this...