Re: [PATCH bpf-next v2 2/2] selftests/bpf: Add pairs_redir_to_connected helper

[Martin KaFai Lau <martin.lau@xxxxxxxxx>]

On 10/5/23 12:21 AM, Geliang Tang wrote:
> --- a/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c
> +++ b/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c
> @@ -1336,32 +1336,22 @@ static void test_redir(struct test_sockmap_listen *skel, struct bpf_map *map,
>   	}
>   }
>   
> -static void unix_redir_to_connected(int sotype, int sock_mapfd,
> -			       int verd_mapfd, enum redir_mode mode)
> +static void pairs_redir_to_connected(int cli0, int peer0, int cli1, int peer1,
> +				     int sock_mapfd, int verd_mapfd, enum redir_mode mode)
>   {
>   	const char *log_prefix = redir_mode_str(mode);
> -	int c0, c1, p0, p1;
>   	unsigned int pass;
>   	int err, n;
> -	int sfd[2];
>   	u32 key;
>   	char b;
>   
>   	zero_verdict_count(verd_mapfd);
>   
> -	if (socketpair(AF_UNIX, sotype | SOCK_NONBLOCK, 0, sfd))
> -		return;
> -	c0 = sfd[0], p0 = sfd[1];
> -
> -	if (socketpair(AF_UNIX, sotype | SOCK_NONBLOCK, 0, sfd))
> -		goto close0;
> -	c1 = sfd[0], p1 = sfd[1];
> -
> -	err = add_to_sockmap(sock_mapfd, p0, p1);
> +	err = add_to_sockmap(sock_mapfd, peer0, peer1);
>   	if (err)
>   		goto close;
>   
> -	n = write(c1, "a", 1);
> +	n = write(cli1, "a", 1);
>   	if (n < 0)
>   		FAIL_ERRNO("%s: write", log_prefix);
>   	if (n == 0)
> @@ -1376,16 +1366,34 @@ static void unix_redir_to_connected(int sotype, int sock_mapfd,
>   	if (pass != 1)
>   		FAIL("%s: want pass count 1, have %d", log_prefix, pass);
>   
> -	n = recv_timeout(mode == REDIR_INGRESS ? p0 : c0, &b, 1, 0, IO_TIMEOUT_SEC);
> +	n = recv_timeout(mode == REDIR_INGRESS ? peer0 : cli0, &b, 1, 0, IO_TIMEOUT_SEC);
>   	if (n < 0)
>   		FAIL_ERRNO("%s: recv_timeout", log_prefix);
>   	if (n == 0)
>   		FAIL("%s: incomplete recv", log_prefix);
>   
>   close:
> -	xclose(c1);
> -	xclose(p1);
> -close0:
> +	xclose(cli1);
> +	xclose(peer1);
> +}
> +
> +static void unix_redir_to_connected(int sotype, int sock_mapfd,
> +			       int verd_mapfd, enum redir_mode mode)
> +{
> +	int c0, c1, p0, p1;
> +	int sfd[2];
> +
> +	if (socketpair(AF_UNIX, sotype | SOCK_NONBLOCK, 0, sfd))
> +		return;
> +	c0 = sfd[0], p0 = sfd[1];
> +
> +	if (socketpair(AF_UNIX, sotype | SOCK_NONBLOCK, 0, sfd))
> +		goto close;
> +	c1 = sfd[0], p1 = sfd[1];
> +
> +	pairs_redir_to_connected(c0, p0, c1, p1, sock_mapfd, verd_mapfd, mode);
> +
> +close:
>   	xclose(c0);
>   	xclose(p0);
>   }
> @@ -1661,51 +1669,19 @@ static int inet_socketpair(int family, int type, int *s, int *c)
>   static void udp_redir_to_connected(int family, int sock_mapfd, int verd_mapfd,
>   				   enum redir_mode mode)
>   {
> -	const char *log_prefix = redir_mode_str(mode);
>   	int c0, c1, p0, p1;
> -	unsigned int pass;
> -	int err, n;
> -	u32 key;
> -	char b;
> -
> -	zero_verdict_count(verd_mapfd);
> +	int err;
>   
>   	err = inet_socketpair(family, SOCK_DGRAM, &p0, &c0);
>   	if (err)
>   		return;
>   	err = inet_socketpair(family, SOCK_DGRAM, &p1, &c1);
>   	if (err)
> -		goto close_cli0;
> -
> -	err = add_to_sockmap(sock_mapfd, p0, p1);
> -	if (err)
> -		goto close_cli1;
> -
> -	n = write(c1, "a", 1);
> -	if (n < 0)
> -		FAIL_ERRNO("%s: write", log_prefix);
> -	if (n == 0)
> -		FAIL("%s: incomplete write", log_prefix);
> -	if (n < 1)
> -		goto close_cli1;
> -
> -	key = SK_PASS;
> -	err = xbpf_map_lookup_elem(verd_mapfd, &key, &pass);
> -	if (err)
> -		goto close_cli1;
> -	if (pass != 1)
> -		FAIL("%s: want pass count 1, have %d", log_prefix, pass);
> +		goto close;
>   
> -	n = recv_timeout(mode == REDIR_INGRESS ? p0 : c0, &b, 1, 0, IO_TIMEOUT_SEC);
> -	if (n < 0)
> -		FAIL_ERRNO("%s: recv_timeout", log_prefix);
> -	if (n == 0)
> -		FAIL("%s: incomplete recv", log_prefix);
> +	pairs_redir_to_connected(c0, p0, c1, p1, sock_mapfd, verd_mapfd, mode);
>   
> -close_cli1:
> -	xclose(c1);
> -	xclose(p1);
> -close_cli0:
> +close:
>   	xclose(c0);
>   	xclose(p0);

Patch 1 is applied. Thanks.

In patch 2, the xclose() here is confusing after this change. It is also 
inconsistent from how other tests in sockmap_listen.c is doing it. c0/p0 and 
c1/p1 are opened here but only c0/p0 is closed here and c1/p1 is closed in the 
pairs_redir_to_connected() above instead.