-
Notifications
You must be signed in to change notification settings - Fork 95
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
maps: Handle GetValueAndDeleteBatch
return code appropiately
#157
maps: Handle GetValueAndDeleteBatch
return code appropiately
#157
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good. I think we might get away without changing the signatures.
Also, I have an inlined question.
420f323
to
81933ce
Compare
Long story short, some libbpf APIs that don't return pointers, such as the batch APIs might return `-1` when there's a 'partial success'. In the case of these APIs, with errno=ENOENT, we know that we read and deleted less than `count` entries, but it was still overall sucessful. For more more background see the inline comments. This PR fixes this for `GetValueAndDeleteBatch` only. We aren't addressing all the other batch calls yet as we haven't used them yet. Thanks!
81933ce
to
f2b3ab8
Compare
BTW this PR should fix #147 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This LGTM! Thanks for your help.
I'd really like to make sure each batch API is updated for correctness alongside this. Do you plan on doing so? It's alright if not, I just want to add it to my queue and keep track!
@grantseltzer I will probably tackle the |
This PR is based off [@kakkoyun's work](parca-dev#326) to use libbpf(go)'s batch APIs. **Context** The main issue we found while working with this API was that they were erroring with `EPERM`. After some debugging, we realised that libbpf wasn't handle with errors in the way we expected. The debugging write-up and more context can be found [here](aquasecurity/libbpfgo#159), and the fix is in [this PR](aquasecurity/libbpfgo#157). After landing these changes upstream, we pointed to the updated libbpfgo version, as well as added some [regression tests](parca-dev#381) to ensure that libbpfgo behaves as expected, and to make it easier in the future to write further compatibility tests. Note: the rest of the batch APIs error handling is still unfixed. Tracking in aquasecurity/libbpfgo#162.
This PR is based off [@kakkoyun's work](parca-dev#326) to use libbpf(go)'s batch APIs. **Context** The main issue we found while working with this API was that they were erroring with `EPERM`. After some debugging, we realised that libbpf wasn't handle with errors in the way we expected. The debugging write-up and more context can be found [here](aquasecurity/libbpfgo#159), and the fix is in [this PR](aquasecurity/libbpfgo#157). After landing these changes upstream, we pointed to the updated libbpfgo version, as well as added some [regression tests](parca-dev#381) to ensure that libbpfgo behaves as expected, and to make it easier in the future to write further compatibility tests. Note: the rest of the batch APIs error handling is still unfixed. Tracking in aquasecurity/libbpfgo#162.
This PR is based off [@kakkoyun's work](parca-dev#326) to use libbpf(go)'s batch APIs. **Context** The main issue we found while working with this API was that they were erroring with `EPERM`. After some debugging, we realised that libbpf wasn't handle with errors in the way we expected. The debugging write-up and more context can be found [here](aquasecurity/libbpfgo#159), and the fix is in [this PR](aquasecurity/libbpfgo#157). After landing these changes upstream, we pointed to the updated libbpfgo version, as well as added some [regression tests](parca-dev#381) to ensure that libbpfgo behaves as expected, and to make it easier in the future to write further compatibility tests. Note: the rest of the batch APIs error handling is still unfixed. Tracking in aquasecurity/libbpfgo#162.
This PR is based off [@kakkoyun's work](parca-dev#326) to use libbpf(go)'s batch APIs. **Context** The main issue we found while working with this API was that they were erroring with `EPERM`. After some debugging, we realised that libbpf wasn't handle with errors in the way we expected. The debugging write-up and more context can be found [here](aquasecurity/libbpfgo#159), and the fix is in [this PR](aquasecurity/libbpfgo#157). After landing these changes upstream, we pointed to the updated libbpfgo version, as well as added some [regression tests](parca-dev#381) to ensure that libbpfgo behaves as expected, and to make it easier in the future to write further compatibility tests. Note: the rest of the batch APIs error handling is still unfixed. Tracking in aquasecurity/libbpfgo#162.
This PR is based off [@kakkoyun's work](parca-dev#326) to use libbpf(go)'s batch APIs. **Context** The main issue we found while working with this API was that they were erroring with `EPERM`. After some debugging, we realised that libbpf wasn't handling errors in the way we expected. The debugging write-up and more context can be found [here](aquasecurity/libbpfgo#159), and the fix is in [this PR](aquasecurity/libbpfgo#157). After landing these changes upstream, we pointed to the updated libbpfgo version, as well as added some [regression tests](parca-dev#381) to ensure that libbpfgo behaves as expected, and to make it easier in the future to write further compatibility tests. Note: the rest of the batch APIs error handling is still unfixed. Tracking in aquasecurity/libbpfgo#162.
Long story short, some libbpf APIs that don't return pointers, such as the
batch APIs might return
-1
when there's a 'partial success'. In the caseof these APIs, with
errno=-ENOENT
, we know that we read and deleted less thancount
entries, but it was still overall sucessful.This PR fixes this for
GetValueAndDeleteBatch
only. We aren't addressingall the other batch calls yet as we haven't used them
yet.
Kemal originally tacked this in #152, but unfortunately
not checking the return code and
errno
makes the interpretation of return valuesbogus. I have traced the kernel code and we never returned
EFAULT
at least in thislibbpf call.
Test plan
Thanks!
cc/ @Sylfrena @v-thakkar @kakkoyun