[PATCH mptcp-next v6 5/8] selftests/bpf: Add MPTCP_BASE_TEST macro

Geliang Tang posted 8 patches 1 year, 10 months ago
There is a newer version of this series
[PATCH mptcp-next v6 5/8] selftests/bpf: Add MPTCP_BASE_TEST macro
Posted by Geliang Tang 1 year, 10 months ago
From: Geliang Tang <tanggeliang@kylinos.cn>

This patch adds a macro MPTCP_BASE_TEST to unify all MPTCP tests.
test_mptcp_sock() can be replaced by MPTCP_BASE_TEST(mptcp_sock),
and test_mptcpify() can be replaced by MPTCP_BASE_TEST(mptcpify).

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

diff --git a/tools/testing/selftests/bpf/prog_tests/mptcp.c b/tools/testing/selftests/bpf/prog_tests/mptcp.c
index 50a932e3ad23..6043947f178c 100644
--- a/tools/testing/selftests/bpf/prog_tests/mptcp.c
+++ b/tools/testing/selftests/bpf/prog_tests/mptcp.c
@@ -224,37 +224,6 @@ static void run_mptcp_sock(int cgroup_fd, struct mptcp_sock *skel)
 	close(server_fd);
 }
 
-static void test_mptcp_sock(void)
-{
-	struct nstoken *nstoken = NULL;
-	struct mptcp_sock *sock_skel;
-	int cgroup_fd, err;
-
-	cgroup_fd = test__join_cgroup("/mptcp");
-	if (!ASSERT_GE(cgroup_fd, 0, "test__join_cgroup"))
-		return;
-
-	sock_skel = mptcp_sock__open_and_load();
-	if (!ASSERT_OK_PTR(sock_skel, "skel_open_load"))
-		goto out;
-
-	err = mptcp_sock__attach(sock_skel);
-	if (!ASSERT_OK(err, "skel_attach"))
-		goto out;
-
-	nstoken = create_netns();
-	if (!ASSERT_OK_PTR(nstoken, "create_netns"))
-		goto fail;
-
-	run_mptcp_sock(cgroup_fd, sock_skel);
-
-fail:
-	cleanup_netns(nstoken);
-	mptcp_sock__destroy(sock_skel);
-out:
-	close(cgroup_fd);
-}
-
 static int verify_mptcpify(int server_fd, int client_fd)
 {
 	struct __mptcp_info info;
@@ -310,43 +279,46 @@ static void run_mptcpify(int cgroup_fd, struct mptcpify *skel)
 	close(server_fd);
 }
 
-static void test_mptcpify(void)
-{
-	struct nstoken *nstoken = NULL;
-	struct mptcpify *mptcpify_skel;
-	int cgroup_fd;
-	int err;
-
-	cgroup_fd = test__join_cgroup("/mptcpify");
-	if (!ASSERT_GE(cgroup_fd, 0, "test__join_cgroup"))
-		return;
-
-	mptcpify_skel = mptcpify__open_and_load();
-	if (!ASSERT_OK_PTR(mptcpify_skel, "skel_open_load"))
-		goto out;
-
-	err = mptcpify__attach(mptcpify_skel);
-	if (!ASSERT_OK(err, "skel_attach"))
-		goto out;
-
-	nstoken = create_netns();
-	if (!ASSERT_OK_PTR(nstoken, "create_netns"))
-		goto fail;
-
-	run_mptcpify(cgroup_fd, mptcpify_skel);
-
-fail:
-	cleanup_netns(nstoken);
-	mptcpify__destroy(mptcpify_skel);
-out:
-	close(cgroup_fd);
-}
-
 static const unsigned int total_bytes = 10 * 1024 * 1024;
 #define ADDR_1	"10.0.1.1"
 #define ADDR_2	"10.0.1.2"
 #define PORT_1	10001
 
