on 6/24/2022 8:17 PM, Shinichiro Kawasaki wrote:
On Jun 24, 2022 / 17:59, Li, Zhijian wrote:
On Jun 24, 2022 / 15:50, Xiao Yang wrote:
In _have_kernel_option(), SKIP_REASON = "kernel option NVME_MULTIPATH
has not been enabled" is expected but all nvmeof-mp tests are skipped
due to the SKIP_REASON. For example: >
----------------------------------------------------- > ./check
nvmeof-mp/001 > nvmeof-mp/*** [not run] > kernel option NVME_MULTIPATH
has not been enabled >
----------------------------------------------------- > > Avoid the
issue by unsetting the SKIP_REASON. > > Signed-off-by: Xiao Yang
<yangx.jy@xxxxxxxxxxx>
Good catch. Thanks!
This issue was triggered by the commit 7ae143852f6c ("common/rc: don't unset
previous SKIP_REASON in _have_kernel_option()"). So let's add a "Fixes" tag to
note it.
--- > tests/nvmeof-mp/rc | 5 +++++ > 1 file changed, 5 insertions(+) >
diff --git a/tests/nvmeof-mp/rc b/tests/nvmeof-mp/rc > index
dcb2e3c..9c91f8c 100755 > --- a/tests/nvmeof-mp/rc > +++
b/tests/nvmeof-mp/rc > @@ -24,6 +24,11 @@ and multipathing has been
enabled in the nvme_core kernel module" > return > fi > > + # In
_have_kernel_option(), SKIP_REASON = "kernel option > + # NVME_MULTIPATH
has not been enabled" is expected so > + # avoid skipping tests by
unsetting the SKIP_REASON
Can we have shorter comment? Like:
# Avoid test skip due to SKIP_REASON set by _have_kernel_option().
+ unset SKIP_REASON
Well, IMO it's not always correct to unsetSKIP_REASON, for example, if the
OS didn't have kernel config file, we should report the test as 'not run'
Actually, this group_requires() in tests/nvmeof-mp/rc has another
_have_kernel_option() call for DM_UEVENT. It will catch and report the "no
kernel config "case. So, I think it is ok to apply Xiao's solution to fix
the current issue.
I think Li's point is still valid, but let's take action for it later. One more
point I want to mention is that "unset SKIP_REASON" is not a good practice. To
seek for the best shape, I can think of following changes:
below changes sound good :)
1) Introduce _check_kernel_option(), which checks the specified kernel option
is defined, but does not set SKIP_RESAON. Using this, "unset SKIP_REASON" of
group_requires() in tests/nvme-of/rc (and tests/nvme/039) can be avoided.
2) Introduce _have_kernel_config_file() which sets SKIP_REASON when neither
/boot/config* nor /proc/config.gz is available. It can be called from the
group_requires() in tests/nvme-of/rc before _check_kernel_option() to ensure
the kernel option check is valid.
