On Tue, May 7, 2024 at 9:02 AM Matthieu Baerts matttbe@kernel.org wrote:
Hi Alexei,
Thank you for the review!
On 07/05/2024 16:44, Alexei Starovoitov wrote:
On Tue, May 7, 2024 at 3:53 AM Matthieu Baerts (NGI0) matttbe@kernel.org wrote:
From: Geliang Tang tanggeliang@kylinos.cn
Each MPTCP subtest tests test__start_subtest(suffix), then invokes test_suffix(). It makes sense to add a new macro RUN_MPTCP_TEST to simpolify the code.
Signed-off-by: Geliang Tang tanggeliang@kylinos.cn Reviewed-by: Mat Martineau martineau@kernel.org Signed-off-by: Matthieu Baerts (NGI0) matttbe@kernel.org
tools/testing/selftests/bpf/prog_tests/mptcp.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/tools/testing/selftests/bpf/prog_tests/mptcp.c b/tools/testing/selftests/bpf/prog_tests/mptcp.c index baf976a7a1dd..9d1b255bb654 100644 --- a/tools/testing/selftests/bpf/prog_tests/mptcp.c +++ b/tools/testing/selftests/bpf/prog_tests/mptcp.c @@ -347,10 +347,14 @@ static void test_mptcpify(void) close(cgroup_fd); }
+#define RUN_MPTCP_TEST(suffix) \ +do { \
if (test__start_subtest(#suffix)) \test_##suffix(); \+} while (0)
Please no. Don't hide it behind macros.
I understand, I'm personally not a big fan of hiding code being a macro too. This one saves only one line. Geliang added a few more tests in our tree [1], for a total of 9, so that's only saving 9 lines.
Related to that, if you don't mind, Geliang also added another macro -- MPTCP_SCHED_TEST -- for tests that are currently only in our tree [2] (not ready yet). We asked him to reduce the size of this macro to the minimum. We accepted it because it removed quite a lot of similar code with very small differences [3]. Do you think we should revert this modification too?
Yeah. Pls don't hide such things in macros. Refactor into helper function in normal C.
But, what do you mean "in your tree" ? That's your development tree and you plan to send all that properly as patches to bpf-next someday?
[1] https://github.com/multipath-tcp/mptcp_net-next/blob/4369d9cbd752e166961ac0d...
[2] https://github.com/multipath-tcp/mptcp_net-next/blob/4369d9cbd752e166961ac0d...
[3] https://lore.kernel.org/mptcp/cover.1713321357.git.tanggeliang@kylinos.cn/T/...
Cheers, Matt -- Sponsored by the NGI0 Core fund.