Re: [PATCH mptcp-next 4/4] selftests: mptcp: pm: get server-side flag

[Geliang Tang <geliang@kernel.org>]

Hi Matt,

On Thu, 2025-09-11 at 10:35 +0200, Matthieu Baerts wrote:
> Hi Geliang,
> 
> On 11/09/2025 10:16, Geliang Tang wrote:
> > Hi Matt,
> > 
> > On Tue, 2025-09-09 at 18:30 +0200, Matthieu Baerts (NGI0) wrote:
> > > server-side info linked to the MPTCP connect/established events
> > > can
> > > now
> > > come from the flags, in addition to the dedicated attribute.
> > > 
> > > The attribute is now deprecated -- in favour of the new flag, and
> > > will
> > > be removed later on.
> > > 
> > > Print this info only once.
> > > 
> > > Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> > > ---
> > >  tools/testing/selftests/net/mptcp/pm_nl_ctl.c | 9 ++++++++-
> > >  1 file changed, 8 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/tools/testing/selftests/net/mptcp/pm_nl_ctl.c
> > > b/tools/testing/selftests/net/mptcp/pm_nl_ctl.c
> > > index
> > > 93fea3442216c8fef43731a99c1d5710f234b150..d4981b76693bbddca741694
> > > 37a5
> > > 40ad6294cf1d5 100644
> > > --- a/tools/testing/selftests/net/mptcp/pm_nl_ctl.c
> > > +++ b/tools/testing/selftests/net/mptcp/pm_nl_ctl.c
> > > @@ -2,6 +2,7 @@
> > >  
> > >  #include <errno.h>
> > >  #include <error.h>
> > > +#include <stdbool.h>
> > >  #include <stdio.h>
> > >  #include <stdlib.h>
> > >  #include <string.h>
> > > @@ -113,6 +114,8 @@ static int capture_events(int fd, int
> > > event_group)
> > >  		error(1, errno, "could not join the "
> > > MPTCP_PM_EV_GRP_NAME " mcast group");
> > >  
> > >  	do {
> > > +		bool server_side = false;
> > > +
> > >  		FD_ZERO(&rfds);
> > >  		FD_SET(fd, &rfds);
> > >  		res_len = NLMSG_ALIGN(sizeof(struct nlmsghdr)) +
> > > @@ -187,18 +190,22 @@ static int capture_events(int fd, int
> > > event_group)
> > >  				else if (attrs->rta_type ==
> > > MPTCP_ATTR_ERROR)
> > >  					fprintf(stderr,
> > > ",error:%u",
> > > *(__u8 *)RTA_DATA(attrs));
> > >  				else if (attrs->rta_type ==
> > > MPTCP_ATTR_SERVER_SIDE)
> > > -					fprintf(stderr,
> > > ",server_side:%u", *(__u8 *)RTA_DATA(attrs));
> > 
> > It is a little strange that server_side is displayed at the end. It
> > changes the original display order.  It is better to display it in
> > the
> > original position, but check whether the server_side flag is false:
> 
> I'm not sure to understand why it is strange: the output is read by
> scripts, the order should not matter, right?

I mean the original display order of "./pm_nl_ctl events" is:

type:1,token:2327950928,server_side:1,family:10,saddr6:::ffff:10.0.1.1,
daddr6:::ffff:10.0.1.2,sport:10000,dport:40004,loc_id:0,rem_id:0

With this patch, the order is changed:

type:1,token:1072969578,family:10,saddr6:::ffff:10.0.1.1,daddr6:::ffff:
10.0.1.2,sport:10000,dport:42484,loc_id:0,rem_id:0,server_side:1

If an application still uses code like this to parse server_side, an
error will occur.

sscanf(str, "type:%u,token:%u,server_side:%u",
       &type, &token, &server_side);

So it makes sense to keep the original order. But up to you.

Thanks,
-Geliang

> 
> Plus the order depends on what was received by the kernel, not where
> they are printed here.
> 
> Do you really think it is worth it changing this patch and making
> longer
> just to keep an order that we don't currently fully control here?
> 
> Cheers,
> Matt