On 3/5/19 7:34 AM, Omar Sandoval wrote:

On Wed, Feb 20, 2019 at 05:12:29PM +0900, Shin'ichiro Kawasaki wrote:

quoted

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.

quoted

Signed-off-by: Shin'ichiro Kawasaki <redacted>
---
  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.

OK. Will revise the patch.

quoted

  			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?

Yes, the suggested code works well. Will revise the patch.

I will resend this patch and the following two patches as a new series.
Thank you for the review!

-- 
Best Regards,
Shin'ichiro Kawasaki