Received-SPF: pass (google.com: domain of linux-kernel+bounces-87099-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Precedence: bulk
MIME-Version: 1.0
References: <20240229155235.263157-1-laura.nao@collabora.com>
In-Reply-To: <20240229155235.263157-1-laura.nao@collabora.com>
From: Miguel Ojeda <miguel.ojeda.sandonis@gmail.com>
Date: Thu, 29 Feb 2024 17:44:48 +0100
Message-ID: <CANiq72n5VmPQL32smcWz9HJdNg_KfLLRM7syhtM+TiL+pPcEGQ@mail.gmail.com>
Subject: Re: [PATCH v5] kselftest: Add basic test for probing the rust sample modules
To: Laura Nao <laura.nao@collabora.com>
Cc: ojeda@kernel.org, alex.gaynor@gmail.com, wedsonaf@gmail.com, 
	shuah@kernel.org, usama.anjum@collabora.com, a.hindborg@samsung.com, 
	aliceryhl@google.com, benno.lossin@proton.me, bjorn3_gh@protonmail.com, 
	boqun.feng@gmail.com, gary@garyguo.net, kernel@collabora.com, 
	linux-kernel@vger.kernel.org, linux-kselftest@vger.kernel.org, 
	rust-for-linux@vger.kernel.org, kernel@valentinobst.de, 
	sergio.collado@gmail.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Thu, Feb 29, 2024 at 4:53=E2=80=AFPM Laura Nao <laura.nao@collabora.com>=
 wrote:
>
> Add new basic kselftest that checks if the available rust sample modules
> can be added and removed correctly.
>
> Signed-off-by: Laura Nao <laura.nao@collabora.com>
> Reviewed-by: Sergio Gonzalez Collado <sergio.collado@gmail.com>
> Reviewed-by: Muhammad Usama Anjum <usama.anjum@collabora.com>

Thanks for this Laura!

Replying here to what you wrote in v4:

> At first, I hadn't planned for the kselftest to skip entirely if only
> one of the two sample modules was missing. However, considering that
> this kselftest is designed to test all available sample modules, and
> given that both are enabled with the provided configuration file, I
> believe it's more logical to verify the presence of both modules before
> running the test. If either of them is missing, then we exit the test
> with a skip code. This also covers the case where rust is not available.

I guess it depends on what is the expected behavior in kselftests in
general and whether the user is expected to have merged the provided
`config` or not.

Also, what about modules being built-in / `--first-run` in `modprobe`?
`modprobe` by default may return successfully even if no module was
loaded (or even present, if it was builtin). In that case, is a
kselftest script supposed to succeed, skip or fail? I would say at the
least it should be "skip" (like it is done in the case where the
module is not found), and I wouldn't mind "fail" either (i.e. running
`modprobe` with `--first-run`).

In addition, what about module removal failures? Are they ignored on
purpose, e.g. because the kernel might not be configured with module
unloading? If it is possible to check whether `MODULE_UNLOAD` is
supported in the current config, it would be nice to check the removal
also worked. And if it is not supported, skipping the removal entirely.

Finally, what about the case where `RUST` isn't enabled? I think Shuah
mentioned it in a previous version.

> +KTAP_HELPERS=3D"${DIR}/../kselftest/ktap_helpers.sh"
> +if [ -e "$KTAP_HELPERS" ]; then
> +    source "$KTAP_HELPERS"
> +else
> +    echo "$KTAP_HELPERS file not found [SKIP]"
> +    exit 4
> +fi

I am not sure I understand this. In which situation could this happen?
The helpers should always be there, no? I tested this with `make
-C...../selftests install TARGETS=3Drust INSTALL_PATH=3D...` and it seems
to work in that case too.

To be clear, I agree with Shuah that we should test that everything is
working as expected. In fact, I would prefer to run with `-e` or, much
better, use something else than bash :) But if something should never
happen, should it be a skip? Shouldn't we just fail because the test
infrastructure is somehow missing?

Orthogonally, if we want the test, shouldn't this just test the
`source` command directly rather than a proxy (file existing)?

Thanks!

Cheers,
Miguel