Re: [PATCH mptcp-next v6 2/9] Squash to "selftests/bpf: Add bpf scheduler test" 1 verify

[Matthieu Baerts <matttbe@kernel.org>]

Hi Geliang,

On 04/04/2024 15:03, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> Add send_data_and_verify helper.

Please always add the reason, even if it is just a "to avoid duplicated
code".
> 
> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> ---
>  .../testing/selftests/bpf/prog_tests/mptcp.c  | 40 ++++++++++++++-----
>  1 file changed, 30 insertions(+), 10 deletions(-)
> 
> diff --git a/tools/testing/selftests/bpf/prog_tests/mptcp.c b/tools/testing/selftests/bpf/prog_tests/mptcp.c
> index c29c81239603..e1114745da63 100644
> --- a/tools/testing/selftests/bpf/prog_tests/mptcp.c
> +++ b/tools/testing/selftests/bpf/prog_tests/mptcp.c
> @@ -457,23 +457,44 @@ static int has_bytes_sent(char *addr)
>  	return system(cmd);
>  }
>  
> -static void test_default(void)
> +static void send_data_and_verify(char *msg, int addr1, int addr2)
>  {
>  	int server_fd, client_fd;
> -	struct nstoken *nstoken;
>  
> -	nstoken = sched_init("subflow", "default");
> -	if (!ASSERT_OK_PTR(nstoken, "sched_init:default"))
> -		goto fail;
>  	server_fd = start_mptcp_server(AF_INET, ADDR_1, PORT_1, 0);
> +	if (!ASSERT_NEQ(server_fd, -1, "start_mptcp_server"))
> +		return;
> +
>  	client_fd = connect_to_fd(server_fd, 0);
> +	if (!ASSERT_NEQ(client_fd, -1, "connect_to_fd"))
> +		goto close_server;
>  
> -	send_data(server_fd, client_fd, "default");
> -	ASSERT_OK(has_bytes_sent(ADDR_1), "has_bytes_sent addr_1");
> -	ASSERT_OK(has_bytes_sent(ADDR_2), "has_bytes_sent addr_2");
> +	send_data(server_fd, client_fd, msg);
> +
> +	if (addr1)

Maybe clearer to use booleans if you use addr1 and addr2 as such, no?

> +		ASSERT_OK(has_bytes_sent(ADDR_1), "Should have bytes_sent on addr1");
> +	else
> +		ASSERT_GT(has_bytes_sent(ADDR_1), 0, "Shouldn't have bytes_sent on addr1");
> +	if (addr2)
> +		ASSERT_OK(has_bytes_sent(ADDR_2), "Should have bytes_sent on addr2");
> +	else
> +		ASSERT_GT(has_bytes_sent(ADDR_2), 0, "Shouldn't have bytes_sent on addr2");

In case of error, will we directly understand from which scheduler the
error is linked to? I understood that some BPF tests can be run in
parallel, maybe we need a prefix to know which tests had an issue? Or
maybe the prefix is not needed because these schedulers tests are
executed in sequence and we print which one is being executed?

In other words, what do you see if on purpose you create an error by
inverting 'addr2' value for example in only one sched test, and you
launch the tests with 'test_progs -j'?

>  
>  	close(client_fd);
> +close_server:
>  	close(server_fd);
> +}
> +
> +static void test_default(void)
> +{
> +	struct nstoken *nstoken;
> +
> +	nstoken = sched_init("subflow", "default");
> +	if (!ASSERT_OK_PTR(nstoken, "sched_init:default"))
> +		goto fail;
> +
> +	send_data_and_verify("default", 1, 1);


It is good to have such helper to reduce duplicated code, but having:

  1, 1

is really not clear, no?

Maybe clearer (especially later if you use this a lot), to use macros?
e.g. to have:

  send_data_and_verify("default", USE_ADDR1, NO_ADDR2);

or

  send_data_and_verify("default", WITH_DATA, WITHOUT_DATA);

(we might understand one is for addr1 and the other for addr2)

WDYT? I don't know if it is clearer.

> +
>  fail:
>  	cleanup_netns(nstoken);
>  }
> @@ -663,8 +684,7 @@ void test_mptcp(void)
>  {
>  	RUN_MPTCP_TEST(base);
>  	RUN_MPTCP_TEST(mptcpify);
> -	if (test__start_subtest("default"))
> -		test_default();
> +	RUN_MPTCP_TEST(default);
>  	if (test__start_subtest("first"))
>  		test_first();
>  	if (test__start_subtest("bkup"))

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