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

[Geliang Tang <geliang@kernel.org>]

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 will send a patch to close remotesock and listensock later.

Thanks,
-Geliang

> 
> Cheers,
> Matt