Re: [PATCH mptcp-next v1 2/2] selftests: mptcp: sockopt: add TCP_MAXSEG sockopt tests

[Matthieu Baerts <matttbe@kernel.org>]

Hi Geliang,

On 22/04/2025 11:30, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> This patch adds the TCP_MAXSEG sockopt tests in mptcp_sockopt.c. Since
> in getsockopt TCP_MAXSEG, the "user_mss" value can be obtained only in
> the LISTEN state (see do_tcp_getsockopt in net/ipv4/tcp.c), the test
> items are added to server() instead of client().

I think it is better to use packetdrill to do such validation, similar
to what was done with other socket options. No?

> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> ---
>  .../selftests/net/mptcp/mptcp_sockopt.c       | 22 +++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/tools/testing/selftests/net/mptcp/mptcp_sockopt.c b/tools/testing/selftests/net/mptcp/mptcp_sockopt.c
> index 926b0be87c99..e2043c0261bd 100644
> --- a/tools/testing/selftests/net/mptcp/mptcp_sockopt.c
> +++ b/tools/testing/selftests/net/mptcp/mptcp_sockopt.c
> @@ -689,6 +689,26 @@ static int xaccept(int s)
>  	return fd;
>  }
>  
> +static void test_tcp_maxseg_sockopt(int fd)
> +{
> +	int maxseg = 1000;
> +	socklen_t s;
> +	int r;
> +
> +	s = sizeof(maxseg);
> +	r = setsockopt(fd, IPPROTO_TCP, TCP_MAXSEG, &maxseg, s);
> +	if (r != 0)
> +		die_perror("setsockopt TCP_MAXSEG");
> +
> +	maxseg = 0;
> +	r = getsockopt(fd, IPPROTO_TCP, TCP_MAXSEG, &maxseg, &s);
> +	if (r != -1 && errno != EINVAL)
> +		die_perror("getsockopt TCP_MAXSEG did not indicate -EINVAL");
> +
> +	if (maxseg != 1000)
> +		xerror("maxseg=%d", maxseg);
> +}

Here, you set and get the value, but you don't really check the
behaviour is the one we expect.

It is important to check the behaviour. It would be easier to do that
with packetdrill I think.

> +
>  static int server(int pipefd)
>  {
>  	int fd = -1, r;
> @@ -713,6 +733,8 @@ static int server(int pipefd)
>  
>  	process_one_client(r, pipefd);
>  
> +	test_tcp_maxseg_sockopt(fd);
> +
>  	return 0;
>  }
>  

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.