[v1] Add TCP_MAXSEG socket option support

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

Posted by Geliang Tang 7 months, 3 weeks ago

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().

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);
+}
+
 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;
 }
 
-- 
2.43.0

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

Posted by Matthieu Baerts 7 months, 3 weeks ago

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.

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

Posted by Geliang Tang 7 months, 3 weeks ago

Hi Matt,

Thanks for the review.

> 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.

I totally agree, but I want to verify the set and get values in this
set first, I will add packetdrill tests for it later (once I learn how
to modify packetdrill).

Thanks,
-Geliang

> 
> > +
> >  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