Re: [PATCH blktests 3/5] check: Add TEST_DEV_PART_SYSFS variable

[Omar Sandoval <osandov@osandov.com>]

On Wed, Feb 20, 2019 at 05:12:29PM +0900, Shin'ichiro Kawasaki wrote:
> When partition devices are specified in TEST_DEV, TEST_DEV_SYSFS
> variable points to the sysfs paths of holder devices of the partition
> devices (e.g., /sys/block/sda). This sysfs path is different from the
> sysfs path of the partition devices (e.g., /sys/block/sda/sda1). For
> example, size parameters exist in both the holder device sysfs and
> the partition device sysfs with different values.
> 
> To allow test cases to access sysfs path of the partition devices,
> add TEST_DEV_PART_SYSFS variable. TEST_DEV_SYSFS is set as is to refer
> the sysfs path of the holder devices. If the TEST_DEV is not a partition
> device, an empty string is set to the TEST_DEV_PART_SYSFS variable.
> 
> Change _find_sysfs_dir() function to return the holder device sysfs as
> well as the partition device sysfs. The function obtains the canonical
> sysfs path, and if the device is a partition device, the function cut the
> last device name in the canonical sysfs path to obtain the holder device
> sysfs path.

This makes sense. A couple of small tweaks below.

> Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
> ---
>  check | 51 ++++++++++++++++++++++++++++++++++-----------------
>  new   | 16 ++++++++++++++--
>  2 files changed, 48 insertions(+), 19 deletions(-)
> 
> diff --git a/check b/check
> index f41ecba..e45b34f 100755
> --- a/check
> +++ b/check
> @@ -442,13 +442,19 @@ _run_test() {
>  				_warning "$TEST_NAME: fallback_device call failure"
>  				return 0
>  			fi
> -			if ! sysfs_dir="$(_find_sysfs_dir "$test_dev")"; then
> +
> +			local dirs
> +			local sysfs_dir
> +			local part_sysfs_dir
> +			if ! dirs=$(_find_sysfs_dir "$test_dev") ; then
>  				_warning "$TEST_NAME: could not find sysfs directory for ${test_dev}"
>  				cleanup_fallback_device
>  				return 0
>  			fi
> +			read -r sysfs_dir part_sysfs_dir < <(echo "${dirs}")

Let's rename _find_sysfs_dir to find_sysfs_dirs and make it set
TEST_DEV_SYSFS_DIRS["$test_dev"] and
TEST_DEV_PART_SYSFS_DIRS["$test_dev"] itself instead of returning the
string.

>  			TEST_DEVS=( "${test_dev}" )
>  			TEST_DEV_SYSFS_DIRS["$test_dev"]="$sysfs_dir"
> +			TEST_DEV_PART_SYSFS_DIRS["$test_dev"]="$part_sysfs_dir"
>  			FALLBACK_DEVICE=1
>  		fi
>  
> @@ -464,6 +470,7 @@ _run_test() {
>  		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"
>  				_output_notrun "$TEST_NAME => $(basename "$TEST_DEV")"
> @@ -483,6 +490,7 @@ _run_test() {
>  		if (( FALLBACK_DEVICE )); then
>  			cleanup_fallback_device
>  			unset TEST_DEV_SYSFS_DIRS["${TEST_DEVS[0]}"]
> +			unset TEST_DEV_PART_SYSFS_DIRS["${TEST_DEVS[0]}"]
>  			TEST_DEVS=()
>  		fi
>  
> @@ -507,6 +515,8 @@ _run_group() {
>  		for i in "${!TEST_DEVS[@]}"; do
>  			TEST_DEV="${TEST_DEVS[$i]}"
>  			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
>  				_output_notrun "${group}/*** => $(basename "$TEST_DEV")"
>  				unset TEST_DEVS["$i"]
> @@ -529,28 +539,31 @@ _run_group() {
>  
>  _find_sysfs_dir() {
>  	local test_dev="$1"
> +	local sysfs_path
>  	local major=$((0x$(stat -L -c '%t' "$test_dev")))
>  	local minor=$((0x$(stat -L -c '%T' "$test_dev")))
> -	local dev="$major:$minor"
> +	local sysdev_path="/sys/dev/block/${major}:${minor}"
>  
> -	local block_dir part_dir
> -	for block_dir in /sys/block/*; do
> -		if [[ $(cat "${block_dir}/dev") = "$dev" ]]; then
> -			echo "$block_dir"
> -			return
> -		fi
> -		for part_dir in "$block_dir"/*; do
> -			if [[ -r ${part_dir}/dev && $(cat "${part_dir}/dev") = "$dev" ]]; then
> -				echo "$block_dir"
> -				return
> -			fi
> -		done
> -	done
> +	# Get the canonical sysfs path
> +	if ! sysfs_path=/sys/dev/block/$(readlink "${sysdev_path}"); then

sysfs_path="$(realpath "/sys/dev/block/${major}:${minor}")" is a bit
shorter, does that still work?

> +		return 1
> +	fi
>  
> -	return 1
> +	if [[ -r "${sysfs_path}"/partition ]]; then
> +		# If the device is a partition device, cut the last device name
> +		# of the canonical sysfs path to access to the sysfs of its
> +		# holder device.
> +		#   e.g.   .../block/sda/sda1  ->  ...block/sda
> +		# Return both the holder device sysfs path and the partition
> +		# device sysfs path.
> +		echo "${sysfs_path%/*}" "${sysfs_path}"
> +	else
> +		echo "${sysfs_path}" ""
> +	fi
>  }
>  
>  declare -A TEST_DEV_SYSFS_DIRS
> +declare -A TEST_DEV_PART_SYSFS_DIRS
>  _check() {
>  	# shellcheck disable=SC2034
>  	SRCDIR="$(realpath src)"
> @@ -563,11 +576,15 @@ _check() {
>  			_error "${test_dev} is not a block device"
>  		fi
>  
> +		local dirs
>  		local sysfs_dir
> -		if ! sysfs_dir="$(_find_sysfs_dir "$test_dev")"; then
> +		local part_sysfs_dir
> +		if ! dirs=$(_find_sysfs_dir "$test_dev") ; then
>  			_error "could not find sysfs directory for ${test_dev}"
>  		fi
> +		read -r sysfs_dir part_sysfs_dir < <(echo "${dirs}")
>  		TEST_DEV_SYSFS_DIRS["$test_dev"]="$sysfs_dir"
> +		TEST_DEV_PART_SYSFS_DIRS["$test_dev"]="$part_sysfs_dir"
>  	done
>  
>  	local test_name group prev_group
> diff --git a/new b/new
> index d7d5f7c..24c066d 100755
> --- a/new
> +++ b/new
> @@ -80,7 +80,10 @@ group_requires() {
>  # 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).
> +# /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
> @@ -165,7 +168,10 @@ DESCRIPTION=""
>  # 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).
> +# /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
> @@ -207,6 +213,12 @@ DESCRIPTION=""
>  #    - \$TEST_DEV_SYSFS -- the sysfs directory of the device (e.g.,
>  #                         /sys/block/sda). In general, you should use the
>  #                         _test_dev_queue_{get,set} helpers.
> +#                         If the device is a partition device, the sysfs
> +#                         directory of its holder device is set.
> +#    - \$TEST_DEV_PART_SYSFS -- the sysfs directory of the device if the device
> +#                               is a partition device (e.g.,
> +#                               /sys/block/sda/sda1). Empty string is set if
> +#                               the device is not a partition device.
>  test() {
>  	echo "Running \${TEST_NAME}"
>  
> -- 
> 2.20.1
>