[Devel] [PATCH hci-8.1 v4 0/1] drv-net-hw: skip the tests that need a real interface

[Devel] [PATCH hci-8.1 v4 0/1] drv-net-hw: skip the tests that need a real interface

[Eva Kurchatova]

Patchset for task https://virtuozzo.atlassian.net/browse/VSTOR-139651

Changes in v4:
- Runs that pick their interfaces by driver (NETIF_FIND_DRIVER) were
  skipped; pass them to lib.sh untouched, allowing to run now.
- Unchanged: only the first interface is judged, lib.sh builds its pair
  from that one and skips by itself when a later one is missing. Only
  a veth is detected. Everything else is configuration choice.
- Rebased.

Link to v3: http://lore.virtuozzo.com/kernel/20261006222651.2510417-1-eva.kurchatova@virtuozzo.com/T/#t
    
Changes in v3:
- Check the kind of the first interface instead of NETIF_CREATE and
  NETIF_TYPE, which stay at their defaults when forwarding.config
  names real NICs; those ran through lib.sh and were skipped here.
- Take the interfaces from the command line too, as lib.sh does.
- Rebased.

Link to v2: http://lore.virtuozzo.com/kernel/20260928194332.551992-1-eva.kurchatova@virtuozzo.com/T/#t

Changes in v2:
- Read forwarding.config before deciding, the way lib.sh does.
- One helper, skip_on_veth.sh, instead of a copy in each test.
- Add ethtool_rmon.sh, it fails on veth the same way.
- One-line comments, fix the Feature tag.
- Sent alone, 1/4 to 3/4 are merged.

Link to v1: http://lore.virtuozzo.com/kernel/20260831225137.1642608-4-eva.kurchatova@virtuozzo.com/

Eva Kurchatova (1):
  selftests: drv-net-hw: skip the tests that need a real interface

 .../testing/selftests/drivers/net/hw/Makefile |  1 +
 .../selftests/drivers/net/hw/ethtool.sh       |  5 +++
 .../drivers/net/hw/ethtool_extended_state.sh  |  5 +++
 .../selftests/drivers/net/hw/ethtool_rmon.sh  |  5 +++
 .../selftests/drivers/net/hw/hw_stats_l3.sh   |  5 +++
 .../drivers/net/hw/hw_stats_l3_gre.sh         |  5 +++
 .../selftests/drivers/net/hw/skip_on_veth.sh  | 42 +++++++++++++++++++
 7 files changed, 68 insertions(+)
 create mode 100644 tools/testing/selftests/drivers/net/hw/skip_on_veth.sh

-- 
2.56.0

_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

[Devel] [PATCH hci-8.1 v4 1/1] selftests: drv-net-hw: skip the tests that need a real interface

[Eva Kurchatova]

ethtool, ethtool_extended_state, ethtool_rmon, hw_stats_l3 and
hw_stats_l3_gre need hardware: link speed, extended link state, rmon
counters, L3 statistics offload. With no interfaces configured lib.sh
substitutes a veth pair, and every case fails on what veth does not
have.

