Re: [PATCH v7 1/7] nvme: consolidate nvme requirements based on transport type

[Omar Sandoval <osandov@osandov.com>]

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@grimberg.me>
> ---
>  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
>