Re: [PATCH mptcp-next] Squash to "selftests/bpf: Add mptcp subflow subtest"

[Matthieu Baerts <matttbe@kernel.org>]

Hi Geliang,

Thank you for this fix!

11 May 2024 12:42:08 Geliang Tang <geliang@kernel.org>:

> From: Geliang Tang <tanggeliang@kylinos.cn>
>
> Add pm_nl_ctl.
> Use SYS_NOFAIL in _ss_search().
>
> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> ---
> tools/testing/selftests/bpf/Makefile           |  2 +-
> tools/testing/selftests/bpf/pm_nl_ctl.c        |  1 +
> tools/testing/selftests/bpf/prog_tests/mptcp.c | 15 ++++-----------
> 3 files changed, 6 insertions(+), 12 deletions(-)
> create mode 120000 tools/testing/selftests/bpf/pm_nl_ctl.c
>
> diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
> index b90c718218ae..bd2e3b138a5c 100644
> --- a/tools/testing/selftests/bpf/Makefile
> +++ b/tools/testing/selftests/bpf/Makefile
> @@ -131,7 +131,7 @@ TEST_GEN_PROGS_EXTENDED = test_sock_addr test_skb_cgroup_id_user \
>     flow_dissector_load test_flow_dissector test_tcp_check_syncookie_user \
>     test_lirc_mode2_user xdping test_cpp runqslower bench bpf_testmod.ko \
>     xskxceiver xdp_redirect_multi xdp_synproxy veristat xdp_hw_metadata \
> -   xdp_features bpf_test_no_cfi.ko
> +   xdp_features bpf_test_no_cfi.ko pm_nl_ctl
>
> TEST_GEN_FILES += liburandom_read.so urandom_read sign-file uprobe_multi
>
> diff --git a/tools/testing/selftests/bpf/pm_nl_ctl.c b/tools/testing/selftests/bpf/pm_nl_ctl.c
> new file mode 120000
> index 000000000000..5a08c255b278
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/pm_nl_ctl.c

Best to prefix it with "mptcp_": clearer and it will be linked to MPTCP in the maintainers file.

> @@ -0,0 +1 @@
> +../net/mptcp/pm_nl_ctl.c
> \ No newline at end of file
> diff --git a/tools/testing/selftests/bpf/prog_tests/mptcp.c b/tools/testing/selftests/bpf/prog_tests/mptcp.c
> index 793b4b9c2bd2..9c6d1e4f6f35 100644
> --- a/tools/testing/selftests/bpf/prog_tests/mptcp.c
> +++ b/tools/testing/selftests/bpf/prog_tests/mptcp.c
> @@ -362,7 +362,8 @@ static int endpoint_init(char *flags)
>     SYS(fail, "ip -net %s link set dev veth1 up", NS_TEST);
>     SYS(fail, "ip -net %s addr add %s/24 dev veth2", NS_TEST, ADDR_2);
>     SYS(fail, "ip -net %s link set dev veth2 up", NS_TEST);
> -   SYS(fail, "ip -net %s mptcp endpoint add %s %s", NS_TEST, ADDR_2, flags);
> +   if (SYS_NOFAIL("ip -net %s mptcp endpoint add %s %s", NS_TEST, ADDR_2, flags))

It is maybe better to only use mptcp_pm_nl_ctl here: to maintain one way here, our CI will do the same as what the BPF one will do, and we avoid errors printed in stderr if "ip mptcp" is not supported.

> +       SYS(fail, "ip netns exec %s ./pm_nl_ctl add %s flags %s", NS_TEST, ADDR_2, flags);
>
>     return 0;
> fail:
> @@ -371,16 +372,8 @@ static int endpoint_init(char *flags)
>
> static int _ss_search(char *src, char *dst, char *port, char *keyword)
> {
> -   char cmd[128];
> -   int n;
> -
> -   n = snprintf(cmd, sizeof(cmd),
> -            "ip netns exec %s ss -Menita src %s dst %s %s %d | grep -q '%s'",
> -            NS_TEST, src, dst, port, PORT_1, keyword);
> -   if (n < 0 || n >= sizeof(cmd))
> -       return -1;
> -
> -   return system(cmd);
> +   return SYS_NOFAIL("ip netns exec %s ss -Menita src %s dst %s %s %d | grep -q '%s'",
> +             NS_TEST, src, dst, port, PORT_1, keyword);

If "ip mptcp" is not supported, I guess "ss -M" will not be supported as well, no?

Do we need -M here for these tests?

Cheers,
Matt

> }
>
> static int ss_search(char *src, char *keyword)
> --
> 2.43.0