linux-kernel - Re: [PATCH] selftests/net/forwarding: teamd command not found

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]

Message-ID: <87ikqirk6x.fsf@nvidia.com>
Date: Mon, 13 Jan 2025 12:48:31 +0100
From: Petr Machata <petrm@...dia.com>
To: Jakub Kicinski <kuba@...nel.org>
CC: Alessandro Zanni <alessandro.zanni87@...il.com>, <petrm@...dia.com>,
	Hangbin Liu <liuhangbin@...il.com>, <davem@...emloft.net>,
	<edumazet@...gle.com>, <pabeni@...hat.com>, <shuah@...nel.org>,
	<netdev@...r.kernel.org>, <linux-kselftest@...r.kernel.org>,
	<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] selftests/net/forwarding: teamd command not found


Jakub Kicinski <kuba@...nel.org> writes:

> On Fri, 10 Jan 2025 15:35:23 +0100 Alessandro Zanni wrote:
>> On Fri, Jan 10, 2025 at 04:09:07AM +0000, Hangbin Liu wrote:
>> > On Fri, Jan 10, 2025 at 01:07:44AM +0100, Alessandro Zanni wrote:  
>> > > Running "make kselftest TARGETS=net/forwarding" results in several
>> > > occurrences of the same error:
>> > >  ./lib.sh: line 787: teamd: command not found
>> > > 
>> > > Since many tests depends on teamd, this fix stops the tests if the
>> > > teamd command is not installed.
>> > > 
>> > > Signed-off-by: Alessandro Zanni <alessandro.zanni87@...il.com>
>> > > ---
>> > >  tools/testing/selftests/net/forwarding/lib.sh | 1 +
>> > >  1 file changed, 1 insertion(+)
>> > > 
>> > > diff --git a/tools/testing/selftests/net/forwarding/lib.sh b/tools/testing/selftests/net/forwarding/lib.sh
>> > > index 7337f398f9cc..a6a74a4be4bf 100644
>> > > --- a/tools/testing/selftests/net/forwarding/lib.sh
>> > > +++ b/tools/testing/selftests/net/forwarding/lib.sh
>> > > @@ -784,6 +784,7 @@ team_destroy()
>> > >  {
>> > >  	local if_name=$1; shift
>> > >  
>> > > +	require_command $TEAMD
>> > >  	$TEAMD -t $if_name -k
>> > >  }  
>> > 
>> > I saw team_create() has `require_command $TEAMD`. Is some test called
>> > team_destroy() before team_create()?  
>> 
>> Actually, the errors seem to raise in the "cleanup()" method.
>> 
>> So, first, during the test preparation the "team_create()" method is called
>> and checks if teamd is installed. 
>> When it fails and skips the test, the "cleanup()" method calls
>> the "team_destroy()" that raises the errors.
>
> Actually, maybe we're better off switching the team handling to the way
> REQUIRE_JQ REQUIRE_MZ etc are handled? That way we'll validate and exit 
> when lib is imported / sourced.
>
> Petr, do you have a preference?

Yeah, that would be better.

My first though was that this is fine, because it's just for tests that
don't use defer. (With defer, if the creation bails out, you never
schedule the cleanup.)

But if you bail out early from team_destroy(), the rest of the cleanups
will be skipped, which is bad.

So maybe we need something like this:

	$TEAMD -t $if_name -k 2>/dev/null ||:

... i.e. to just quietly tolerate failures. But that's no good either,
because then you never catch issues with the cleanup itself even in
valid runs. So maybe team_create() should note whether $TEAMD was found
and do this conditionally, but at that point, going with REQUIRE_TEAMD
is the cleaner solution.

It looks like we have 7 scripts that use team_create(), so it's not
crazy to just fix them all.