Re: [PATCH bpf-next v6 3/9] selftests/bpf: Use connect_to_fd in sockmap_ktls

[Martin KaFai Lau <martin.lau@linux.dev>]

On 6/28/24 6:20 PM, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> Use public network helper connect_to_fd() instead of open-coding it in
> prog_tests/sockmap_ktls.c. This can avoid duplicate code.
> 
> Acked-by: Eduard Zingerman <eddyz87@gmail.com>
> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> ---
>   .../selftests/bpf/prog_tests/sockmap_ktls.c        | 14 ++------------
>   1 file changed, 2 insertions(+), 12 deletions(-)
> 
> diff --git a/tools/testing/selftests/bpf/prog_tests/sockmap_ktls.c b/tools/testing/selftests/bpf/prog_tests/sockmap_ktls.c
> index 4dc7933bb556..a6b0ed633505 100644
> --- a/tools/testing/selftests/bpf/prog_tests/sockmap_ktls.c
> +++ b/tools/testing/selftests/bpf/prog_tests/sockmap_ktls.c
> @@ -24,26 +24,16 @@ static void test_sockmap_ktls_disconnect_after_delete(int family, int map)
>   	struct network_helper_opts opts = {
>   		.backlog = SOMAXCONN,
>   	};
> -	struct sockaddr_storage addr = {0};
> -	socklen_t len = sizeof(addr);
>   	int err, cli, srv, zero = 0;
>   
>   	srv = start_server_str(family, SOCK_STREAM, NULL, 0, &opts);
>   	if (srv == -1)
>   		return;
>   
> -	err = getsockname(srv, (struct sockaddr *)&addr, &len);
> -	if (!ASSERT_OK(err, "getsockopt"))
> -		goto close_srv;
> -
> -	cli = socket(family, SOCK_STREAM, 0);
> -	if (!ASSERT_GE(cli, 0, "socket"))
> +	cli = connect_to_fd(srv, 0);

This patch looks good.

I would go for the newer connect_to_fd_opts() and stop using the older 
connect_to_fd() in the new code. I just notice there is a newly added "int type" 
argument in the connect_to_fd_opts(). I don't see the "int type" argument is 
useful after removing the connect_to_fd_opts() usage from ip_check_defrag.c. 
This cleanup can be left to a separate patch set. The current patch can stay 
with the older connect_to_fd() for now.

> +	if (!ASSERT_GE(cli, 0, "connect_to_fd"))
>   		goto close_srv;
>   
> -	err = connect(cli, (struct sockaddr *)&addr, len);
> -	if (!ASSERT_OK(err, "connect"))
> -		goto close_cli;
> -
>   	err = bpf_map_update_elem(map, &zero, &cli, 0);
>   	if (!ASSERT_OK(err, "bpf_map_update_elem"))
>   		goto close_cli;