netdev - Re: [PATCH net-next v2 2/2] selftests: net: ksft: support marking tests as disruptive

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <878qxh7mf4.fsf@nvidia.com>
Date: Wed, 31 Jul 2024 13:55:13 +0200
From: Petr Machata <petrm@...dia.com>
To: Stanislav Fomichev <sdf@...ichev.me>
CC: <netdev@...r.kernel.org>, <davem@...emloft.net>, <edumazet@...gle.com>,
	<kuba@...nel.org>, <pabeni@...hat.com>, Shuah Khan <shuah@...nel.org>, "Joe
 Damato" <jdamato@...tly.com>, Petr Machata <petrm@...dia.com>,
	<linux-kselftest@...r.kernel.org>
Subject: Re: [PATCH net-next v2 2/2] selftests: net: ksft: support marking
 tests as disruptive

Stanislav Fomichev <sdf@...ichev.me> writes:

> Add new @ksft_disruptive decorator to mark the tests that might
> be disruptive to the system. Depending on how well the previous
> test works in the CI we might want to disable disruptive tests
> by default and only let the developers run them manually.
>
> KSFT framework runs disruptive tests by default. DISRUPTIVE=False
> environment (or config file) can be used to disable these tests.
> ksft_setup should be called by the test cases that want to use
> new decorator (ksft_setup is only called via NetDrvEnv/NetDrvEpEnv for now).

Is that something that tests would want to genuinely do, manage this
stuff by hand? I don't really mind having the helper globally
accessible, but default I'd keep it inside env.py and expect others to
inherit appropriately.

> @@ -127,6 +129,36 @@ KSFT_RESULT_ALL = True
>              KSFT_RESULT = False
>  
>  
> +def ksft_disruptive(func):
> +    """
> +    Decorator that marks the test as disruptive (e.g. the test
> +    that can down the interface). Disruptive tests can be skipped
> +    by passing DISRUPTIVE=False environment variable.
> +    """
> +
> +    @functools.wraps(func)
> +    def wrapper(*args, **kwargs):
> +        if not KSFT_DISRUPTIVE:
> +            raise KsftSkipEx(f"marked as disruptive")

Since this is a skip, it will fail the overall run. But that happened
because the user themselves set DISRUPTIVE=0 to avoid, um, disruption to
the system. I think it should either be xfail, or something else
dedicated that conveys the idea that we didn't run the test, but that's
fine.

Using xfail for this somehow doesn't seem correct, nothing failed. Maybe
we need KsftOmitEx, which would basically be an xfail with a more
appropriate name?

> +def ksft_setup(env):
> +    """
> +    Setup test framework global state from the environment.
> +    """
> +
> +    def get_bool(env, name):
> +        return env.get(name, "").lower() in ["true", "1"]

"yes" should alse be considered, for compatibility with the bash
selftests.

It's also odd that 0 is false, 1 is true, but 2 is false again. How
about something like this?

    def get_bool(env, name):
        value = env.get(name, "").lower()
        if value in ["yes", "true"]:
            return True
        if value in ["no", "false"]:
            return False

        try:
            return bool(int(value))
        except:
            raise something something invalid value

So that people at least know if they set it to nonsense that it's
nonsense?

Dunno. The bash selftests just take "yes" and don't care about being
very user friendly in that regard at all. _load_env_file() likewise
looks like it just takes strings and doesn't care about the semantics.
So I don't feel too strongly about this at all. Besides the "yes" bit,
that should be recognized.