Upstream runs these tests on real hardware only and dropped the veth
check in commit 0c499a351777 ("selftests: forwarding: Ditch
skip_on_veth()"), so this patch is not for upstream.

Skip when the first interface the test would use is a veth, or does
not exist and lib.sh would create one. Real interfaces, named in
forwarding.config or on the command line, run as before, and so does a
run that picks its interfaces by driver with NETIF_FIND_DRIVER. The
check runs before lib.sh is sourced, since lib.sh creates the pair as
it is sourced and would leave it behind.

https://virtuozzo.atlassian.net/browse/VSTOR-139651
Feature: fix selftests
Signed-off-by: Eva Kurchatova <eva.kurchatova@virtuozzo.com>
---
 .../testing/selftests/drivers/net/hw/Makefile |  1 +
 .../selftests/drivers/net/hw/ethtool.sh       |  5 +++
 .../drivers/net/hw/ethtool_extended_state.sh  |  5 +++
 .../selftests/drivers/net/hw/ethtool_rmon.sh  |  5 +++
 .../selftests/drivers/net/hw/hw_stats_l3.sh   |  5 +++
 .../drivers/net/hw/hw_stats_l3_gre.sh         |  5 +++
 .../selftests/drivers/net/hw/skip_on_veth.sh  | 42 +++++++++++++++++++
 7 files changed, 68 insertions(+)
 create mode 100644 tools/testing/selftests/drivers/net/hw/skip_on_veth.sh

diff --git a/tools/testing/selftests/drivers/net/hw/Makefile b/tools/testing/selftests/drivers/net/hw/Makefile
index e556b5150a51..39aece5e2776 100644
--- a/tools/testing/selftests/drivers/net/hw/Makefile
+++ b/tools/testing/selftests/drivers/net/hw/Makefile
@@ -19,6 +19,7 @@ TEST_PROGS = \
 
 TEST_FILES := \
 	ethtool_lib.sh \
+	skip_on_veth.sh \
 	#
 
 TEST_INCLUDES := \
diff --git a/tools/testing/selftests/drivers/net/hw/ethtool.sh b/tools/testing/selftests/drivers/net/hw/ethtool.sh
index fa6953de6b6d..e1fbe37a71fc 100755
--- a/tools/testing/selftests/drivers/net/hw/ethtool.sh
+++ b/tools/testing/selftests/drivers/net/hw/ethtool.sh
@@ -11,6 +11,11 @@ ALL_TESTS="
 "
 NUM_NETIFS=2
 lib_dir=$(dirname "$0")
+
+# Needs a device that can set link speed and autonegotiation
+source "$lib_dir"/skip_on_veth.sh
+skip_on_veth "$@"
+
 source "$lib_dir"/../../../net/forwarding/lib.sh
 source ethtool_lib.sh
 
diff --git a/tools/testing/selftests/drivers/net/hw/ethtool_extended_state.sh b/tools/testing/selftests/drivers/net/hw/ethtool_extended_state.sh
index a7584448416e..1201130cae67 100755
--- a/tools/testing/selftests/drivers/net/hw/ethtool_extended_state.sh
+++ b/tools/testing/selftests/drivers/net/hw/ethtool_extended_state.sh
@@ -9,6 +9,11 @@ ALL_TESTS="
 
 NUM_NETIFS=2
 lib_dir=$(dirname "$0")
+
+# Needs a device that reports an extended link state
+source "$lib_dir"/skip_on_veth.sh
+skip_on_veth "$@"
+
 source "$lib_dir"/../../../net/forwarding/lib.sh
 source ethtool_lib.sh
 
diff --git a/tools/testing/selftests/drivers/net/hw/ethtool_rmon.sh b/tools/testing/selftests/drivers/net/hw/ethtool_rmon.sh
index 8f60c1685ad4..1745afe671d4 100755
--- a/tools/testing/selftests/drivers/net/hw/ethtool_rmon.sh
+++ b/tools/testing/selftests/drivers/net/hw/ethtool_rmon.sh
@@ -8,6 +8,11 @@ ALL_TESTS="
 
 NUM_NETIFS=2
 lib_dir=$(dirname "$0")
+
+# Needs a device with rmon histogram counters
+source "$lib_dir"/skip_on_veth.sh
+skip_on_veth "$@"
+
 source "$lib_dir"/../../../net/forwarding/lib.sh
 
 ETH_FCS_LEN=4
diff --git a/tools/testing/selftests/drivers/net/hw/hw_stats_l3.sh b/tools/testing/selftests/drivers/net/hw/hw_stats_l3.sh
index 67fafefc80be..460633f27f40 100755
--- a/tools/testing/selftests/drivers/net/hw/hw_stats_l3.sh
+++ b/tools/testing/selftests/drivers/net/hw/hw_stats_l3.sh
@@ -49,6 +49,11 @@ ALL_TESTS="
 "
 NUM_NETIFS=4
 lib_dir=$(dirname "$0")
+
+# Needs a device that offloads L3 statistics
+source "$lib_dir"/skip_on_veth.sh
+skip_on_veth "$@"
+
 source "$lib_dir"/../../../net/forwarding/lib.sh
 source "$lib_dir"/../../../net/forwarding/tc_common.sh
 
diff --git a/tools/testing/selftests/drivers/net/hw/hw_stats_l3_gre.sh b/tools/testing/selftests/drivers/net/hw/hw_stats_l3_gre.sh
index a94d92e1abce..abe8c09700b4 100755
--- a/tools/testing/selftests/drivers/net/hw/hw_stats_l3_gre.sh
+++ b/tools/testing/selftests/drivers/net/hw/hw_stats_l3_gre.sh
@@ -13,6 +13,11 @@ ALL_TESTS="
 "
 NUM_NETIFS=6
 lib_dir=$(dirname "$0")
+
+# Needs a device that offloads L3 statistics
+source "$lib_dir"/skip_on_veth.sh
+skip_on_veth "$@"
+
 source "$lib_dir"/../../../net/forwarding/lib.sh
 source "$lib_dir"/../../../net/forwarding/ipip_lib.sh
 source "$lib_dir"/../../../net/forwarding/tc_common.sh
diff --git a/tools/testing/selftests/drivers/net/hw/skip_on_veth.sh b/tools/testing/selftests/drivers/net/hw/skip_on_veth.sh
new file mode 100644
index 000000000000..64f9c217d0fe
--- /dev/null
+++ b/tools/testing/selftests/drivers/net/hw/skip_on_veth.sh
@@ -0,0 +1,42 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-2.0
+
+# Source with lib_dir set, ahead of lib.sh, which creates the interfaces.
+
+forwarding_dir="$lib_dir"/../../../net/forwarding
+
+# lib.sh reads NETIF_FIND_DRIVER before forwarding.config: the environment only
+find_driver=${NETIF_FIND_DRIVER-}
+
+# forwarding.config assigns by name into the array lib.sh declares
+declare -A NETIFS
+
+if [[ -f $forwarding_dir/forwarding.config ]]; then
+	source "$forwarding_dir/forwarding.config"
+fi
+
+: "${NETIF_CREATE:=yes}"
+: "${NETIF_TYPE:=veth}"
+
+# Call before sourcing lib.sh, exiting after it leaves the veth pair behind.
+# Judges by the first interface, the one lib.sh creates the pair from,
+# found the way lib.sh finds it: keep in step with lib.sh.
+skip_on_veth()
+{
+	# lib.sh takes the interfaces from the command line ahead of anything
+	local p1=${1:-}
+	local kind
+
+	# Otherwise with NETIF_FIND_DRIVER it takes those of that driver, and
+	# skips by itself when it finds none
+	[[ -z $p1 && -n $find_driver ]] && return
+	p1=${p1:-${NETIFS[p1]-veth0}}
+	if ip link show dev "$p1" &> /dev/null; then
+		kind=$(ip -j -d link show dev "$p1" | jq -r '.[].linkinfo.info_kind')
+	elif [[ $NETIF_CREATE == yes && $NETIF_TYPE == veth ]]; then
+		kind=veth # what lib.sh creates when the interface is missing
+	fi
+	[[ $kind == veth ]] || return
+	echo "SKIP: needs a real interface, set NETIFS in forwarding.config"
+	exit 4 # ksft_skip, lib.sh is not sourced yet
+}
-- 
2.56.0

_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel

Re: [Devel] [PATCH hci-8.1 v4 1/1] selftests: drv-net-hw: skip the tests that need a real interface

[no-reply]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] selftests: skip_on_veth() misses a forwarding.config veth override under NETIF_FIND_DRIVER
- [Low] selftests: skip_on_veth() duplicates lib.sh interface selection and skips explicit veths
--

