Re: [PATCH mptcp-net v2 2/3] selftests: mptcp: close IPC descriptor on server side

[Geliang Tang <geliang@kernel.org>]

Hi Matt,

On Wed, 2025-09-03 at 13:57 +0200, Matthieu Baerts wrote:
> Hi Geliang,
> 
> On 03/09/2025 06:08, Geliang Tang wrote:
> > From: Geliang Tang <tanggeliang@kylinos.cn>
> > 
> > The client-side function 'connect_one_server()' correctly closes
> > the IPC
> > descriptor (a pipe or UNIX socket) after use. However, the server-
> > side
> > functions 'process_one_client()' in both 'mptcp_sockopt.c' and
> > 'mptcp_inq.c'
> > were missing the corresponding 'close()' call for their IPC
> > descriptors.
> > 
> > This omission could lead to resource leaks (file descriptors) in
> > the test
> > server processes over time.
> > 
> > This patch adds the missing 'close(pipefd)' and 'close(unixfd)'
> > calls in the
> > server-side code, ensuring symmetric and correct resource cleanup.
> 
> I don't know if we need such patch. I mean: yes, that's better to
> close
> such FD before closing the application, but then:
> 
> - it is strange to close it in the middle of a function
> (process_one_client()), and not where it has been created (main())
> 

Yes, close it in main() is much better. I added close(pipefds[1]) after
calling server(pipefds[1]) in the new version instead of adding
close(pipefd) in process_one_client().

> - if I'm not mistaken, unixfds[0] / pipefds[0] are not closed in the
> server process.

Yes, indeed. close(pipefds[0]) is missing in the server process too.

> So if you really want to fix that, I think it would be better to
> close
> all these unix FD in the 'main()' function, no?

I also moved 'close(pipefd)' from connect_one_server() to main(), just
after calling client(pipefds[0]). And here's the new version:

        e1 = pipe(pipefds);
        if (e1 < 0)
                die_perror("pipe");

        s = xfork();
        if (s == 0) {
                close(pipefds[0]);
                ret = server(pipefds[1]);
                close(pipefds[1]);
                return ret;
        }

        close(pipefds[1]);

        /* wait until server bound a socket */
        e1 = read(pipefds[0], &e1, 4); 
        assert(e1 == 4); 

        c = xfork();
        if (c == 0) {
                ret = client(pipefds[0]);
                close(pipefds[0]);
                return ret;
        }

        close(pipefds[0]);

Is this better?

Thanks,
-Geliang

> 
> Cheers,
> Matt