Re: [PATCH net-next] selftests: add selftest for UDP SO_PEEK_OFF support

[Willem de Bruijn <willemdebruijn.kernel@gmail.com>]

Jason Xing wrote:
> From: Jason Xing <kernelxing@tencent.com>
> 
> Add the SO_PEEK_OFF selftest for UDP. In this patch, I mainly do
> three things:
> 1. rename tcp_so_peek_off.c
> 2. adjust for UDP protocol
> 3. add selftests into it
> 
> Suggested-by: Jon Maloy <jmaloy@redhat.com>
> Signed-off-by: Jason Xing <kernelxing@tencent.com>

A few minor comments. Nothing important.

Subject to Stan's point about .gitignore:

Reviewed-by: Willem de Bruijn <willemb@google.com>

> -int tcp_peek_offset_probe(sa_family_t af)
> +int sk_peek_offset_probe(sa_family_t af, int proto)
>  {
> +	int type = (proto == IPPROTO_TCP ? SOCK_STREAM : SOCK_DGRAM);
>  	int optv = 0;
>  	int ret = 0;
>  	int s;
>  
> -	s = socket(af, SOCK_STREAM | SOCK_CLOEXEC, IPPROTO_TCP);
> +	s = socket(af, type, proto);

Removing the SOCK_CLOEXEC because not relevant to this single thread
process, I suppose?

Not important, but no need for proto, can just be 0.

>  	if (s < 0) {
>  		ksft_perror("Temporary TCP socket creation failed");
>  	} else {
>  		if (!setsockopt(s, SOL_SOCKET, SO_PEEK_OFF, &optv, sizeof(int)))
>  			ret = 1;
>  		else
> -			printf("%s does not support SO_PEEK_OFF\n", afstr(af));
> +			printf("%s does not support SO_PEEK_OFF\n", afstr(af, proto));
>  		close(s);
>  	}
>  	return ret;
>  }
>  
> -static void tcp_peek_offset_set(int s, int offset)
> +static void sk_peek_offset_set(int s, int offset)
>  {
>  	if (setsockopt(s, SOL_SOCKET, SO_PEEK_OFF, &offset, sizeof(offset)))
>  		ksft_perror("Failed to set SO_PEEK_OFF value\n");
>  }
>  
> -static int tcp_peek_offset_get(int s)
> +static int sk_peek_offset_get(int s)
>  {
>  	int offset;
>  	socklen_t len = sizeof(offset);
> @@ -50,8 +54,9 @@ static int tcp_peek_offset_get(int s)
>  	return offset;
>  }
>  
> -static int tcp_peek_offset_test(sa_family_t af)
> +static int sk_peek_offset_test(sa_family_t af, int proto)
>  {
> +	int type = (proto == IPPROTO_TCP ? SOCK_STREAM : SOCK_DGRAM);
>  	union {
>  		struct sockaddr sa;
>  		struct sockaddr_in a4;
> @@ -62,13 +67,13 @@ static int tcp_peek_offset_test(sa_family_t af)
>  	int recv_sock = 0;
>  	int offset = 0;
>  	ssize_t len;
> -	char buf;
> +	char buf[2];
>  
>  	memset(&a, 0, sizeof(a));
>  	a.sa.sa_family = af;
>  
> -	s[0] = socket(af, SOCK_STREAM, IPPROTO_TCP);
> -	s[1] = socket(af, SOCK_STREAM | SOCK_NONBLOCK, IPPROTO_TCP);
> +	s[0] = recv_sock = socket(af, type, proto);
> +	s[1] = socket(af, type, proto);

Same

>  
>  	if (s[0] < 0 || s[1] < 0) {
>  		ksft_perror("Temporary socket creation failed\n");
> @@ -82,76 +87,78 @@ static int tcp_peek_offset_test(sa_family_t af)
>  		ksft_perror("Temporary socket getsockname() failed\n");
>  		goto out;
>  	}
> -	if (listen(s[0], 0) < 0) {
> +	if (proto == IPPROTO_TCP && listen(s[0], 0) < 0) {
>  		ksft_perror("Temporary socket listen() failed\n");
>  		goto out;
>  	}
> -	if (connect(s[1], &a.sa, sizeof(a)) >= 0 || errno != EINPROGRESS) {
> +	if (connect(s[1], &a.sa, sizeof(a))) {
>  		ksft_perror("Temporary socket connect() failed\n");
>  		goto out;
>  	}

Changed due to the removal of SOCK_NONBLOCK above. Definitely
simplifies the test.

Just note that error test is == -1 or < 0, also for consistency with
the rest of the file.

> -	recv_sock = accept(s[0], NULL, NULL);
> -	if (recv_sock <= 0) {
> -		ksft_perror("Temporary socket accept() failed\n");
> -		goto out;
> +	if (proto == IPPROTO_TCP) {
> +		recv_sock = accept(s[0], NULL, NULL);
> +		if (recv_sock <= 0) {
> +			ksft_perror("Temporary socket accept() failed\n");
> +			goto out;
> +		}