netdev - Re: [PATCH net 10/17] selftests: forwarding: ethtool

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date: Wed, 2 Aug 2023 16:22:35 +0200
From: Petr Machata <petrm@...dia.com>
To: Ido Schimmel <idosch@...sch.org>
CC: Petr Machata <petrm@...dia.com>, <vladimir.oltean@....com>, Ido Schimmel
	<idosch@...dia.com>, <netdev@...r.kernel.org>, <davem@...emloft.net>,
	<kuba@...nel.org>, <pabeni@...hat.com>, <edumazet@...gle.com>,
	<razor@...ckwall.org>, <mirsad.todorovac@....unizg.hr>
Subject: Re: [PATCH net 10/17] selftests: forwarding: ethtool_mm: Skip when
 using veth pairs


Ido Schimmel <idosch@...sch.org> writes:

> On Wed, Aug 02, 2023 at 02:27:49PM +0200, Petr Machata wrote:
>> 
>> Vladimir Oltean <vladimir.oltean@....com> writes:
>> 
>> > @@ -266,6 +278,14 @@ setup_prepare()
>> >  	h1=${NETIFS[p1]}
>> >  	h2=${NETIFS[p2]}
>> >
>> > +	for netif in ${NETIFS[@]}; do
>> > +		ethtool --show-mm $netif 2>&1 &> /dev/null
>> > +		if [[ $? -ne 0 ]]; then
>> > +			echo "SKIP: $netif does not support MAC Merge"
>> > +			exit $ksft_skip
>> > +		fi
>> > +	done
>> > +
>> 
>> Ido, if you decide to go this route, just hoist the loop to the global
>> scope before registering the trap, then you don't need the hX_created
>> business.
>
> I think the idea was to run this check after verifying that ethtool
> supports MAC Merge in setup_prepare(). How about moving all these checks

True, I missed that.

> before doing any configuration and registering a trap handler?
>
> diff --git a/tools/testing/selftests/net/forwarding/ethtool_mm.sh b/tools/testing/selftests/net/forwarding/ethtool_mm.sh
> index 4331e2161e8d..39e736f30322 100755
> --- a/tools/testing/selftests/net/forwarding/ethtool_mm.sh
> +++ b/tools/testing/selftests/net/forwarding/ethtool_mm.sh
> @@ -258,11 +258,6 @@ h2_destroy()
>  
>  setup_prepare()
>  {
> -       check_ethtool_mm_support
> -       check_tc_fp_support
> -       require_command lldptool
> -       bail_on_lldpad "autoconfigure the MAC Merge layer" "configure it manually"
> -
>         h1=${NETIFS[p1]}
>         h2=${NETIFS[p2]}
>  
> @@ -278,7 +273,18 @@ cleanup()
>         h1_destroy
>  }
>  
> -skip_on_veth
> +check_ethtool_mm_support
> +check_tc_fp_support
> +require_command lldptool
> +bail_on_lldpad "autoconfigure the MAC Merge layer" "configure it manually"
> +
> +for netif in ${NETIFS[@]}; do
> +       ethtool --show-mm $netif 2>&1 &> /dev/null
> +       if [[ $? -ne 0 ]]; then
> +               echo "SKIP: $netif does not support MAC Merge"
> +               exit $ksft_skip
> +       fi
> +done
>  
>  trap cleanup EXIT
>

Looks good. These checks are usually placed right after sourcing the
libraries, but I don't care much one way or another.