Recently, I wanted to understand why the userspace_pm.sh test was
failing on some arch according to lkft reports. There was nothing in the
logs apart from seeing the expected event had been missing. I then
realised the output of pm_nl_ctl was muted but also this tool was not
looking if there were errors after having sent some Netlink commands.

I don't think it is a good idea to mute the output and ignore errors,
especially here in the tests where we do want to validate these Netlink
commands. So now the tool checks for errors and the output is no longer
discarded.

Note that I didn't manage to read the error message set by the kernel. I
think the rtattr pointer in nl_error() is wrong. I think it should get
info from &err->msg instead of nh. Anyway, here we at least print errno
and just in case someone wants to have a look, it is easy to test:

  ./pm_nl_ctl ann 1.1.1.1 token 12 id 0

Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net>
---
Matthieu Baerts (2):
      selftests: mptcp: pm_nl_ctl: always look for errors
      selftests: mptcp: userspace_pm: unmute unexpected errors

 tools/testing/selftests/net/mptcp/pm_nl_ctl.c     |  33 ++++---
 tools/testing/selftests/net/mptcp/userspace_pm.sh | 100 +++++++++++-----------
 2 files changed, 72 insertions(+), 61 deletions(-)
---
base-commit: cde4fa6a0a5ed3effb75008ce99eb842bc448e5d
change-id: 20230705-mptcp-issue-368-pm_nl_ctl_ack-bb1625f5bdda

Best regards,
-- 
Matthieu Baerts <matthieu.baerts@tessares.net>

On Wed, 5 Jul 2023, Matthieu Baerts wrote:

> Recently, I wanted to understand why the userspace_pm.sh test was
> failing on some arch according to lkft reports. There was nothing in the
> logs apart from seeing the expected event had been missing. I then
> realised the output of pm_nl_ctl was muted but also this tool was not
> looking if there were errors after having sent some Netlink commands.
>
> I don't think it is a good idea to mute the output and ignore errors,
> especially here in the tests where we do want to validate these Netlink
> commands. So now the tool checks for errors and the output is no longer
> discarded.
>
> Note that I didn't manage to read the error message set by the kernel. I
> think the rtattr pointer in nl_error() is wrong. I think it should get
> info from &err->msg instead of nh. Anyway, here we at least print errno
> and just in case someone wants to have a look, it is easy to test:
>
>  ./pm_nl_ctl ann 1.1.1.1 token 12 id 0
>
> Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net>


Hi Matthieu -

Good idea to add the relevant error output.

I noticed in the CI run for this patch set 
(https://cirrus-ci.com/task/5548960819970048) that some netlink error info 
is printed in the "OK" test results too:

[17:50:36.964] # flush addrs                                       [ OK ]
[17:50:37.070] # netlink error -22 (Invalid argument)
[17:50:37.076] # ./pm_nl_ctl: bailing out due to netlink error[s]
[17:50:37.187] # rcv addrs above hard limit                        [ OK ]
[17:50:37.295] # netlink error -22 (Invalid argument)
[17:50:37.300] # ./pm_nl_ctl: bailing out due to netlink error[s]
[17:50:37.413] # subflows above hard limit                         [ OK ]

Do you expect that with your changes?


- Mat


> ---
> Matthieu Baerts (2):
>      selftests: mptcp: pm_nl_ctl: always look for errors
>      selftests: mptcp: userspace_pm: unmute unexpected errors
>
> tools/testing/selftests/net/mptcp/pm_nl_ctl.c     |  33 ++++---
> tools/testing/selftests/net/mptcp/userspace_pm.sh | 100 +++++++++++-----------
> 2 files changed, 72 insertions(+), 61 deletions(-)
> ---
> base-commit: cde4fa6a0a5ed3effb75008ce99eb842bc448e5d
> change-id: 20230705-mptcp-issue-368-pm_nl_ctl_ack-bb1625f5bdda
>
> Best regards,
> -- 
> Matthieu Baerts <matthieu.baerts@tessares.net>
>
>
>

Hi Mat,

On 13/07/2023 19:17, Mat Martineau wrote:
> On Wed, 5 Jul 2023, Matthieu Baerts wrote:
> 
>> Recently, I wanted to understand why the userspace_pm.sh test was
>> failing on some arch according to lkft reports. There was nothing in the
>> logs apart from seeing the expected event had been missing. I then
>> realised the output of pm_nl_ctl was muted but also this tool was not
>> looking if there were errors after having sent some Netlink commands.
>>
>> I don't think it is a good idea to mute the output and ignore errors,
>> especially here in the tests where we do want to validate these Netlink
>> commands. So now the tool checks for errors and the output is no longer
>> discarded.
>>
>> Note that I didn't manage to read the error message set by the kernel. I
>> think the rtattr pointer in nl_error() is wrong. I think it should get
>> info from &err->msg instead of nh. Anyway, here we at least print errno
>> and just in case someone wants to have a look, it is easy to test:
>>
>>  ./pm_nl_ctl ann 1.1.1.1 token 12 id 0
>>
>> Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net>
> 
> 
> Hi Matthieu -
> 
> Good idea to add the relevant error output.
> 
> I noticed in the CI run for this patch set
> (https://cirrus-ci.com/task/5548960819970048) that some netlink error
> info is printed in the "OK" test results too:
> 
> [17:50:36.964] # flush addrs                                       [ OK ]
> [17:50:37.070] # netlink error -22 (Invalid argument)
> [17:50:37.076] # ./pm_nl_ctl: bailing out due to netlink error[s]
> [17:50:37.187] # rcv addrs above hard limit                        [ OK ]
> [17:50:37.295] # netlink error -22 (Invalid argument)
> [17:50:37.300] # ./pm_nl_ctl: bailing out due to netlink error[s]
> [17:50:37.413] # subflows above hard limit                         [ OK ]
> 
> Do you expect that with your changes?

Good catch, I didn't think about looking at pm_netlink.sh.

The 3 errors I see looks normal to me:
- trying to add an entry already there
- trying to set the two limits above the hard limit

So I guess I can mute stderr in a v2.

Cheers,
Matt
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net