Re: [PATCH mptcp-net 2/2] selftests: mptcp: lib: get counters for the right test

[Geliang Tang <geliang@kernel.org>]

Hi Matt,

On Thu, 2026-08-13 at 12:29 +0200, Matthieu Baerts (NGI0) wrote:
> When the value for a MIB counter is required, mptcp_lib_get_counter
> is
> called. It tries to use the cache, if available. If not it falls back
> to
> calling 'nstat' directly by looking at the absolute counters.
> 
> That's an issue for tests that don't recreate the netns for each
> subtest. In this case, 'nstat -a' will look at the counters for the
> netns.
> 
> Instead, it should look at the increment for the current test, by
> using
> the history recorded in /tmp/<ns>.nstat, if available, and not using
> '-a' which was dumping the absolute values.
> 
> While at it, rename the previous 'hist' variable to 'cache' as it was
> used to look at the cache, not the nstat history.
> 
> Fixes: 71388a9f331d ("selftests: mptcp: lib: get counters from nstat
> history")
> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> ---
>  tools/testing/selftests/net/mptcp/mptcp_lib.sh | 16 +++++++++-------
>  1 file 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 da1da414c30f..b9d14647f401 100644
> --- a/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> @@ -416,19 +416,21 @@ mptcp_lib_nstat_get() {
>  }
>  
>  # $1: ns, $2: MIB counter
> -# Get the counter from the history (mptcp_lib_nstat_{init,get}()) if
> available.
> -# If not, get the counter from nstat ignoring any history.
> +# Get the counter from the cache (mptcp_lib_nstat_{init,get}()) if
> available.
> +# If not, get the counter from nstat ignoring any cache, but using
> the history.
>  mptcp_lib_get_counter() {
>  	local ns="${1}"
>  	local counter="${2}"
> -	local hist="/tmp/${ns}.out"
> +	local cache="/tmp/${ns}.out"
> +	local hist="/tmp/${ns}.nstat"
>  	local count
>  
> -	if [[ -s "${hist}" && "${counter}" == *"Tcp"* ]]; then
> -		count=$(awk "/^${counter} / {print \$2; exit}"
> "${hist}")
> +	if [[ -s "${cache}" && "${counter}" == *"Tcp"* ]]; then
> +		count=$(awk "/^${counter} / {print \$2; exit}"
> "${cache}")
>  	else
> -		count=$(ip netns exec "${ns}" nstat -asz
> "${counter}" |
> -			awk 'NR==1 {next} {print $2}')
> +		count=$(NSTAT_HISTORY="${hist}" ip netns exec
> "${ns}" \
> +			nstat -sz "${counter}" |
> +				awk 'NR==1 {next} {print $2}')
>  	fi
>  	if [ -z "${count}" ]; then
>  		mptcp_lib_fail_if_expected_feature "${counter}
> counter"

I noticed that the code being modified in mptcp_lib_pr_nstat and
mptcp_lib_get_counter is duplicated. I'm wondering if we could remove
this redundancy - for example, by creating a new helper for this logic.

# $1: ns ; $@: nstat arguments
# If cache exists, return it; otherwise run nstat.
mptcp_lib_nstat_cmd() {
    local ns="${1}"
    local cache="/tmp/${ns}.out"
    local hist="/tmp/${ns}.nstat"

    if [ -s "${cache}" ]; then
        cat "${cache}"
    else
        NSTAT_HISTORY="${hist}" ip netns exec "${ns}" nstat -sz "${@}"
    fi
}

Then mptcp_lib_pr_nstat can be simplified to:

mptcp_lib_pr_nstat() {
    local ns="${1}"

    mptcp_lib_nstat_cmd "${ns}" |
            awk '/Tcp/ { print "  "$0 }'
}

And mptcp_lib_get_counter can be simplified to:

mptcp_lib_get_counter() {
    local ns="${1}"
    local counter="${2}"
    local count

    count=$(mptcp_lib_nstat_cmd "${ns}" "${counter}" |
            awk -v c="${counter}" '$1 == c {print $2; exit}')
    if [ -z "${count}" ]; then
        mptcp_lib_fail_if_expected_feature "${counter} counter"
        return 1
    fi

    echo "${count}"
}

What do you think - would this work?

Thanks,
-Geliang