On Thu, Sep 03, 2020 at 04:53:31PM -0700, Sagi Grimberg wrote:
> Right now, only pci and loop have tests, hence these are
> the only ones that are allowed. The user can pass an env
> variable nvme_trtype and check for the necessary modules.
>
> This allows prepares us to support other transport types.
>
> Note that test 031 is designed to run only with nvme, hence
> it overrides the environment variable to nvme_trtype=pci.
Thanks, Sagi, this is a nice cleanup. Some comments below, though.
> Signed-off-by: Sagi Grimberg <sagi@xxxxxxxxxxx>
> ---
> tests/nvme/002 | 3 ++-
> tests/nvme/003 | 3 ++-
> tests/nvme/004 | 3 ++-
> tests/nvme/005 | 6 +++---
> tests/nvme/006 | 4 ++--
> tests/nvme/007 | 2 +-
> tests/nvme/008 | 4 ++--
> tests/nvme/009 | 2 +-
> tests/nvme/010 | 4 ++--
> tests/nvme/011 | 4 ++--
> tests/nvme/012 | 5 +++--
> tests/nvme/013 | 4 ++--
> tests/nvme/014 | 4 ++--
> tests/nvme/015 | 3 ++-
> tests/nvme/016 | 2 +-
> tests/nvme/017 | 2 +-
> tests/nvme/018 | 4 ++--
> tests/nvme/019 | 4 ++--
> tests/nvme/020 | 2 +-
> tests/nvme/021 | 4 ++--
> tests/nvme/022 | 4 ++--
> tests/nvme/023 | 4 ++--
> tests/nvme/024 | 4 ++--
> tests/nvme/025 | 4 ++--
> tests/nvme/026 | 4 ++--
> tests/nvme/027 | 4 ++--
> tests/nvme/028 | 4 ++--
> tests/nvme/029 | 4 ++--
> tests/nvme/030 | 5 ++---
> tests/nvme/031 | 5 ++---
> tests/nvme/032 | 4 ++++
> tests/nvme/rc | 19 +++++++++++++++++++
> 32 files changed, 80 insertions(+), 54 deletions(-)
>
> diff --git a/tests/nvme/002 b/tests/nvme/002
> index 07b7fdae2d39..aaa5ec4d729a 100755
> --- a/tests/nvme/002
> +++ b/tests/nvme/002
> @@ -10,7 +10,8 @@
> DESCRIPTION="create many subsystems and test discovery"
>
> requires() {
> - _have_program nvme && _have_modules loop nvme-loop nvmet && _have_configfs
> + _nvme_requires
> + _have_modules loop
Bash functions return the status of the last executed command, and the
requires function needs to return 0 on success and 1 on failure. So,
this is losing the return value of _nvme_requires. Just chain multiple
checks with && to fix this (here and the other places _nvme_requires was
added with other checks):
requires() {
_nvme_requires && _have_modules loop
}
> diff --git a/tests/nvme/010 b/tests/nvme/010
> index 2ed0f4871a30..53b97484615f 100755
> --- a/tests/nvme/010
> +++ b/tests/nvme/010
> @@ -10,8 +10,8 @@ DESCRIPTION="run data verification fio job on NVMeOF block device-backed ns"
> TIMED=1
>
> requires() {
> - _have_program nvme && _have_fio && \
> - _have_modules loop nvme-loop nvmet && _have_configfs
> + _nvme_requires
> + _have_fio _have_modules loop
Looks like these two got squashed into one command. It should be
_nvme_requires && _have_fio && _have_modules loop
> test() {
> diff --git a/tests/nvme/032 b/tests/nvme/032
> index 0d0d53b325e6..017d4a339971 100755
> --- a/tests/nvme/032
> +++ b/tests/nvme/032
> @@ -11,11 +11,15 @@
>
> . tests/nvme/rc
>
> +#restrict test to nvme-pci only
> +nvme_trtype=pci
> +
> DESCRIPTION="test nvme pci adapter rescan/reset/remove during I/O"
> QUICK=1
> CAN_BE_ZONED=1
>
> requires() {
> + _nvme_requires
> _have_fio
> }
>
> diff --git a/tests/nvme/rc b/tests/nvme/rc
> index 6ffa971b4308..320aa4b2b475 100644
> --- a/tests/nvme/rc
> +++ b/tests/nvme/rc
> @@ -6,6 +6,25 @@
>
> . common/rc
>
> +nvme_trtype=${nvme_trtype:-"loop"}
> +
> +_nvme_requires() {
> + _have_program nvme
> + case ${nvme_trtype} in
> + loop)
> + _have_modules nvmet nvme-core nvme-loop
> + _have_configfs
Same here, _have_modules nvmet nvme-core nvme-loop && _have_configfs.
> + ;;
> + pci)
> + _have_modules nvme nvme-core
> + ;;
> + *)
> + SKIP_REASON="unsupported nvme_trtype=${nvme_trtype}"
> + return 1
> + esac
> + return 0
This return swallows the return value of the checks. You can drop it.
> +}
> +
> group_requires() {
> _have_root
> }
> --
> 2.25.1
>
