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-10 at 19:00 +0200, Matthieu Baerts wrote:
> Hi Geliang,
> 
> On 04/09/2025 10:26, Geliang Tang wrote:
> > 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?
> 
> (sorry, I thought I replied...)
> 
> Yes, that looks more manageable like that: all close() by the
> "function
> owner" (where they have been opened).
> 
> Also, please don't bother about closing all FD in the error exit
> paths,
> e.g. in xfork(), etc. when xerror() is called. These are more
> unexpected
> errors, and closing everything properly is this case, for the
> selftests
> is not a priority, and might cause issues if the code is more complex
> (+
> issues for the backports, etc.)

I agree, the priority is really low. I think there is no need to
backport these three patches. Should I change them to -next from -net
in v3 and treat them as cleanups instead of fixes?

Thanks,
-Geliang

> 
> Cheers,
> Matt