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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <Zw5WRhrNqJCZp1mf@hog>
Date: Tue, 15 Oct 2024 13:47:18 +0200
From: Sabrina Dubroca <sd@...asysnail.net>
To: Jakub Kicinski <kuba@...nel.org>, Ales Nezbeda <anezbeda@...hat.com>
Cc: netdev@...r.kernel.org, sdf@...ichev.me, davem@...emloft.net
Subject: Re: [PATCH net v2] selftests: rtnetlink: add 'ethtool' as a
 dependency

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@...nel.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