Re: [PATCH mptcp-net v2 2/5] selftests: mptcp: connect: catch IO errors on listen side

[Matthieu Baerts <matttbe@kernel.org>]

Hi Geliang,

On 06/09/2025 01:49, Geliang Tang wrote:
> On Fri, 2025-09-05 at 20:18 +0200, Matthieu Baerts (NGI0) wrote:
>> IO errors were correctly printed to stderr, and propagated up to the
>> main loop for the server side, but the returned value was ignored. As
>> a
>> consequence, the program for the listener side was no longer exiting
>> with an error code in case of IO issues.
>>
>> Because of that, some issues might not have been seen. But very
>> likely,
>> most issues either had an effect on the client side, or the file
>> transfer was not the expected one, e.g. the connection got reset
>> before
>> the end. Still, it is better to fix this.
>>
>> The main consequence of this issue is the error that was reported by
>> the
>> selftests: the received and sent files were different, and the MIB
>> counters were not printed. Also, when such errors happened during the
>> 'disconnect' tests, the program tried to continue until the timeout.
>>
>> Now when an IO error is detected, the program exits directly with an
>> error.
>>
>> Fixes: 05be5e273c84 ("selftests: mptcp: add disconnect tests")
>> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
>> ---
>>  tools/testing/selftests/net/mptcp/mptcp_connect.c | 6 +++++-
>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/tools/testing/selftests/net/mptcp/mptcp_connect.c
>> b/tools/testing/selftests/net/mptcp/mptcp_connect.c
>> index
>> 4f07ac9fa207cb08a934582b98d688d0b9512f97..c1586a7286123a509495ac319fd
>> 624b98f0bfd18 100644
>> --- a/tools/testing/selftests/net/mptcp/mptcp_connect.c
>> +++ b/tools/testing/selftests/net/mptcp/mptcp_connect.c
>> @@ -1112,6 +1112,8 @@ int main_loop_s(int listensock)
>>  	salen = sizeof(ss);
>>  	remotesock = accept(listensock, (struct sockaddr *)&ss,
>> &salen);
>>  	if (remotesock >= 0) {
>> +		int err;
>> +
>>  		maybe_close(listensock);
>>  		check_sockaddr(pf, &ss, salen);
>>  		check_getpeername(remotesock, &ss, salen);
>> @@ -1125,7 +1127,9 @@ int main_loop_s(int listensock)
>>  		SOCK_TEST_TCPULP(remotesock, 0);
>>  
>>  		memset(&winfo, 0, sizeof(winfo));
>> -		copyfd_io(fd, remotesock, 1, true, &winfo);
>> +		err = copyfd_io(fd, remotesock, 1, true, &winfo);
>> +		if (err)
>> +			return err;
> 
> The file descriptor (fd) should be closed before returning.

I do agree it would be better to do so, but when you look in this file,
most errors paths don't close the FD, because that's the exit path.

'fd' will need to be closed, 'remotesock' as well, but same when it was
not possible to create 'fd'. Also, I guess 'listensock' is never closed.
Honestly, I don't think we need to increase the complexity, and close
them, just for the tests. It would be different if the FD were reused
later on, but I don't think we need to spend time on these details when
this test program is about to close.

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