Re: [RFC bpf-next v2 8/8] selftests/bpf: Add mptcpify selftest

[Geliang Tang <geliang.tang@suse.com>]

On Thu, Jul 06, 2023 at 10:21:47AM +0200, Matthieu Baerts wrote:
> Hi Geliang,
> 
> (without BPF ML not to disturb them for this detail)
> 
> On 06/07/2023 09:47, Geliang Tang wrote:
> > This patch extends the MPTCP test base, add a selftest test_mptcpify()
> > for the mptcpify case.
> > 
> > Open and load the mptcpify test prog to mptcpify the TCP sockets
> > dynamically, then use start_server() and connect_to_fd() to create a
> > TCP socket, but actually what's created is an MPTCP socket, which can
> > be verified through the output of 'ss' command.
> > 
> > Signed-off-by: Geliang Tang <geliang.tang@suse.com>
> > ---
> >  .../testing/selftests/bpf/prog_tests/mptcp.c  | 91 +++++++++++++++++++
> >  1 file changed, 91 insertions(+)
> > 
> > diff --git a/tools/testing/selftests/bpf/prog_tests/mptcp.c b/tools/testing/selftests/bpf/prog_tests/mptcp.c
> > index b2a833a900c2..de05140fe638 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/mptcp.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/mptcp.c
> 
> (...)
> 
> > +static int verify_mptcpify(void)
> > +{
> > +	char cmd[128];
> > +	int err = 0;
> > +
> > +	snprintf(cmd, sizeof(cmd),
> > +		 "ip netns exec %s ss -tOni | grep -q '%s'",
> > +		 NS_TEST, "tcp-ulp-mptcp");
> > +	if (!ASSERT_OK(system(cmd), "No tcp-ulp-mptcp found!"))
> > +		err++;
> > +
> > +	snprintf(cmd, sizeof(cmd),
> > +		 "ip netns exec %s nstat -asz %s | awk '%s' | grep -q '%s'",
> > +		 NS_TEST, "MPTcpExtMPCapableSYNACKRX",
> > +		 "NR==1 {next} {print $2}", "1");
> 
> It might be safer to increase cmd size to 256 (or even 1024) because if
> I'm not mistaken, you might need here 129 chars if NS_TEST can go up to
> 31 chars:
> 
> > ip netns exec 1234567890123456789012345678901 nstat -asz MPTcpExtMPCapableSYNACKRX | awk 'NR==1 {next} {print $2}' | grep -q '1'
> 
> That's 128 chars but without '\0'.

Yes, I had changed this size to 256 and sent out the v3 before reading
your letter :)

Now the tests for v3 all passed. Let's wait for the reply.

Thanks,
-Geliang

> 
> But I don't think you need a v3 now: it is an issue just with the tests
> and also because I know most subsystems ask not to send more than one
> version per 24h ;)
> 
> BTW, excellent cover-letter, thanks for that!
> 
> > +	if (!ASSERT_OK(system(cmd), "No MPTcpExtMPCapableSYNACKRX found!"))
> > +		err++;
> > +
> > +	return err;
> > +}
> 
> Cheers,
> Matt
> -- 
> Tessares | Belgium | Hybrid Access Solutions
> www.tessares.net