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@xxxxxxxxxxx>
>
> Two small things below, but
>
> Reviewed-by: Alan Maguire <alan.maguire@xxxxxxxxxx>
[...]
>> +#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); \
})
[...]
>> +{
>> + __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 ?
Thanks for the review !
Alexis
--
Alexis Lothoré, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
