bpf: unit tests and fixes for prepend_name function
#1902
Conversation
mtardy
added
the
release-note/bug
✅ Deploy Preview for tetragon ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
mtardy
force-pushed
the
pr/mtardy/bpf-fix-prend-name
branch
14 times, most recently
from
1f8b455 to
436aee2
Compare
mtardy
added
area/bpf
| __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.
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>
mtardy
force-pushed
the
pr/mtardy/bpf-fix-prend-name
branch
from
436aee2 to
d23a34a
Compare
|
Note that this has a chance of fixing #1707, it can be related. |
| // 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.
Question: Why do we need that?
There was a problem hiding this comment.
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.
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!