Skip tests based on SKIP_REASON, not return value

Currently, {,group_,device_}requires are required to both set
SKIP_REASON and return non-zero. This is somewhat redundant, and will be
inconsistent with skipping from test{,_device} that is about to be
implemented. Let's ignore the return value and always skip the test if
SKIP_REASON is set. The _have_foo and _test_dev_foo helpers should still
return so that they can be chained together.

check

@@ -423,6 +423,15 @@ _test_dev_is_zoned() {
 	return 0
 }
 
+_test_dev_is_not_zoned() {
+	if _test_dev_is_zoned; then
+		SKIP_REASON="${TEST_DEV} is a zoned block device"
+		return 1
+	fi
+	unset SKIP_REASON
+	return 0
+}
+
 _run_test() {
 	TEST_NAME="$1"
 	CAN_BE_ZONED=0
@@ -435,9 +444,12 @@ _run_test() {
 	. "tests/${TEST_NAME}"
 
 	if declare -fF test >/dev/null; then
-		if declare -fF requires >/dev/null && ! requires; then
-			_output_notrun "$TEST_NAME"
-			return 0
+		if declare -fF requires >/dev/null; then
+			requires
+			if [[ -v SKIP_REASON ]]; then
+				_output_notrun "$TEST_NAME"
+				return 0
+			fi
 		fi
 
 		RESULTS_DIR="$OUTPUT/nodev"
@@ -471,24 +483,30 @@ _run_test() {
 			return 0
 		fi
 
-		if declare -fF requires >/dev/null && ! requires; then
-			_output_notrun "$TEST_NAME"
-			return 0
+		if declare -fF requires >/dev/null; then
+			requires
+			if [[ -v SKIP_REASON ]]; then
+				_output_notrun "$TEST_NAME"
+				return 0
+			fi
 		fi
 
 		local ret=0
 		for TEST_DEV in "${TEST_DEVS[@]}"; do
 			TEST_DEV_SYSFS="${TEST_DEV_SYSFS_DIRS["$TEST_DEV"]}"
 			TEST_DEV_PART_SYSFS="${TEST_DEV_PART_SYSFS_DIRS["$TEST_DEV"]}"
-			if (( !CAN_BE_ZONED )) && _test_dev_is_zoned; then
-				SKIP_REASON="${TEST_DEV} is a zoned block device"
+			if (( !CAN_BE_ZONED )) && ! _test_dev_is_not_zoned; then
 				_output_notrun "$TEST_NAME => $(basename "$TEST_DEV")"
+				unset SKIP_REASON
 				continue
 			fi
-			unset SKIP_REASON
-			if declare -fF device_requires >/dev/null && ! device_requires; then
-				_output_notrun "$TEST_NAME => $(basename "$TEST_DEV")"
-				continue
+			if declare -fF device_requires >/dev/null; then
+				device_requires
+				if [[ -v SKIP_REASON ]]; then
+					_output_notrun "$TEST_NAME => $(basename "$TEST_DEV")"
+					unset SKIP_REASON
+					continue
+				fi
 			fi
 			RESULTS_DIR="$OUTPUT/$(basename "$TEST_DEV")"
 			if ! _call_test test_device; then
@@ -514,9 +532,12 @@ _run_group() {
 	# shellcheck disable=SC1090
 	. "tests/${group}/rc"
 
-	if declare -fF group_requires >/dev/null && ! group_requires; then
-		_output_notrun "${group}/***"
-		return 0
+	if declare -fF group_requires >/dev/null; then
+		group_requires
+		if [[ -v SKIP_REASON ]]; then
+			_output_notrun "${group}/***"
+			return 0
+		fi
 	fi
 
 	if declare -fF group_device_requires >/dev/null; then
@@ -526,9 +547,11 @@ _run_group() {
 			TEST_DEV_SYSFS="${TEST_DEV_SYSFS_DIRS["$TEST_DEV"]}"
 			# shellcheck disable=SC2034
 			TEST_DEV_PART_SYSFS="${TEST_DEV_PART_SYSFS_DIRS["$TEST_DEV"]}"
-			if ! group_device_requires; then
+			group_device_requires
+			if [[ -v SKIP_REASON ]]; then
 				_output_notrun "${group}/*** => $(basename "$TEST_DEV")"
 				unset TEST_DEVS["$i"]
+				unset SKIP_REASON
 			fi
 		done
 		# Fix the array indices.

common/rc

@@ -139,7 +139,7 @@ _have_kernel_option() {
 	for f in /proc/config.gz /boot/config-$(uname -r); do
 		[ -e "$f" ] || continue
 		if zgrep -q "^CONFIG_${opt}=[my]$" "$f"; then
-			SKIP_REASON=""
+			unset SKIP_REASON
 			return 0
 		else
 			SKIP_REASON="kernel option $opt has not been enabled"

new

@@ -63,32 +63,30 @@ if [[ ! -e tests/${group} ]]; then
 # . common/foo
 
 # TODO: if this test group has any extra requirements, it should define a
-# group_requires() function. If tests in this group can be run,
-# group_requires() should return 0. Otherwise, it should return non-zero and
-# set the \$SKIP_REASON variable.
+# group_requires() function. If tests in this group cannot be run,
+# group_requires() should set the \$SKIP_REASON variable.
 #
 # Usually, group_requires() just needs to check that any necessary programs and
 # kernel features are available using the _have_foo helpers. If
-# group_requires() returns non-zero, all tests in this group will be skipped.
+# group_requires() sets \$SKIP_REASON, all tests in this group will be skipped.
 group_requires() {
 	_have_root
 }
 
 # TODO: if this test group has extra requirements for what devices it can be
 # run on, it should define a group_device_requires() function. If tests in this
-# group can be run on the test device, it should return zero. Otherwise, it
-# should return non-zero and set the \$SKIP_REASON variable. \$TEST_DEV is the
-# full path of the block device (e.g., /dev/nvme0n1 or /dev/sda1), and
-# \$TEST_DEV_SYSFS is the sysfs path of the disk (not the partition, e.g.,
-# /sys/block/nvme0n1 or /sys/block/sda). If the target device is a partition
-# device, \$TEST_DEV_PART_SYSFS is the sysfs path of the partition device
-# (e.g., /sys/block/nvme0n1/nvme0n1p1 or /sys/block/sda/sda1). Otherwise,
-# \$TEST_DEV_PART_SYSFS is an empty string.
+# group cannot be run on the test device, it should set the \$SKIP_REASON
+# variable. \$TEST_DEV is the full path of the block device (e.g., /dev/nvme0n1
+# or /dev/sda1), and \$TEST_DEV_SYSFS is the sysfs path of the disk (not the
+# partition, e.g., /sys/block/nvme0n1 or /sys/block/sda). If the target device
+# is a partition device, \$TEST_DEV_PART_SYSFS is the sysfs path of the
+# partition device (e.g., /sys/block/nvme0n1/nvme0n1p1 or /sys/block/sda/sda1).
+# Otherwise, \$TEST_DEV_PART_SYSFS is an empty string.
 #
 # Usually, group_device_requires() just needs to check that the test device is
 # the right type of hardware or supports any necessary features using the
-# _test_dev_foo helpers. If group_device_requires() returns non-zero, all tests
-# in this group will be skipped on that device.
+# _test_dev_foo helpers. If group_device_requires() sets \$SKIP_REASON, all
+# tests in this group will be skipped on that device.
 # group_device_requires() {
 # 	_test_dev_is_foo && _test_dev_supports_bar
 # }
@@ -153,29 +151,27 @@ DESCRIPTION=""
 # CAN_BE_ZONED=1
 
 # TODO: if this test has any extra requirements, it should define a requires()
-# function. If the test can be run, requires() should return 0. Otherwise, it
-# should return non-zero and set the \$SKIP_REASON variable. Usually,
-# requires() just needs to check that any necessary programs and kernel
-# features are available using the _have_foo helpers. If requires() returns
-# non-zero, the test is skipped.
+# function. If the test cannot be run, requires() should set the \$SKIP_REASON
+# variable. Usually, requires() just needs to check that any necessary programs
+# and kernel features are available using the _have_foo helpers. If requires()
+# sets \$SKIP_REASON, the test is skipped.
 # requires() {
 # 	_have_foo
 # }
 
 # TODO: if this test has extra requirements for what devices it can be run on,
-# it should define a device_requires() function. If this test can be run on the
-# test device, it should return zero. Otherwise, it should return non-zero and
-# set the \$SKIP_REASON variable. \$TEST_DEV is the full path of the block
-# device (e.g., /dev/nvme0n1 or /dev/sda1), and \$TEST_DEV_SYSFS is the sysfs
-# path of the disk (not the partition, e.g., /sys/block/nvme0n1 or
+# it should define a device_requires() function. If this test cannot be run on
+# the test device, it should set \$SKIP_REASON. \$TEST_DEV is the full path of
+# the block device (e.g., /dev/nvme0n1 or /dev/sda1), and \$TEST_DEV_SYSFS is
+# the sysfs path of the disk (not the partition, e.g., /sys/block/nvme0n1 or
 # /sys/block/sda). If the target device is a partition device,
 # \$TEST_DEV_PART_SYSFS is the sysfs path of the partition device (e.g.,
 # /sys/block/nvme0n1/nvme0n1p1 or /sys/block/sda/sda1). Otherwise,
 # \$TEST_DEV_PART_SYSFS is an empty string.
 #
 # Usually, device_requires() just needs to check that the test device is the
 # right type of hardware or supports any necessary features using the
-# _test_dev_foo helpers. If device_requires() returns non-zero, the test will
+# _test_dev_foo helpers. If device_requires() sets \$SKIP_REASON, the test will
 # be skipped on that device.
 # device_requires() {
 # 	_test_dev_is_foo && _test_dev_supports_bar

tests/meta/007

@@ -10,7 +10,6 @@ DESCRIPTION="skip in requires()"
 
 requires() {
 	SKIP_REASON="(╯°□°)╯︵ ┻━┻"
-	return 1
 }
 
 test() {

tests/meta/008

@@ -10,7 +10,6 @@ DESCRIPTION="skip in device_requires()"
 
 device_requires() {
 	SKIP_REASON="(╯°□°)╯︵ $TEST_DEV ┻━┻"
-	return 1
 }
 
 test_device() {

tests/meta/rc

@@ -9,17 +9,13 @@
 group_requires() {
 	if [[ "${META_REQUIRES_SKIP:-}" ]]; then
 		SKIP_REASON="META_REQUIRES_SKIP was set"
-		return 1
 	fi
-	return 0
 }
 
 group_device_requires() {
 	if [[ "${META_DEVICE_REQUIRES_SKIP:-}" ]]; then
 		SKIP_REASON="META_DEVICE_REQUIRES_SKIP was set"
-		return 1
 	fi
-	return 0
 }
 
 _have_writeable_kmsg() {

tests/nvmeof-mp/rc

@@ -19,10 +19,10 @@ group_requires() {
 	# option.
 	if _have_kernel_option NVME_MULTIPATH; then
 		SKIP_REASON="CONFIG_NVME_MULTIPATH has been set in .config"
-		return 1
+		return
 	fi
 
-	_have_configfs || return $?
+	_have_configfs || return
 	required_modules=(
 		dm_multipath
 		dm_queue_length
@@ -38,30 +38,30 @@ group_requires() {
 		scsi_dh_emc
 		scsi_dh_rdac
 	)
-	_have_modules "${required_modules[@]}" || return 1
+	_have_modules "${required_modules[@]}" || return
 
 	for p in mkfs.ext4 mkfs.xfs multipath multipathd pidof; do
-		_have_program "$p" || return $?
+		_have_program "$p" || return
 	done
 
-	_multipathd_version_ge 0.7.0 || return $?
+	_multipathd_version_ge 0.7.0 || return
 
-	_have_root || return $?
+	_have_root || return
 
-	_have_kernel_option DM_UEVENT || return $?
+	_have_kernel_option DM_UEVENT || return
 
 	# shellcheck disable=SC2043
 	for name in multipathd; do
 		if pidof "$name" >/dev/null; then
 			SKIP_REASON="$name must be stopped before the nvmeof-mp tests are run"
-			return 1
+			return
 		fi
 	done
 	if [ -e /etc/multipath.conf ] &&
 	    ! diff -q /etc/multipath.conf tests/nvmeof-mp/multipath.conf >&/dev/null
 	then
 		SKIP_REASON="/etc/multipath.conf already exists"
-		return 1
+		return
 	fi
 }
 

tests/srp/015

@@ -8,10 +8,9 @@ DESCRIPTION="File I/O on top of multipath concurrently with logout and login (mq
 TIMED=1
 
 requires() {
-	_have_modules siw
-	_have_kver 5 5
-	# See also commit 4336c5821a7b ("rdma: add 'link add/delete' commands").
-	_have_iproute2 190404
+	# See also iproute commit 4336c5821a7b ("rdma: add 'link add/delete'
+	# commands").
+	_have_modules siw && _have_kver 5 5 && _have_iproute2 190404
 }
 
 test_disconnect_repeatedly() {

tests/srp/rc

@@ -29,10 +29,10 @@ is_lio_configured() {
 group_requires() {
 	local m name p required_modules
 
-	_have_configfs || return $?
+	_have_configfs || return
 	if is_lio_configured; then
 		SKIP_REASON="LIO must be unloaded before the SRP tests are run"
-		return 1
+		return
 	fi
 	required_modules=(
 		dm_multipath
@@ -56,31 +56,31 @@ group_requires() {
 		target_core_iblock
 		target_core_mod
 	)
-	_have_modules "${required_modules[@]}" || return 1
+	_have_modules "${required_modules[@]}" || return
 
 	for p in mkfs.ext4 mkfs.xfs multipath multipathd pidof sg_reset; do
-		_have_program "$p" || return $?
+		_have_program "$p" || return
 	done
 
-	_multipathd_version_ge 0.7.0 || return $?
+	_multipathd_version_ge 0.7.0 || return
 
-	_have_root || return $?
+	_have_root || return
 
-	_have_src_program discontiguous-io || return $?
+	_have_src_program discontiguous-io || return
 
-	_have_kernel_option DM_UEVENT || return $?
+	_have_kernel_option DM_UEVENT || return
 
 	for name in srp_daemon multipathd; do
 		if pidof "$name" >/dev/null; then
 			SKIP_REASON="$name must be stopped before the SRP tests are run"
-			return 1
+			return
 		fi
 	done
 	if [ -e /etc/multipath.conf ] &&
 	    ! diff -q /etc/multipath.conf tests/srp/multipath.conf >&/dev/null
 	then
 		SKIP_REASON="/etc/multipath.conf already exists"
-		return 1
+		return
 	fi
 }
 

tests/zbd/rc

@@ -12,11 +12,9 @@
 #
 
 group_requires() {
-	_have_root || return $?
-	_have_program blkzone || return $?
-	_have_program dd || return $?
-	_have_kernel_option BLK_DEV_ZONED || return $?
-	_have_modules null_blk && _have_module_param null_blk zoned
+	_have_root && _have_program blkzone && _have_program dd &&
+		_have_kernel_option BLK_DEV_ZONED && _have_modules null_blk &&
+		_have_module_param null_blk zoned
 }
 
 group_device_requires() {