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

[Matthieu Baerts <matttbe@kernel.org>]

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?

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