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

[Sabrina Dubroca <sd@queasysnail.net>]

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