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:
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.
--
Shin'ichiro Kawasaki
