Re: [PATCH 2/3] selftests/bpf: integrate test_xdp_veth into test_progs

[Alexis Lothoré]

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