| Message ID | 20240909083410.60121-1-anezbeda@redhat.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net,v2] selftests: rtnetlink: add 'ethtool' as a dependency | expand |
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.
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.
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
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(-)