Using '${?}' inside the if-statement to check the returned value from
the command that was evaluated as part of the if-statement is not
correct: here, '${?}' will be linked to the previous instruction, not
the one that is expected here (${cmd}).

Instead, simply mark the error, except if an error is expected. If
that's the case, 1 can be passed as the 4th argument of this helper. Two
checks from pm_netlink.sh expect an error.

Note that we could expect a specific returned value, but the checks
currently expecting an error can be used with 'ip mptcp' or 'pm_nl_ctl',
and these two tools don't return the same error code.

Fixes: 2d0c1d27ea4e ("selftests: mptcp: add mptcp_lib_check_output helper")
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
 tools/testing/selftests/net/mptcp/mptcp_lib.sh  | 8 ++++----
 tools/testing/selftests/net/mptcp/pm_netlink.sh | 8 +++++---
 2 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_lib.sh b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
index 5fea7e7df628..9e0cd426845d 100644
--- a/tools/testing/selftests/net/mptcp/mptcp_lib.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
@@ -474,18 +474,18 @@ mptcp_lib_wait_local_port_listen() {
 	wait_local_port_listen "${@}" "tcp"
 }
 
+# $1: error file, $2: cmd, $3: expected msg, [$4: expected error]
 mptcp_lib_check_output() {
 	local err="${1}"
 	local cmd="${2}"
 	local expected="${3}"
+	local exp_error="${4:-0}"
 	local cmd_ret=0
 	local out
 
-	if ! out=$(${cmd} 2>"${err}"); then
-		cmd_ret=${?}
-	fi
+	out=$(${cmd} 2>"${err}") || cmd_ret=1
 
-	if [ ${cmd_ret} -ne 0 ]; then
+	if [ "${cmd_ret}" != "${exp_error}" ]; then
 		mptcp_lib_pr_fail "command execution '${cmd}' stderr"
 		cat "${err}"
 		return 2
diff --git a/tools/testing/selftests/net/mptcp/pm_netlink.sh b/tools/testing/selftests/net/mptcp/pm_netlink.sh
index 123d9d7a0278..26ff4c360a77 100755
--- a/tools/testing/selftests/net/mptcp/pm_netlink.sh
+++ b/tools/testing/selftests/net/mptcp/pm_netlink.sh
@@ -122,10 +122,12 @@ check()
 	local cmd="$1"
 	local expected="$2"
 	local msg="$3"
+	local no_err="$4"
 	local rc=0
 
 	mptcp_lib_print_title "$msg"
-	mptcp_lib_check_output "${err}" "${cmd}" "${expected}" || rc=${?}
+	mptcp_lib_check_output "${err}" "${cmd}" "${expected}" "${no_err}" ||
+		rc=${?}
 	if [ ${rc} -eq 2 ]; then
 		mptcp_lib_result_fail "${msg} # error ${rc}"
 		ret=${KSFT_FAIL}
@@ -158,13 +160,13 @@ check "show_endpoints" \
 			    "3,10.0.1.3,signal backup")" "dump addrs"
 
 del_endpoint 2
-check "get_endpoint 2" "" "simple del addr"
+check "get_endpoint 2" "" "simple del addr" 1
 check "show_endpoints" \
 	"$(format_endpoints "1,10.0.1.1" \
 			    "3,10.0.1.3,signal backup")" "dump addrs after del"
 
 add_endpoint 10.0.1.3 2>/dev/null
-check "get_endpoint 4" "" "duplicate addr"
+check "get_endpoint 4" "" "duplicate addr" 1
 
 add_endpoint 10.0.1.4 flags signal
 check "get_endpoint 4" "$(format_endpoints "4,10.0.1.4,signal")" "id addr increment"

-- 
2.53.0

Hello,

On 14/04/2026 15:34, Matthieu Baerts (NGI0) wrote:
> Using '${?}' inside the if-statement to check the returned value from
> the command that was evaluated as part of the if-statement is not
> correct: here, '${?}' will be linked to the previous instruction, not
> the one that is expected here (${cmd}).
> 
> Instead, simply mark the error, except if an error is expected. If
> that's the case, 1 can be passed as the 4th argument of this helper. Two
> checks from pm_netlink.sh expect an error.
> 
> Note that we could expect a specific returned value, but the checks
> currently expecting an error can be used with 'ip mptcp' or 'pm_nl_ctl',
> and these two tools don't return the same error code.

(...)

> diff --git a/tools/testing/selftests/net/mptcp/mptcp_lib.sh b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> index 5fea7e7df628..9e0cd426845d 100644
> --- a/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> @@ -474,18 +474,18 @@ mptcp_lib_wait_local_port_listen() {
>  	wait_local_port_listen "${@}" "tcp"
>  }
>  
> +# $1: error file, $2: cmd, $3: expected msg, [$4: expected error]
>  mptcp_lib_check_output() {
>  	local err="${1}"
>  	local cmd="${2}"
>  	local expected="${3}"
> +	local exp_error="${4:-0}"
>  	local cmd_ret=0
>  	local out
>  
> -	if ! out=$(${cmd} 2>"${err}"); then
> -		cmd_ret=${?}
> -	fi
> +	out=$(${cmd} 2>"${err}") || cmd_ret=1
>  
> -	if [ ${cmd_ret} -ne 0 ]; then
> +	if [ "${cmd_ret}" != "${exp_error}" ]; then
>  		mptcp_lib_pr_fail "command execution '${cmd}' stderr"
>  		cat "${err}"

Here, we could print a better error and output:

-               mptcp_lib_pr_fail "command execution '${cmd}' stderr"
-               cat "${err}"
+               mptcp_lib_pr_fail "unexpected returned code for
'${cmd}', info:"
+               if [ "${exp_error}" = 0 ]; then
+                       cat "${err}"
+               else
+                       echo "${out}"
+               fi


>  		return 2
> diff --git a/tools/testing/selftests/net/mptcp/pm_netlink.sh b/tools/testing/selftests/net/mptcp/pm_netlink.sh
> index 123d9d7a0278..26ff4c360a77 100755
> --- a/tools/testing/selftests/net/mptcp/pm_netlink.sh
> +++ b/tools/testing/selftests/net/mptcp/pm_netlink.sh
> @@ -122,10 +122,12 @@ check()
>  	local cmd="$1"
>  	local expected="$2"
>  	local msg="$3"
> +	local no_err="$4"

I forgot to rename this one to "exp_error".

I can do that when applying the patches if there are nothing else.

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