commit cb2e8721faec852dd04b82eca1ff441b1fede385
Author: Eva Kurchatova <eva.kurchatova@virtuozzo.com>

selftests: drv-net-hw: skip the tests that need a real interface

This patch adds a skip_on_veth() helper. The helper is sourced by the
ethtool, ethtool_extended_state, ethtool_rmon, hw_stats_l3 and
hw_stats_l3_gre tests ahead of lib.sh. It exits with ksft_skip when the
first interface would be a veth, or when lib.sh would create one.

> diff --git a/tools/testing/selftests/drivers/net/hw/skip_on_veth.sh b/tools/testing/selftests/drivers/net/hw/skip_on_veth.sh
> new file mode 100644
> index 0000000000000..64f9c217d0fed
> --- /dev/null
> +++ b/tools/testing/selftests/drivers/net/hw/skip_on_veth.sh
> @@ -0,0 +1,42 @@

[ ... ]

> +# forwarding.config assigns by name into the array lib.sh declares
> +declare -A NETIFS
> +
> +if [[ -f $forwarding_dir/forwarding.config ]]; then
> +	source "$forwarding_dir/forwarding.config"
> +fi
> +
> +: "${NETIF_CREATE:=yes}"
> +: "${NETIF_TYPE:=veth}"
> +
> +# Call before sourcing lib.sh, exiting after it leaves the veth pair behind.
> +# Judges by the first interface, the one lib.sh creates the pair from,
> +# found the way lib.sh finds it: keep in step with lib.sh.
> +skip_on_veth()
> +{
> +	# lib.sh takes the interfaces from the command line ahead of anything
> +	local p1=${1:-}
> +	local kind
> +
> +	# Otherwise with NETIF_FIND_DRIVER it takes those of that driver, and
> +	# skips by itself when it finds none
> +	[[ -z $p1 && -n $find_driver ]] && return

[Severity: Medium]
Can this early return bypass the veth check when forwarding.config
overrides the discovered interface? In lib.sh, netif_find_driver runs
first when NETIF_FIND_DRIVER is set, and forwarding.config is sourced
after that:

    if [[ $NETIF_FIND_DRIVER ]]; then
        unset NETIFS
        declare -A NETIFS
        netif_find_driver
    fi
    ...
    if [[ -f $net_forwarding_dir/forwarding.config ]]; then
        source "$net_forwarding_dir/forwarding.config"

Suppose NETIF_FIND_DRIVER is set, no arguments are given, and
forwarding.config sets NETIFS[p1] to a veth. Suppose instead it sets
NETIFS[p1] to a missing name that lib.sh would create as a veth. In
both cases skip_on_veth() returns without looking at NETIFS[p1]. The
five tests then run on veth and fail, which is what this patch is meant
to prevent.

The comment above the function says the logic is "found the way lib.sh
finds it: keep in step with lib.sh", but this ordering is not mirrored.
Should the NETIFS[p1] value from forwarding.config be checked in this
case too?

> +	p1=${p1:-${NETIFS[p1]-veth0}}
> +	if ip link show dev "$p1" &> /dev/null; then
> +		kind=$(ip -j -d link show dev "$p1" | jq -r '.[].linkinfo.info_kind')

[Severity: Low]
This isn't a bug, but skip_on_veth() copies lib.sh's interface selection
by hand. That covers the NETIFS defaults, NETIF_FIND_DRIVER,
forwarding.config, command line arguments and NETIF_CREATE/NETIF_TYPE.
The two copies have to be kept in step manually.

forwarding.config is also sourced twice, once here and once in lib.sh,
so any side effects in the config run twice.

The check skips purely on info_kind being veth. A user who deliberately
passes or configures a pre-created veth pair is therefore skipped.
The message "set NETIFS in forwarding.config" is misleading when NETIFS
is already set.

Would it be cleaner to share the selection logic with lib.sh rather than
duplicating it?

> +	elif [[ $NETIF_CREATE == yes && $NETIF_TYPE == veth ]]; then
> +		kind=veth # what lib.sh creates when the interface is missing
> +	fi
> +	[[ $kind == veth ]] || return
> +	echo "SKIP: needs a real interface, set NETIFS in forwarding.config"
> +	exit 4 # ksft_skip, lib.sh is not sourced yet
> +}

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261007145044.2647274-1-eva.kurchatova@virtuozzo.com?part=1

_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel