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

[Matthieu Baerts <matttbe@kernel.org>]

Hi Geliang,

On 10/09/2025 04:44, Geliang Tang wrote:
> Hi Matt,
> 
> On Sat, 2025-09-06 at 15:54 +0200, Matthieu Baerts wrote:
>> 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..c1586a7286123a509495ac3
>>>> 19fd
>>>> 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.
> 
> What I mean here is not to return directly, something like:
> 
> '''
> int err = 0;
> 
> if (remotesock >= 0) {
>     ... ...
>     err = copyfd_io(fd, remotesock, 1, true, &winfo);
> }
> 
> if (cfg_input)
>     close(fd);
> 
> if (!err && --cfg_repeat > 0)
>     goto again;
> 
> return err;
> '''
> 
> WDYT?

I do agree it is better. But again, I think there is no need to do that
in this patch, because most error exit paths in this file don't close
the fd's. That makes the patch longer and slightly more complex -- which
can cause more troubles during the backports -- just for the exit error
path. But because I keep being reminded by net maintainers that this
series is needed to fix CI noises (and real issues), I want to move on,
and I'm going to send a v3 with this modification.

> I will send a patch to close remotesock and listensock later.
> 
> Thanks,
> -Geliang
> 
>>
>> Cheers,
>> Matt

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