netdev - Re: [PATCH net-next 01/10] selftests: net: lib: Introduce deferred commands

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: <87ttddhg03.fsf@nvidia.com>
Date: Tue, 15 Oct 2024 11:06:35 +0200
From: Petr Machata <petrm@...dia.com>
To: Paolo Abeni <pabeni@...hat.com>
CC: Petr Machata <petrm@...dia.com>, "David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
	<netdev@...r.kernel.org>, <linux-kselftest@...r.kernel.org>, Shuah Khan
	<shuah@...nel.org>, Benjamin Poirier <bpoirier@...dia.com>, Hangbin Liu
	<liuhangbin@...il.com>, Vladimir Oltean <vladimir.oltean@....com>, "Ido
 Schimmel" <idosch@...dia.com>, Przemek Kitszel
	<przemyslaw.kitszel@...el.com>, <mlxsw@...dia.com>
Subject: Re: [PATCH net-next 01/10] selftests: net: lib: Introduce deferred
 commands


Paolo Abeni <pabeni@...hat.com> writes:

> Hi,
>
> On 10/9/24 14:06, Petr Machata wrote:
>> diff --git a/tools/testing/selftests/net/lib/sh/defer.sh b/tools/testing/selftests/net/lib/sh/defer.sh
>> new file mode 100644
>> index 000000000000..8d205c3f0445
>> --- /dev/null
>> +++ b/tools/testing/selftests/net/lib/sh/defer.sh
>> @@ -0,0 +1,115 @@
>> +#!/bin/bash
>> +# SPDX-License-Identifier: GPL-2.0
>> +
>> +# map[(scope_id,track,cleanup_id) -> cleanup_command]
>> +# track={d=default | p=priority}
>> +declare -A __DEFER__JOBS
>> +
>> +# map[(scope_id,track) -> # cleanup_commands]
>> +declare -A __DEFER__NJOBS
>> +
>> +# scope_id of the topmost scope.
>> +__DEFER__SCOPE_ID=0
>> +
>> +__defer__ndefer_key()
>> +{
>> +	local track=$1; shift
>
> Minor nit: IMHO the trailing shift is here a bit confusing: it let me
> think about other arguments, which are not really expected.

This is IMHO how a function header should look like:

function()
{
	local foo=$1; shift
	local bar=$1; shift
	local baz=$1; shift

	...
}

Because it lets you reorder the arguments freely just by reordering the
lines, copy argument subsets to other functions without risking
forgetting / screwing up renumbering, etc. It's easy to parse visually
as well. If the function uses "$@" as rest argument, it will contain the
rest by default. It's just a very convenient notation overall. Vast
majority of net/lib.sh and net/forwarding/lib.sh use this.

>> +__defer__schedule()
>> +{
>> +	local track=$1; shift
>> +	local ndefers=$(__defer__ndefers $track)
>> +	local ndefers_key=$(__defer__ndefer_key $track)
>> +	local defer_key=$(__defer__defer_key $track $ndefers)
>> +	local defer="$@"
>> +
>> +	__DEFER__JOBS[$defer_key]="$defer"
>> +	__DEFER__NJOBS[$ndefers_key]=$((${__DEFER__NJOBS[$ndefers_key]} + 1))
>
> '${__DEFER__NJOBS[$ndefers_key]}' is actually '$ndefers', right? If so
> it would be better to reuse the avail variable.

I figured I would leave it all spelled out, because the left hand side
needs to be, and having the same expression on both sides makes it clear
that this is just an X++ sort of a deal.