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@xxxxxxxxxxx>
[...]
>> +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
--
Alexis Lothoré, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
