Received-SPF: pass (google.com: domain of linux-kernel+bounces-88616-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
From: Laura Nao <laura.nao@collabora.com>
To: laura.nao@collabora.com
Cc: a.hindborg@samsung.com,
	alex.gaynor@gmail.com,
	aliceryhl@google.com,
	benno.lossin@proton.me,
	bjorn3_gh@protonmail.com,
	boqun.feng@gmail.com,
	gary@garyguo.net,
	kernel@collabora.com,
	kernel@valentinobst.de,
	linux-kernel@vger.kernel.org,
	linux-kselftest@vger.kernel.org,
	ojeda@kernel.org,
	rust-for-linux@vger.kernel.org,
	sergio.collado@gmail.com,
	shuah@kernel.org,
	usama.anjum@collabora.com,
	wedsonaf@gmail.com
Subject: Re: [PATCH v5] kselftest: Add basic test for probing the rust sample modules
Date: Fri,  1 Mar 2024 16:22:32 +0100
Message-Id: <20240301152232.122399-1-laura.nao@collabora.com>
In-Reply-To: <20240229155235.263157-1-laura.nao@collabora.com>
References: <20240229155235.263157-1-laura.nao@collabora.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Hi Miguel,

On 2/29/24 17:44, Miguel Ojeda wrote:
> On Thu, Feb 29, 2024 at 4:53 PM 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.
> 

It's my understanding (and please correct if I'm wrong) that when a 
kselftest is shipped with a config file, that config file should be 
treated as a requirement for the test and the user is expected to use it
(running make kselftest-merge). I agree the script shouldn't blow up if
the user doesn't though, so it still makes sense to gracefully skip the
test when the requirements are not met.

> 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`).
>

This makes me realize that I should probably put these in the config
too:

CONFIG_MODULES=y
CONFIG_MODULE_UNLOAD=y

Adding --first-time (you meant --first-time, right?) definitely makes
sense, thanks for the pointer. I think having the modules being built-in
should be treated as a skip, same as when they are not there at all.

So something like this:

 for sample in "${rust_sample_modules[@]}"; do
-    if ! /sbin/modprobe -n -q "$sample"; then
+    if ! /sbin/modprobe -n -q --first-time "$sample"; then
         ktap_skip_all "module $sample is not found in /lib/modules/$(uname -r)"
         exit "$KSFT_SKIP"
     fi

will cover both cases.
 
> 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.
> 

I think it's safe to assume no other module will depend on the sample
rust modules, so is there any other reason unloading the modules 
might fail apart from MODULE_UNLOAD not being enabled? If not, then I
think we should just check if the removal worked and continue/skip the
test accordingly.

I can't just simply skip all tests like this though:

 for sample in "${rust_sample_modules[@]}"; do
     if /sbin/modprobe -q "$sample"; then
-        /sbin/modprobe -q -r "$sample"
+        if ! /sbin/modprobe -q -r "$sample"; then
+            ktap_skip_all "Failed to unload module $sample, please enable CONFIG_MODULE_UNLOAD"
+            exit "$KSFT_SKIP"
+        fi
         ktap_test_pass "$sample"
     else
         ktap_test_fail "$sample"

as the test plan has already been printed by then.
I'll need to rework the script a bit to skip the test upon errors on 
module removal.

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

When rust is not enabled, no sample module is enabled either so the test
would still catch this in the first `if ! /sbin/modprobe -n -q --first-time
"$sample"` block and exit with the skip code.

If we need more granularity on the feedback provided to the user (i.e.
indication on what particular options are missing), then I guess we 
could check the current kernel config (/proc/config.gz) and skip the
entire test if any required config is missing. However, this adds an 
extra dependency on CONFIG_IKCONFIG=y and CONFIG_IKCONFIG_PROC=y.

Any advice on the best approach here?

>> +KTAP_HELPERS="${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=rust INSTALL_PATH=...` 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?
>

Kselftest exit codes are predefined
(https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/tools/testing/selftests/kselftest.h?h=v6.8-rc6#n74),
so if we use `set -e` and source a missing file we end up returning "1"
as if the test was run and failed. With this check we're sure to return
a value that makes sense in the event the helpers file ever gets moved.
 
> Orthogonally, if we want the test, shouldn't this just test the
> `source` command directly rather than a proxy (file existing)?
> 

Sure, checking the return value for source also makes sense.

Thanks!

Best,
Laura