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

[Matthieu Baerts <matttbe@kernel.org>]

Hi Geliang,

On 11/09/2025 12:05, Geliang Tang wrote:
> On Thu, 2025-09-11 at 11:35 +0200, Matthieu Baerts wrote:
>> Hi Geliang,
>>
>> On 11/09/2025 11:04, Geliang Tang wrote:
>>> 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..d4981b76693bbddca74
>>>>>> 1694
>>>>>> 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:::f
>>> fff:
>>> 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.
>>
>> I hope no applications are doing that: this pm_nl_ctl tool lists the
> 
> I do use this in BPF path manager selftests under development.

Probably best to avoid that. Can you not split based on ',' (strtok*())
and take the values for the keys you are interested in?

>> arguments in the order it receives it, but this order is set by the
>> kernel and can change. In other words, this order is not fixed in
>> stone.
>>
>> Plus this tool is for the selftests, I don't think other people are
>> using it. 'ip mptcp' is probably used instead. And even they should
>> not
>> expect the order to be fixed.
>>
>> So I don't think we need to increase the complexity here if it is not
>> needed.
> 
> Sure. I have no comments then. Please add my tag for this set:
> 
> Reviewed-by: Geliang Tang <geliang@kernel.org>

Thanks! (Please next time send this as a reply to the cover-letter, so
the tag will be automatically propagated to each patch.)

Now in our tree (feat. for net-next):

New patches for t/upstream:
- 419c5f213113: mptcp: pm: netlink: only add server-side attr when true
- 7a32b2ee28a7: mptcp: pm: netlink: announce server-side flag
- ed9ca7fac132: mptcp: pm: netlink: deprecate server-side attribute
- 65cb7a9d4d5e: selftests: mptcp: pm: get server-side flag
- Results: 9d5b2844a3ca..7b11ba96fa1d (export)

Tests are now in progress:

- export:
https://github.com/multipath-tcp/mptcp_net-next/commit/d1dac2258ca371e5cfcd46487e28590da3162f9b/checks

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.