diff mbox series

[mptcp-next,v1,3/8] Squash to "selftests/bpf: Add bpf scheduler test"

Message ID c02d0f8555c7427d58155d34709f2c8250c8ee2c.1741226722.git.tanggeliang@kylinos.cn (mailing list archive)
State Accepted, archived
Commit a1aaa0e29d017376e210b9d6548df94b81b558eb
Delegated to: Matthieu Baerts
Headers show
Series cleanups for bpf schedulers | expand

Checks

Context Check Description
matttbe/build success Build and static analysis OK
matttbe/checkpatch success total: 0 errors, 0 warnings, 0 checks, 44 lines checked
matttbe/shellcheck success MPTCP selftests files have not been modified
matttbe/KVM_Validation__normal success Success! ✅
matttbe/KVM_Validation__debug success Success! ✅
matttbe/KVM_Validation__btf-normal__only_bpftest_all_ success Success! ✅
matttbe/KVM_Validation__btf-debug__only_bpftest_all_ success Success! ✅

Commit Message

Geliang Tang March 6, 2025, 2:15 a.m. UTC 
From: Geliang Tang <tanggeliang@kylinos.cn>

A cleanup, move netns_new() out of sched_init().

Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 .../testing/selftests/bpf/prog_tests/mptcp.c  | 22 +++++++++----------
 1 file changed, 10 insertions(+), 12 deletions(-)

Comments

Mat Martineau March 18, 2025, 1:11 a.m. UTC | #1 
On Thu, 6 Mar 2025, Geliang Tang wrote:

