Paolo Abeni <[email protected]> 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.
