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

["Eva Kurchatova (Virtuozzo)" <eva.kurchatova@virtuozzo.com>]

On 10/23/26 22:25, no-reply@sashiko.virtuozzo.com wrote:
> 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
>> +}

It is not empty. skip_on_veth.sh sources forwarding.config into the same 
shell that sources lib.sh afterwards, so a NETIF_FIND_DRIVER set there 
is a shell variable lib.sh sees, no export needed. lib.sh then picks the 
interfaces by driver. Checked on a VM with NETIF_FIND_DRIVER=virtio_net 
only in forwarding.config: lib.sh took the virtio NIC, and the helper 
stepped aside as it should.


> [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.

It still applies: NETIFS has to name a real interface, and here it names 
a veth. A veth named on purpose is skipped by intent, these tests cannot 
pass on one.


> 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.


Same as in v4: lib.sh builds its pair from p1, and a real p1 with a veth 
p2 is a configuration someone wrote that way. I can change this, but 
this seems out of scope.


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