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

[Petr Machata <petrm@nvidia.com>]

Paolo Abeni <pabeni@redhat.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.