Re: [PATCH net-next v3 2/9] selftests: net: add helpers for running a command on other targets

[Petr Machata <petrm@nvidia.com>]

Ioana Ciornei <ioana.ciornei@nxp.com> writes:

> On Fri, Mar 20, 2026 at 11:58:42AM +0100, Petr Machata wrote:
>> 
>> Ioana Ciornei <ioana.ciornei@nxp.com> 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_cmd() function decides where to execute the command passed
>> > through $@ based on the env variable TARGET value while run_on() will
>> > receive the target through its first argument.
>> >
>> > Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>
>> > ---
>> > 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 | 39 ++++++++++++++++++++++++++++++
>> >  1 file changed, 39 insertions(+)
>> >
>> > diff --git a/tools/testing/selftests/net/lib.sh b/tools/testing/selftests/net/lib.sh
>> > index b40694573f4c..f7c54d05758e 100644
>> > --- a/tools/testing/selftests/net/lib.sh
>> > +++ b/tools/testing/selftests/net/lib.sh
>> > @@ -28,6 +28,10 @@ EXIT_STATUS=0
>> >  # Per-test return value. Clear at the beginning of each test.
>> >  RET=0
>> >  
>> > +# If a specific command needs to be executed on another target than local, set
>> > +# this appropriately before calling run_cmd
>> > +CUR_TARGET="local:"
>> > +
>> 
>> These are new functions, they should just expose these parameters as
>> actual function parameters, not as hidden environment. If it is
>> desirable that run_cmd() can run local commands, maybe it makes sense to
>> make the current run_cmd() a helper like __run_cmd(), which takes the
>> remote-config parameter, and have run_cmd() be a wrapper:
>> 
>> run_cmd()
>> {
>> 	__run_cmd local: "$@"
>> }
>
> Why I chose to also add a function like run_cmd that makes use of an
> environment variable is to support cases such as the following.
>
> get_ifname_by_ip()
> {
> 	local ip_addr=$1; shift
>
> 	run_cmd ip -j addr show to "$ip_addr" | jq -r '.[].ifname'
> }

Oh, I see, this didn't even occur to me. run_cmd() remembering the last
used target like this is... unobvious. I can't promise you it would bite
us in the ass in the future, but stuff like this tends to bite one in
their ass. If get_ifname_by_ip() needs to know where it should run, well
then that's just the way it is.

> Helpers such as get_ifname_by_ip() (added in patch #3) that don't have
> access to an interface name so that they can directly call run_on.
>
> Do you mean that for any helpers such as the one above that need to be
> run both on the local system and the remote it's better just to add a
> new parameter to it that it's indeed the target on which it should be
> run and get rid of the run_cmd all together?

run_cmd() is fine I think. It just needs to be honest about its
requirements.

> get_ifname_by_ip()
> {
> 	local target=$1; shift
> 	local ip_addr=$1; shift
>
> 	run_on "$target" ip -j addr show to "$ip_addr" | jq -r '.[].ifname'
> }

Yep.