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

[Geliang Tang <geliang@kernel.org>]

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