Re: [PATCH mptcp-net v2 6/6] selftests: mptcp: connect: trunc: read all recv data

[Geliang Tang <geliang@kernel.org>]

Hi Matt,

On Mon, 2025-11-03 at 13:12 +0100, Matthieu Baerts wrote:
> Hi Geliang,
> 
> On 03/11/2025 08:13, Geliang Tang wrote:
> > Hi Matt,
> > 
> > On Sun, 2025-11-02 at 12:30 +0100, Matthieu Baerts (NGI0) wrote:
> > > MPTCP Join "fastclose server" selftest is sometimes failing
> > > because
> > > the
> > > client output file doesn't have the expected size, e.g. 296B
> > > instead
> > > of
> > > 1024B.
> > 
> > Thanks for this fix. After repeatedly running the fastclose tests,
> > patches 5 and 6 have indeed eliminated the flaky failures.
> 
> Thank you for the tests.
> 
> > > When looking at a packet trace when this happens, the server sent
> > > the
> > > expected 1024B in two parts -- 100B, then 924B -- then the
> > > MP_FASTCLOSE.
> > > It is then strange to see the client only receiving 296B, which
> > > would
> > > mean it only got a part of the second packet. The problem is then
> > > not
> > > on
> > > the networking side, but rather on the data reception side.
> > > 
> > > When mptcp_connect is launched with '-f -1', it means the
> > > connection
> > > might stop before having sent everything, because a reset has
> > > been
> > > received. When this happens, the program was directly stopped.
> > > But it
> > > is
> > > also possible there are still some data to read, simply because
> > > the
> > > previous 'read' step was done with a buffer smaller than the
> > > pending
> > > data, see do_rnd_read(). In this case, it is important to read
> > > what's
> > > left in the kernel buffers before stopping without error like
> > > before.
> > > 
> > > SIGPIPE is now ignored, not to quit the app before having read
> > > everything.
> > > 
> > > Fixes: 6bf41020b72b ("selftests: mptcp: update and extend
> > > fastclose
> > > test-cases")
> > > Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> > > ---
> > > v2:
> > >  - Also catch EPIPE, and ignore SIGPIPE
> > > ---
> > >  tools/testing/selftests/net/mptcp/mptcp_connect.c | 18
> > > +++++++++++++-----
> > >  1 file changed, 13 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/tools/testing/selftests/net/mptcp/mptcp_connect.c
> > > b/tools/testing/selftests/net/mptcp/mptcp_connect.c
> > > index c030b08a7195..404a77bf366a 100644
> > > --- a/tools/testing/selftests/net/mptcp/mptcp_connect.c
> > > +++ b/tools/testing/selftests/net/mptcp/mptcp_connect.c
> > > @@ -710,8 +710,14 @@ static int copyfd_io_poll(int infd, int
> > > peerfd,
> > > int outfd,
> > >  
> > >  				bw = do_rnd_write(peerfd, winfo-
> > > >buf
> > > + winfo->off, winfo->len);
> > >  				if (bw < 0) {
> > > -					if (cfg_rcv_trunc)
> > > -						return 0;
> > > +					/* expected reset,
> > > continue
> > > to read */
> > > +					if (cfg_rcv_trunc &&
> > > +					    (errno == ECONNRESET
> > > ||
> > > +					     errno == EPIPE)) {
> > > +						fds.events &=
> > > ~POLLOUT;
> > > +						continue;
> > > +					}
> > 
> > We're adjusting the behavior of the falseclose tests here and need
> > to
> > ensure it doesn't affect other tests. I'm considering adding a new
> > config like cfg_rcv_ign to encapsulate these changes within an if
> > (cfg_rcv_ign) block, and only enable this new config in the
> > falseclose
> > tests.
> > 
> > WDYT?
> 
> I don't think we need an extra option. For the moment, when one side
> truncates the output ("-f XXX", which will cause a fastclose), the
> other
> side will be launched with "-f -1". In this case, "cfg_rcv_trunc"
> will
> be set, and we only change the behaviour for that.
> 
> It looks like this "-f" option is only use for the fastclose tests,
> and
> both peers have a "-f". So we can keep this as it is, no?

Yes, you're right. Let's keep this patch as is.

Reviewed-by: Geliang Tang <geliang@kernel.org>

Thanks,
-Geliang

> 
> Cheers,
> Matt