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

[Matthieu Baerts <matttbe@kernel.org>]

Hi Geliang,

On 12/05/2024 10:41, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> Use pm_nl_ctl when 'ip mptcp' fails.
> Use SYS_NOFAIL in _ss_search() and drop 'ss -M'.
> 
> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> ---
>  tools/testing/selftests/bpf/prog_tests/mptcp.c | 17 ++++++-----------
>  1 file changed, 6 insertions(+), 11 deletions(-)
> 
> diff --git a/tools/testing/selftests/bpf/prog_tests/mptcp.c b/tools/testing/selftests/bpf/prog_tests/mptcp.c
> index 793b4b9c2bd2..2459c0e2a794 100644
> --- a/tools/testing/selftests/bpf/prog_tests/mptcp.c
> +++ b/tools/testing/selftests/bpf/prog_tests/mptcp.c
> @@ -362,7 +362,10 @@ 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)) {
> +		SYS(fail, "ip netns exec %s ./mptcp_pm_nl_ctl add %s flags %s",
> +		    NS_TEST, ADDR_2, flags);

As I mentioned in the v1, I would prefer only using 'ip mptcp', and I
find it strange to have a CI validating networking feature using an old
version of IPRoute2. But if we need an alternative method for the BPF
CI, I would prefer also having our CI using this method, to prevent any
issue with this method, and only realising that when upstreaming
patches, delaying them and causing confusions.

But feel free to add a command:

  /* It would be better to use  "ip -net %s mptcp endpoint add %s %s",
   * but the BPF CI is using an old version of IPRoute (5.5.0).
   */


> +	}
>  
>  	return 0;
>  fail:
> @@ -371,16 +374,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 -enita src %s dst %s %s %d | grep -q '%s'",
> +			  NS_TEST, src, dst, port, PORT_1, keyword);
>  }
>  
>  static int ss_search(char *src, char *keyword)

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.