[PATCH net v2] selftests: rtnetlink: add 'ethtool' as a dependency

[PATCH net v2] selftests: rtnetlink: add 'ethtool' as a dependency

[Ales Nezbeda]

Introduction of `kci_test_macsec_offload()` in `rtnetlink.sh` created
a new dependency `ethtool` for the test suite. This dependency is not
reflected in checks that run before all the tests, so if the `ethtool`
is not present, all tests will start, but `macsec_offload` test will
fail with a misleading message. Message would say that MACsec offload is
not supported, yet it never actually managed to check this, since it
needs the `ethtool` to do so.

Use `require_command` in `rtnetlink.sh` to allow single check command
for all the dependencies. This also requires adding the `require_command`
to the `lib.sh` used by the test.

Fixes: 3b5222e2ac57 ("selftests: rtnetlink: add MACsec offload tests")
Signed-off-by: Ales Nezbeda <anezbeda@redhat.com>
---
v2
  - Add `require_command` to the `lib.sh` and use that instead
  - v1 ref: https://lore.kernel.org/netdev/20240903151524.586614-1-anezbeda@redhat.com
---
 tools/testing/selftests/net/lib.sh       | 13 +++++++++++++
 tools/testing/selftests/net/rtnetlink.sh |  9 +++------
 2 files changed, 16 insertions(+), 6 deletions(-)

diff --git a/tools/testing/selftests/net/lib.sh b/tools/testing/selftests/net/lib.sh
index 8ee4489238ca..890c579674be 100644
--- a/tools/testing/selftests/net/lib.sh
+++ b/tools/testing/selftests/net/lib.sh
@@ -218,3 +218,16 @@ tc_rule_handle_stats_get()
 	    | jq ".[] | select(.options.handle == $handle) | \
 		  .options.actions[0].stats$selector"
 }
+
+##############################################################################
+# Sanity checks
+
+require_command()
+{
+	local cmd=$1; shift
+
+	if [[ ! -x "$(command -v "$cmd")" ]]; then
+		echo "SKIP: $cmd not installed"
+		exit $ksft_skip
+	fi
+}
\ No newline at end of file
diff --git a/tools/testing/selftests/net/rtnetlink.sh b/tools/testing/selftests/net/rtnetlink.sh
index bdf6f10d0558..4a9ffd06990b 100755
--- a/tools/testing/selftests/net/rtnetlink.sh
+++ b/tools/testing/selftests/net/rtnetlink.sh
@@ -1306,12 +1306,9 @@ if [ "$(id -u)" -ne 0 ];then
 	exit $ksft_skip
 fi
 
-for x in ip tc;do
-	$x -Version 2>/dev/null >/dev/null
-	if [ $? -ne 0 ];then
-		end_test "SKIP: Could not run test without the $x tool"
-		exit $ksft_skip
-	fi
+#check for dependencies
+for x in ip tc ethtool;do
+	require_command $x
 done
 
 while getopts t:hvpP o; do
-- 
2.46.0

Re: [PATCH net v2] selftests: rtnetlink: add 'ethtool' as a dependency

[Jakub Kicinski]

On Mon,  9 Sep 2024 10:34:10 +0200 Ales Nezbeda wrote:
> Fixes: 3b5222e2ac57 ("selftests: rtnetlink: add MACsec offload tests")

Don't think it qualifies as a fix, it's an improvement.

> +require_command()
> +{
> +	local cmd=$1; shift
> +
> +	if [[ ! -x "$(command -v "$cmd")" ]]; then
> +		echo "SKIP: $cmd not installed"
> +		exit $ksft_skip
> +	fi
> +}

You can use net/forwarding's lib.sh in net/, altnames.sh already 
uses it.
-- 
pw-bot: cr

Re: [PATCH net v2] selftests: rtnetlink: add 'ethtool' as a dependency

[Ales Nezbeda]

On Tue, Sep 10, 2024 at 2:17 AM Jakub Kicinski <kuba@kernel.org> wrote:
> Don't think it qualifies as a fix, it's an improvement.

Well, if the `ethtool` is not present the test will fail with dubious
results that would indicate that the test failed due to the system not
supporting MACsec offload, which is not true. The error message
doesn't reflect what went wrong, so I thought about it as an issue
that is being fixed. If this is not the case please let me know,
because based on the docs 'Fixes: tag indicates that the patch fixes
an issue', which I would think that wrong error message is an issue. I
might be wrong here though, so if the definition of 'issue' is more
restrictive I can remove the 'Fixes:' tag.

> You can use net/forwarding's lib.sh in net/, altnames.sh already
> uses it.

I see, the problem (and probably should have mentioned it in the patch
itself) is that `rtnetlink.sh` is using one of the variables defined
in the `net/lib.sh` - specifically `ksft_skip`. Furthermore, I felt
like a more clean approach would be to add the `require_command()` to
the `net/lib.sh` so that other tests down the road could potentially
use it as well. Picking a different `lib.sh` would mean either
modifying the `rtnetlink.sh` around it (removing the `ksft_skip` - lot
of changes in unrelated parts and harder for review) or adding it to
the new `lib.sh` and at that point we are adding stuff to the lib
anyway.

Re: [PATCH net v2] selftests: rtnetlink: add 'ethtool' as a dependency

[Sabrina Dubroca]

Hi Jakub,

(this thread probably got buried in your inbox due to netconf/LPC)

2024-09-16, 11:18:13 +0200, Ales Nezbeda wrote:
> On Tue, Sep 10, 2024 at 2:17 AM Jakub Kicinski <kuba@kernel.org> wrote:
> > Don't think it qualifies as a fix, it's an improvement.
> 
> Well, if the `ethtool` is not present the test will fail with dubious
> results that would indicate that the test failed due to the system not
> supporting MACsec offload, which is not true. The error message
> doesn't reflect what went wrong, so I thought about it as an issue
> that is being fixed.

I also think it's a (pretty minor) fix, since the error message is
really misleading.

> If this is not the case please let me know,
> because based on the docs 'Fixes: tag indicates that the patch fixes
> an issue', which I would think that wrong error message is an issue. I
> might be wrong here though, so if the definition of 'issue' is more
> restrictive I can remove the 'Fixes:' tag.
> 
> > You can use net/forwarding's lib.sh in net/, altnames.sh already
> > uses it.
> 
> I see, the problem (and probably should have mentioned it in the patch
> itself) is that `rtnetlink.sh` is using one of the variables defined
> in the `net/lib.sh` - specifically `ksft_skip`.

net/forwarding/lib.sh seems to include net/lib.sh, and uses ksft_skip
too, so there should be no problem:

    source "$net_forwarding_dir/../lib.sh"
    
    ##############################################################################
    # Sanity checks
    
    check_tc_version()
    {
    	tc -j &> /dev/null
    	if [[ $? -ne 0 ]]; then
    		echo "SKIP: iproute2 too old; tc is missing JSON support"
    		exit $ksft_skip
    	fi
    }


> Furthermore, I felt
> like a more clean approach would be to add the `require_command()` to
> the `net/lib.sh` so that other tests down the road could potentially
> use it as well. Picking a different `lib.sh` would mean either

Moving require_command from net/forwarding/lib.sh to net/lib.sh also
seems pretty reasonable.

-- 
Sabrina