On 3/5/19 7:34 AM, Omar Sandoval wrote:
> 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 <[email protected]>
>> ---
>> 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.
>
>> 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
