Hello Stanislas, thanks for the review
On 7/12/24 03:10, Stanislav Fomichev wrote:
On 07/11, Alexis Lothoré (eBPF Foundation) wrote:
test_xdp_veth.sh tests that XDP return codes work as expected, by bringing up multiple veth pairs isolated in different namespaces, attaching specific xdp programs to each interface, and ensuring that the whole chain allows to ping one end interface from the first one. The test runs well but is currently not integrated in test_progs, which prevents it from being run automatically in the CI infrastructure.
Rewrite it as a C test relying on libbpf to allow running it in the CI infrastructure. The new code brings up the same network infrastructure and reuses the same eBPF programs as test_xdp_veth.sh, for which skeletons are already generated by the bpf tests makefile.
Signed-off-by: Alexis Lothoré alexis.lothore@bootlin.com
[...]
+static void generate_random_ns_name(int index, char *out) +{
- int random, count, i;
- count = snprintf(out, NS_NAME_MAX_LEN, "ns%d-", index);
- for(i=0; i<NS_SUFFIX_LEN; i++) {
random=rand() % 2;out[count++]= random ? 'a' + rand() % 26 : 'A' + rand() % 26;- }
- out[count] = 0;
+}
It's been customary to hard-code netns names for all the tests we have, so maybe it's ok here as well?
I indeed wondered if it was really useful to bring this random ns name mechanism from the shell script, but I saw that it has been brought by the dedicated commit 9d66c9ddc9fc ("selftests/bpf/test_xdp_veth: use temp netns for testing"), so I assumed that some real issues about static ns names were encountered and led to this fix. Maybe it is indeed enough if I hardcode ns names but not with a too generic prefix ?
+static int attach_programs_to_veth_pair(struct skeletons *skeletons, int index) +{
- struct bpf_program *local_prog, *remote_prog;
- struct bpf_link **local_link, **remote_link;
- struct nstoken *nstoken;
- struct bpf_link *link;
- int interface;
[..]
- switch(index) {
Can you pls run the patch through the checkpatch.pl? The formatting looks wrong, should be 'switch (index)'. Applies to 'if()' elsewhere as well.
Crap, I forgot this very basic part, my bad, I'll fix all those small issues.
[..]
snprintf(cmd, IP_CMD_MAX_LEN, "ip netns del %s", config[i].namespace);system(cmd);SYS_NOFAIL to avoid separate snprintf?
[...]
+static int check_ping(struct skeletons *skeletons) +{
- char cmd[IP_CMD_MAX_LEN];
- /* Test: if all interfaces are properly configured, we must be able to ping
* veth33 from veth11*/- snprintf(cmd, IP_CMD_MAX_LEN,
"ip netns exec %s ping -c 1 -W 1 %s > /dev/null",config[0].namespace, IP_DST);- return system(cmd);
SYS_NOFAL here as well?
Thanks for the tip, I'll use this macro.
Btw, not sure it makes sense to split that work into 3 patches. After you first patch the test is broken anyway, so might as well just delete the script at that point...
I have made sure that the sh script still runs correctly even after renaming the sections in the xdp program. But indeed, maybe I can squash the new test patch and the shell scrip deletion patch.
Thanks,
Alexis