Re: [PATCH mptcp-next v3] selftests: mptcp: print stats before socket closure

[Geliang Tang <geliang@kernel.org>]

Hi Matt,

On Thu, 2026-10-22 at 09:58 +0200, Matthieu Baerts wrote:
> Hi Geliang,
> 
> Thank you for your patience.
> 
> On 21/09/2026 09:52, Geliang Tang wrote:
> > Hi Matt,
> > 
> > Sorry for the delayed reply.
> > 
> > On Tue, 2026-09-08 at 19:52 +0200, Matthieu Baerts wrote:
> > > Hi Geliang,
> > > 
> > > On 05/09/2026 10:26, Geliang Tang wrote:
> > > > Hi Matt,
> > > > 
> > > > On Sat, 2026-08-15 at 00:28 +0200, Matthieu Baerts (NGI0)
> > > > wrote:
> > > > > In case of poll timeout, it might be too late to print the
> > > > > stats
> > > > > after
> > > > > the socket closure.
> > > > > 
> > > > > Now, in case of poll timeout, 'ss' and 'nstat' are invoked
> > > > > from
> > > > > mptcp_connect to print the stats before exiting. This should
> > > > > help
> > > > > debugging poll timeout issues.
> > > > > 
> > > > > Note: for this "workaround", system() is used for debugging
> > > > > purposes
> > > > 
> > > > This print_err_stats() implementation is basically the same as
> > > > mptcp_lib_pr_err_stats() in the shell, right?
> > > 
> > > Correct.
> > > 
> > > > I think it's better to use mptcp_lib_pr_err_stats() in the
> > > > shell to
> > > > print the information. In mptcp_connect.c, when a poll timeout
> > > > occurs,
> > > > we send a signal to the shell and don't close the socket
> > > > immediately -
> > > > we wait for the shell to finish printing the information before
> > > > closing
> > > > it. What do you think of this approach? There's a reference
> > > > implementation in the attachment, but I haven't tested it.
> > > Thank you for your review. I agree that what you suggested is
> > > better
> > > for
> > > a tool that would be used in production. But here with the
> > > selftests,
> > > I
> > > don't think we need your long patch with a higher complexity,
> > > just
> > > for
> > > the selftests. I think it is fine to have some "system()" calls
> > > with
> > > the
> > > same ss/nstat commands compared to a patch of 50+ lines to get
> > > the
> > > same
> > > result. No?
> > 
> > I agree with you - simplicity is a plus as well.
> > 
> > I tested it and it works. So LGTM!
> > 
> >     Reviewed-by: Geliang Tang <geliang@kernel.org>
> > 
> > 
> > However, we need to handle something in mptcp_connect.sh
> > afterwards.
> > Otherwise, when a timeout occurs, we'll get two duplicate outputs:
> > one
> > from mptcp_connect, which is the log added by this patch, and one
> > from
> > mptcp_connect.sh, which is an invalid log. I think in this case we
> > should ignore the latter, for example by adding the following to
> > mptcp_connect.sh:
> > 
> > do_transfer()
> > {
> >   ...
> >   mptcp_lib_pr_fail "client exit code $retc, server $rets"
> >   if [ ${rets} -ne 2 ] && [ ${retc} -ne 2 ]; then
> >     mptcp_lib_pr_err_stats "${listener_ns}" "${connector_ns}"
> > "${port}"
> >   fi
> >   ...
> > }
> > 
> > What do you think? If you don't have time, I can submit this
> > follow-up
> > patch as well.
> 
> Is it really an issue to have two stats outputs? They are not
> duplicated: one is explicitly printing stats before socket closure,
> and
> the other one after.
> 
> Ignoring the 2nd output is easy, but this output can be valuable?
> Also,
> I don't think there will be a timeout issue on both the client and
> the
> server. Likely only one of them will timeout, and the additional
> check
> you suggest is not needed.
> 
> So OK like that?

OK, let's keep this patch as is. Please merge it:

Reviewed-by: Geliang Tang <geliang@kernel.org>

Thanks,
-Geliang

> 
> Cheers,
> Matt