> From: Geliang Tang <tanggeliang@kylinos.cn>
>
> A cleanup, move netns_new() out of sched_init().
>
> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> ---
> .../testing/selftests/bpf/prog_tests/mptcp.c  | 22 +++++++++----------
> 1 file changed, 10 insertions(+), 12 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/mptcp.c b/tools/testing/selftests/bpf/prog_tests/mptcp.c
> index ac6f6a6f7700..bd824b19ee13 100644
> --- a/tools/testing/selftests/bpf/prog_tests/mptcp.c
> +++ b/tools/testing/selftests/bpf/prog_tests/mptcp.c
> @@ -562,23 +562,16 @@ static void test_iters_subflow(void)
> 	close(cgroup_fd);
> }
>
> -static struct netns_obj *sched_init(char *flags, char *sched)
> +static int sched_init(char *flags, char *sched)
> {
> -	struct netns_obj *netns;
> -
> -	netns = netns_new(NS_TEST, true);
> -	if (!ASSERT_OK_PTR(netns, "netns_new"))
> -		return NULL;
> -
> -	if (endpoint_init("subflow", 2) < 0)
> +	if (endpoint_init(flags, 2) < 0)
> 		goto fail;
>
> 	SYS(fail, "ip netns exec %s sysctl -qw net.mptcp.scheduler=%s", NS_TEST, sched);
>
> -	return netns;
> +	return 0;
> fail:
> -	netns_free(netns);
> -	return NULL;
> +	return -1;

A small thing: since there's now only the 'return' on this error path, I 
suggest removing the goto and directly returning -1 above. Matthieu do you 
want to do this when applying, or just leave it as-is?

- Mat

> }
>
> static int ss_search(char *src, char *dst, char *port, char *keyword)
> @@ -636,11 +629,16 @@ static void send_data_and_verify(char *sched, bool addr1, bool addr2)
> static void test_default(void)
> {
> 	struct netns_obj *netns;
> +	int err;
>
> -	netns = sched_init("subflow", "default");
> +	netns = netns_new(NS_TEST, true);
> 	if (!netns)
> 		goto fail;
>
> +	err = sched_init("subflow", "default");
> +	if (!ASSERT_OK(err, "sched_init"))
> +		goto fail;
> +
> 	send_data_and_verify("default", WITH_DATA, WITH_DATA);
>
> fail:
> -- 
> 2.43.0
>
>
>
Matthieu Baerts March 20, 2025, 7:38 p.m. UTC | #2 
Hi Mat,

On 18/03/2025 02:11, Mat Martineau wrote:
> On Thu, 6 Mar 2025, Geliang Tang wrote:
> 
>> From: Geliang Tang <tanggeliang@kylinos.cn>
>>
>> A cleanup, move netns_new() out of sched_init().
>>
>> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
>> ---
>> .../testing/selftests/bpf/prog_tests/mptcp.c  | 22 +++++++++----------
>> 1 file changed, 10 insertions(+), 12 deletions(-)
>>
>> diff --git a/tools/testing/selftests/bpf/prog_tests/mptcp.c b/tools/
>> testing/selftests/bpf/prog_tests/mptcp.c
>> index ac6f6a6f7700..bd824b19ee13 100644
>> --- a/tools/testing/selftests/bpf/prog_tests/mptcp.c
>> +++ b/tools/testing/selftests/bpf/prog_tests/mptcp.c
>> @@ -562,23 +562,16 @@ static void test_iters_subflow(void)
>>     close(cgroup_fd);
>> }
>>
>> -static struct netns_obj *sched_init(char *flags, char *sched)
>> +static int sched_init(char *flags, char *sched)
>> {
>> -    struct netns_obj *netns;
>> -
>> -    netns = netns_new(NS_TEST, true);
>> -    if (!ASSERT_OK_PTR(netns, "netns_new"))
>> -        return NULL;
>> -
>> -    if (endpoint_init("subflow", 2) < 0)
>> +    if (endpoint_init(flags, 2) < 0)
>>         goto fail;
>>
>>     SYS(fail, "ip netns exec %s sysctl -qw net.mptcp.scheduler=%s",
>> NS_TEST, sched);
>>
>> -    return netns;
>> +    return 0;
>> fail:
>> -    netns_free(netns);
>> -    return NULL;
>> +    return -1;
> 
> A small thing: since there's now only the 'return' on this error path, I
> suggest removing the goto and directly returning -1 above. Matthieu do
> you want to do this when applying, or just leave it as-is?

I did the modification, but I missed the fact SYS() still needs the
'fail' label. So I just added these two commit to keep the 'fail' label,
and to return -1 directly.

a7522f3888cd: Revert "Squash to "selftests/bpf: Add bpf scheduler test""
cc33eee80a21: Squash to "selftests/bpf: Add bpf scheduler test"

Cheers,
Matt
diff mbox series

Patch

diff --git a/tools/testing/selftests/bpf/prog_tests/mptcp.c b/tools/testing/selftests/bpf/prog_tests/mptcp.c
index ac6f6a6f7700..bd824b19ee13 100644
--- a/tools/testing/selftests/bpf/prog_tests/mptcp.c
+++ b/tools/testing/selftests/bpf/prog_tests/mptcp.c
@@ -562,23 +562,16 @@  static void test_iters_subflow(void)
 	close(cgroup_fd);
 }
 
-static struct netns_obj *sched_init(char *flags, char *sched)
+static int sched_init(char *flags, char *sched)
 {
-	struct netns_obj *netns;
-
-	netns = netns_new(NS_TEST, true);
-	if (!ASSERT_OK_PTR(netns, "netns_new"))
-		return NULL;
-
-	if (endpoint_init("subflow", 2) < 0)
+	if (endpoint_init(flags, 2) < 0)
 		goto fail;
 
 	SYS(fail, "ip netns exec %s sysctl -qw net.mptcp.scheduler=%s", NS_TEST, sched);
 
-	return netns;
+	return 0;
 fail:
-	netns_free(netns);
-	return NULL;
+	return -1;
 }
 
 static int ss_search(char *src, char *dst, char *port, char *keyword)
@@ -636,11 +629,16 @@  static void send_data_and_verify(char *sched, bool addr1, bool addr2)
 static void test_default(void)
 {
 	struct netns_obj *netns;
+	int err;
 
-	netns = sched_init("subflow", "default");
+	netns = netns_new(NS_TEST, true);
 	if (!netns)
 		goto fail;
 
+	err = sched_init("subflow", "default");
+	if (!ASSERT_OK(err, "sched_init"))
+		goto fail;
+
 	send_data_and_verify("default", WITH_DATA, WITH_DATA);
 
 fail: