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

[no-reply@sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Low] selftests: skip_on_veth() skips explicit veth setups with misleading advice
- [Low] selftests: skip_on_veth() honours NETIF_FIND_DRIVER set only in forwarding.config
--

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

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

This adds a skip_on_veth() helper. The ethtool, ethtool_extended_state,
ethtool_rmon, hw_stats_l3 and hw_stats_l3_gre tests call it before sourcing
lib.sh. The helper skips the test when the first interface is a veth, or
when lib.sh would create a veth pair.

> 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..b65d474ac7b24
> --- /dev/null
> +++ b/tools/testing/selftests/drivers/net/hw/skip_on_veth.sh

[ ... ]

> +skip_on_veth()
> +{
> +	# lib.sh takes the command line, then forwarding.config, then the driver
> +	local p1=${1:-${NETIFS[p1]-}}
> +	local kind
> +
> +	# lib.sh skips by itself when the driver has no interfaces
> +	[[ -z $p1 && -n $NETIF_FIND_DRIVER ]] && return

[Severity: Low]
Can this early return run the test on a veth pair that lib.sh creates?

skip_on_veth.sh sources forwarding.config at file scope, so
NETIF_FIND_DRIVER is non-empty here when only forwarding.config sets it,
and NETIFS[p1] is empty. The helper then returns without skipping.

lib.sh checks NETIF_FIND_DRIVER before it sources forwarding.config:

    : "${NETIF_FIND_DRIVER:=}"

    if [[ $NETIF_FIND_DRIVER ]]; then
        unset NETIFS
        declare -A NETIFS
        netif_find_driver
    fi

At that point the variable is empty unless it was exported in the
environment, so discovery is skipped. NETIFS keeps the default veth0..veth9.
forwarding.config is only sourced after that check, and nothing repeats it.

Then lib.sh reaches:

    if [[ "$NETIF_CREATE" = "yes" ]]; then
        create_netif
    fi

and the test runs on the auto-created veth pair and fails, which is the
failure this patch is meant to remove. The helper's comment says it should
keep in step with lib.sh, but it doesn't here. The environment and command
line cases look correct, so only the config-file-only case differs.

Should the early return only apply when NETIF_FIND_DRIVER is set the way
lib.sh will see it?

[ ... ]

> +	[[ $kind == veth ]] || return 0
> +	echo "SKIP: needs a real interface, set NETIFS in forwarding.config"
> +	exit 4 # ksft_skip, lib.sh is not sourced yet
> +}

[Severity: Low]
This isn't a functional bug, but is the SKIP message accurate in all cases
where it is printed?

The same message is printed when NETIFS[p1] or $1 already names an existing
veth. In that case the advice to set NETIFS in forwarding.config doesn't
apply, because it is already set.

Also, only p1 is checked. If p1 is a real interface and p2 or p3 is a veth,
skip_on_veth() returns 0 and the test still fails the way the patch tries to
avoid.

Could the message distinguish an explicitly named veth from the default
case, and could the other interfaces be checked as well?

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

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