+#define MPTCP_BASE_TEST(name)					\
+static void test_##name(void)					\
+{								\
+	struct nstoken *nstoken;				\
+	int cgroup_fd, err;					\
+	struct name *skel;					\
+								\
+	cgroup_fd = test__join_cgroup("/" #name);		\
+	if (!ASSERT_GE(cgroup_fd, 0, "join_cgroup " #name))	\
+		return;						\
+								\
+	skel = name##__open_and_load();				\
+	if (!ASSERT_OK_PTR(skel, "skel_open_load " #name))	\
+		goto close_cgroup;				\
+								\
+	err = name##__attach(skel);				\
+	if (!ASSERT_OK(err, "skel_attach " #name))		\
+		goto skel_destroy;				\
+								\
+	nstoken = create_netns();				\
+	if (!ASSERT_OK_PTR(nstoken, "create_netns " #name))	\
+		goto skel_destroy;				\
+								\
+	run_##name(cgroup_fd, skel);				\
+								\
+	cleanup_netns(nstoken);					\
+skel_destroy:							\
+	name##__destroy(skel);					\
+close_cgroup:							\
+	close(cgroup_fd);					\
+}
+
+MPTCP_BASE_TEST(mptcp_sock);
+MPTCP_BASE_TEST(mptcpify);
+
 static struct nstoken *sched_init(char *flags, char *sched)
 {
 	struct nstoken *nstoken;
-- 
2.40.1
Re: [PATCH mptcp-next v6 5/8] selftests/bpf: Add MPTCP_BASE_TEST macro
Posted by Mat Martineau 1 year, 10 months ago
On Tue, 2 Apr 2024, Geliang Tang wrote:

> From: Geliang Tang <tanggeliang@kylinos.cn>
>
> This patch adds a macro MPTCP_BASE_TEST to unify all MPTCP tests.
> test_mptcp_sock() can be replaced by MPTCP_BASE_TEST(mptcp_sock),
> and test_mptcpify() can be replaced by MPTCP_BASE_TEST(mptcpify).
>
> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> ---
> .../testing/selftests/bpf/prog_tests/mptcp.c  | 98 +++++++------------
> 1 file changed, 35 insertions(+), 63 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/mptcp.c b/tools/testing/selftests/bpf/prog_tests/mptcp.c
> index 50a932e3ad23..6043947f178c 100644
> --- a/tools/testing/selftests/bpf/prog_tests/mptcp.c
> +++ b/tools/testing/selftests/bpf/prog_tests/mptcp.c
> @@ -224,37 +224,6 @@ static void run_mptcp_sock(int cgroup_fd, struct mptcp_sock *skel)
> 	close(server_fd);
> }
>
> -static void test_mptcp_sock(void)
> -{
> -	struct nstoken *nstoken = NULL;
> -	struct mptcp_sock *sock_skel;
> -	int cgroup_fd, err;
> -
> -	cgroup_fd = test__join_cgroup("/mptcp");
> -	if (!ASSERT_GE(cgroup_fd, 0, "test__join_cgroup"))
> -		return;
> -
> -	sock_skel = mptcp_sock__open_and_load();
> -	if (!ASSERT_OK_PTR(sock_skel, "skel_open_load"))
> -		goto out;
> -
> -	err = mptcp_sock__attach(sock_skel);
> -	if (!ASSERT_OK(err, "skel_attach"))
> -		goto out;
> -
> -	nstoken = create_netns();
> -	if (!ASSERT_OK_PTR(nstoken, "create_netns"))
> -		goto fail;
> -
> -	run_mptcp_sock(cgroup_fd, sock_skel);
> -
> -fail:
> -	cleanup_netns(nstoken);
> -	mptcp_sock__destroy(sock_skel);
> -out:
> -	close(cgroup_fd);
> -}
> -
> static int verify_mptcpify(int server_fd, int client_fd)
> {
> 	struct __mptcp_info info;
> @@ -310,43 +279,46 @@ static void run_mptcpify(int cgroup_fd, struct mptcpify *skel)
> 	close(server_fd);
> }
>
> -static void test_mptcpify(void)
> -{
> -	struct nstoken *nstoken = NULL;
> -	struct mptcpify *mptcpify_skel;
> -	int cgroup_fd;
> -	int err;
> -
> -	cgroup_fd = test__join_cgroup("/mptcpify");
> -	if (!ASSERT_GE(cgroup_fd, 0, "test__join_cgroup"))
> -		return;
> -
> -	mptcpify_skel = mptcpify__open_and_load();
> -	if (!ASSERT_OK_PTR(mptcpify_skel, "skel_open_load"))
> -		goto out;
> -
> -	err = mptcpify__attach(mptcpify_skel);
> -	if (!ASSERT_OK(err, "skel_attach"))
> -		goto out;
> -
> -	nstoken = create_netns();
> -	if (!ASSERT_OK_PTR(nstoken, "create_netns"))
> -		goto fail;
> -
> -	run_mptcpify(cgroup_fd, mptcpify_skel);
> -
> -fail:
> -	cleanup_netns(nstoken);
> -	mptcpify__destroy(mptcpify_skel);
> -out:
> -	close(cgroup_fd);
> -}
> -
> static const unsigned int total_bytes = 10 * 1024 * 1024;
> #define ADDR_1	"10.0.1.1"
> #define ADDR_2	"10.0.1.2"
> #define PORT_1	10001
>
> +#define MPTCP_BASE_TEST(name)					\

Hi Geliang -

I see why you're using a macro here, it's important to have the ASSERT 
macros report the right __func__ name. However I prefer to avoid a macro 
this complicated, since navigating multiple layers of macros when trying 
to debug a failure gets confusing. The boilerplate code is tempting to 
factor out, but for the test code here I don't think it's worth it.

If there are other discussion threads about this refactoring that I missed 
please give me a lore URL so I can better understand the rationale.

Thanks,
Mat

> +static void test_##name(void)					\
> +{								\
> +	struct nstoken *nstoken;				\
> +	int cgroup_fd, err;					\
> +	struct name *skel;					\
> +								\
> +	cgroup_fd = test__join_cgroup("/" #name);		\
> +	if (!ASSERT_GE(cgroup_fd, 0, "join_cgroup " #name))	\
> +		return;						\
> +								\
> +	skel = name##__open_and_load();				\
> +	if (!ASSERT_OK_PTR(skel, "skel_open_load " #name))	\
> +		goto close_cgroup;				\
> +								\
> +	err = name##__attach(skel);				\
> +	if (!ASSERT_OK(err, "skel_attach " #name))		\
> +		goto skel_destroy;				\
> +								\
> +	nstoken = create_netns();				\
> +	if (!ASSERT_OK_PTR(nstoken, "create_netns " #name))	\
> +		goto skel_destroy;				\
> +								\
> +	run_##name(cgroup_fd, skel);				\
> +								\
> +	cleanup_netns(nstoken);					\
> +skel_destroy:							\
> +	name##__destroy(skel);					\
> +close_cgroup:							\
> +	close(cgroup_fd);					\
> +}
> +
> +MPTCP_BASE_TEST(mptcp_sock);
> +MPTCP_BASE_TEST(mptcpify);
> +
> static struct nstoken *sched_init(char *flags, char *sched)
> {
> 	struct nstoken *nstoken;
> -- 
> 2.40.1
>
>
>