-
Notifications
You must be signed in to change notification settings - Fork 341
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
bpf: unit tests and fixes for prepend_name
function
#1902
Conversation
✅ Deploy Preview for tetragon ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
1f8b455
to
436aee2
Compare
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.
Just a couple nits, but LGTM. Might be helpful to have someone else do a pass of the prepend_name
changes, but I didn't find any obvious bugs.
__type(value, struct test_prepend_name_state_map_value); | ||
} test_prepend_name_state_map SEC(".maps"); | ||
|
||
__attribute__((section("raw_tracepoint/test"), used)) int |
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 BPF unit testing is extremely valuable and honestly long overdue. Thanks for adding the first of hopefully many to come
In order to make BPF_PROG_RUN work with BPF_PROG_TYPE_RAW_TRACEPOINT in cilium/ebpf. Signed-off-by: Mahe Tardy <mahe.tardy@gmail.com>
It looked like this particular function presented some bugs: - It would not return -ENAMETOOLONG when the buffer was filled "best-effort" because the name was not fitting. - It would never fill the first byte of the buffer, thus reducing the maximum size written by one. - It would not fill "best-effort" correctly. E.g, previously in a buffer of 4, "pizza" would have been "/piz" (when ignoring previous bug) instead of "izza". Now it tries to write the "end" of the name without a wrong "/" char. - It would behave unexpectedly when filled with more data than size of buffer. BPF unit tests were put under bpf/tests, as more can be added there in the future. The Makefile was updated so that we can share variable definition between the bpf/Makefile and the bpf/tests/Makefile file. This separation allows us not compile the test program along the rest of the BPF progs, and not ship them in the released images/archives. Signed-off-by: Mahe Tardy <mahe.tardy@gmail.com>
Unit tests were added in the previous commit for this function, highlighting some of the issues it presented. This commit introduce a few changes trying to fix the detected bugs. Signed-off-by: Mahe Tardy <mahe.tardy@gmail.com>
Signed-off-by: Mahe Tardy <mahe.tardy@gmail.com>
436aee2
to
d23a34a
Compare
Note that this has a chance of fixing #1707, it can be related. |
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.
LGTM, thanks! Having a skeleton to build eBPF unit tests would be really useful!
// SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) | ||
/* Copyright Authors of Cilium */ | ||
|
||
//go:build ignore |
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.
Question: Why do we need that?
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.
So this is a bit weird but you can't have a .c
file in the same directory as a go module unless you are using CGO 🤔 most Go commands will return C source files not allowed when not using cgo
.
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.
I wanted to house the C file with the BPF test harness and the Go file in the same directory so it's needed!
🚀 @mtardy I feel like we should backport this (or at least the buffix) to 1.0. WDYT? |
Yes, it would make sense since it might fix a user facing bug. Let me do that! |
See commits!