Re: [PATCH net-next v4 02/10] selftests: net: add helpers for running a command on other targets

[Petr Machata]

Petr Machata <petrm@xxxxxxxxxx> writes:

> Ioana Ciornei <ioana.ciornei@xxxxxxx> writes:
>
>> Add a couple of helpers which can be used by tests which need to run a
>> specific bash command on a different target than the local system, be it
>> either another netns or a remote system accessible through ssh.
>>
>> The __run_on() function is passed through $1 the target on which the
>> command should be executed while run_on() is passed the name of the
>> interface that is then used to retrieve the target from the TARGETS
>> array.
>>
>> Signed-off-by: Ioana Ciornei <ioana.ciornei@xxxxxxx>
>> ---
>> Changes in v4:
>> - reworked the helpers so that no global variable is used and
>>   information is passed only through parameters
>> Changes in v3:
>> - s/TARGET/CUR_TARGET
>> - always fallback on running a command locally when either TARGETS is
>>   not declared or there is no entry for a specific interface
>> Changes in v2:
>> - patch is new
>>
>>  tools/testing/selftests/net/lib.sh | 38 ++++++++++++++++++++++++++++++
>>  1 file changed, 38 insertions(+)
>>
>> diff --git a/tools/testing/selftests/net/lib.sh b/tools/testing/selftests/net/lib.sh
>> index b40694573f4c..6c0d613a4de5 100644
>> --- a/tools/testing/selftests/net/lib.sh
>> +++ b/tools/testing/selftests/net/lib.sh
>> @@ -670,3 +670,41 @@ cmd_jq()
>>  	# return success only in case of non-empty output
>>  	[ ! -z "$output" ]
>>  }
>> +
>> +__run_on()
>> +{
>> +	local target=$1; shift
>> +	local type args
>> +
>> +	IFS=':' read -r type args <<< "$target"
>> +
>> +	case "$type" in
>> +	netns)
>> +		# Execute command in network namespace
>> +		# args contains the namespace name
>> +		ip netns exec "$args" "$@"
>> +		;;
>> +	ssh)
>> +		# Execute command via SSH args contains user@host
>> +		ssh -n "$args" "$@"
>> +		;;
>> +	local|*)
>> +		# Execute command locally. This is also the fallback
>> +		# case for when the interface's target is not found in
>> +		# the TARGETS array.
>> +		"$@"
>> +		;;
>> +	esac
>> +}
>> +
>> +run_on()
>> +{
>> +	local iface=$1; shift
>> +	local target="local:"
>> +
>> +	if declare -p TARGETS &>/dev/null; then
>> +		target="${TARGETS[$iface]}"
>
> So I think Jakub's runs fail because there's a shell export somewhere
> that gets inherited through make to the launched test. I guess it would
> be enough for the test to validate that TARGETS is an array, because
> those don't get inherited.
>
> Is there a reason not to reuse DRIVER_TEST_CONFORMANT as a tell though?
>
>> +	fi
>> +
>> +	__run_on "$target" "$@"
>> +}
>
> Does the latter helper need to be in net/lib.sh? Since it uses TARGETS,
> which are a forwarding/lib.sh concept, it seems misplaced there.

Oh, I see, there's an invocation from mac_get() in net/lib.sh itself.
Hummm. Not sure how to tackle this.

I think lib.sh might unset TARGETS explicitly? Or declare -A, but leave
empty? Since it's now an API, net/lib.sh needs to set it to a reasonable
value (or erase). Then forwarding/lib.sh might in theory rely on
existence of that variable and not have to declare it at all.

Or, maybe have a stub run_on() like this to satisfy the run_on() API:

run_on()
{
	"$@"
}

And have the full-blown thing in forward/lib.sh. All the magic with
TARGETS really belongs to forwarding/lib.sh. Bash allows function
redefinition just fine, so a user importing just net/lib.sh would get
the stub, and forwarding/lib.sh users would get the full thing.