On 01/08/2024 10:21, Alexis Lothoré wrote:
On 8/1/24 10:27, Alan Maguire wrote:
On 31/07/2024 11:38, Alexis Lothoré (eBPF Foundation) wrote:
test_cgroup_storage is currently a standalone program which is not run when executing test_progs.
Convert it to the test_progs framework so it can be automatically executed in CI. The conversion led to the following changes:
- converted the raw bpf program in the userspace test file into a dedicated test program in progs/ dir
- reduced the scope of cgroup_storage test: the content from this test overlaps with some other tests already present in test_progs, most notably netcnt and cgroup_storage_multi*. Those tests already check extensively local storage, per-cpu local storage, cgroups interaction, etc. So the new test only keep the part testing that the program return code (based on map content) properly leads to packet being passed or dropped.
Signed-off-by: Alexis Lothoré (eBPF Foundation) alexis.lothore@bootlin.com
Two small things below, but
Reviewed-by: Alan Maguire alan.maguire@oracle.com
[...]
+#define PING_CMD "ping localhost -c 1 -W 1 -q"
other tests seem to redirect ping stdout output to /dev/null ; might be worth doing that too.
That's in fact performed automatically by SYS_NOFAIL :)
#define SYS_NOFAIL(fmt, ...) \
({ \ char cmd[1024]; \ int n; \ n = snprintf(cmd, sizeof(cmd), fmt, ##__VA_ARGS__); \ if (n < sizeof(cmd) && sizeof(cmd) - n >= sizeof(ALL_TO_DEV_NULL)) \ strcat(cmd, ALL_TO_DEV_NULL); \ system(cmd); \ })
[...]
Perfect, I missed that.
+{
- __u64 *counter;
- counter = bpf_get_local_storage(&cgroup_storage, 0);
don't we need a NULL check for counter here? Or does the verifier know bpf_get_local_storage never fails?
Good question. Since the verifier accepted the prog during my tests, I indeed assume that the returned pointer is always valid. Amongst all calls to this function in progs involved in selftests, I found only one performing a check before using the value (lsm_cgroup.c). So I guess it is fine ?
Looks like the prototype for the helper specifies a return type of RET_PTR_TO_MAP_VALUE ; if it was RET_PTR_TO_MAP_VALUE_OR_NULL we'd need the NULL check, but because it's a guaranteed map ptr we are good here without a NULL check.
Thanks!
Alan