Re: [PATCH v5 4/6] fstests: common/sysfs: add new file sysfs and helpers

[Anand Jain <anand.jain@oracle.com>]

On 8/4/25 01:22, Darrick J. Wong wrote:
> On Mon, Apr 07, 2025 at 11:48:18AM +0800, Anand Jain wrote:
>> Introduce `verify_sysfs_syntax()` and `_require_fs_sysfs_attr_policy()`
>> to verify whether a sysfs attribute rejects invalid input arguments
>> during writes.
>>
>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>> ---
>>   common/sysfs | 141 +++++++++++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 141 insertions(+)
>>   create mode 100644 common/sysfs
>>
>> diff --git a/common/sysfs b/common/sysfs
>> new file mode 100644
>> index 000000000000..9f2d77c6b1f5
>> --- /dev/null
>> +++ b/common/sysfs
>> @@ -0,0 +1,141 @@
>> +##/bin/bash
>> +# SPDX-License-Identifier: GPL-2.0+
>> +# Copyright (c) 2025 Oracle.  All Rights Reserved.
>> +#
>> +# Common/sysfs file for the sysfs related helper functions.
>> +
>> +# Test for the existence of a policy at /sys/fs/$FSTYP/$DEV/$ATTR
>> +#
>> +# All arguments are necessary, and in this order:
>> +#  - dev: device name, e.g. $SCRATCH_DEV
>> +#  - attr: path name under /sys/fs/$FSTYP/$dev
>> +#  - policy: policy within /sys/fs/$FSTYP/$dev
>> +#
>> +# Usage example:
>> +#   _has_fs_sysfs_attr_policy /dev/mapper/scratch-dev read_policy round-robin
>> +_has_fs_sysfs_attr_policy()
>> +{
>> +	local dev=$1
>> +	local attr=$2
>> +	local policy=$3
>> +
>> +	if [ ! -b "$dev" -o -z "$attr" -o -z "$policy" ]; then
>> +		_fail \
>> +	     "Usage: _has_fs_sysfs_attr_policy <mounted_device> <attr> <policy>"
>> +	fi
> 
> Shouldn't this return 1 if the parameters are not fully specified?
> 

Do you mean, instead of using _fail, just echo "Usage" and return 1?

>> +
>> +	local dname=$(_fs_sysfs_dname $dev)
>> +	test -e /sys/fs/${FSTYP}/${dname}/${attr}
> 
> What is the point of testing path existence here if the function doesn't
> actually change its behavior?
> 


Ah, I missed the `if`—updated it locally now.

@@ -25,7 +25,9 @@ _has_fs_sysfs_attr_policy()
         fi

         local dname=$(_fs_sysfs_dname $dev)
-       test -e /sys/fs/${FSTYP}/${dname}/${attr}
+       if test -e /sys/fs/${FSTYP}/${dname}/${attr}; then
+               return 1
+       fi

         cat /sys/fs/${FSTYP}/${dname}/${attr} | grep -q ${policy}
  }


>> +
>> +	cat /sys/fs/${FSTYP}/${dname}/${attr} | grep -q ${policy}
>> +}
>> +
>> +# Require the existence of a sysfs entry at /sys/fs/$FSTYP/$DEV/$ATTR
>> +# and value in it contains $policy
>> +# All arguments are necessary, and in this order:
>> +#  - dev: device name, e.g. $SCRATCH_DEV
>> +#  - attr: path name under /sys/fs/$FSTYP/$dev
>> +#  - policy: mentioned in /sys/fs/$FSTYP/$dev/$attr
>> +#
>> +# Usage example:
>> +#   _require_fs_sysfs_attr_policy /dev/mapper/scratch-dev read_policy round-robin
>> +_require_fs_sysfs_attr_policy()
>> +{
>> +	_has_fs_sysfs_attr_policy "$@" && return
>> +
>> +	local dev=$1
>> +	local attr=$2
>> +	local policy=$3
>> +	local dname=$(_fs_sysfs_dname $dev)
>> +
>> +	_notrun "This test requires /sys/fs/${FSTYP}/${dname}/${attr} ${policy}"
>> +}
>> +
>> +_set_sysfs_policy()
>> +{
>> +	local dev=$1
>> +	local attr=$2
>> +	shift
>> +	shift
>> +	local policy=$@
>> +
>> +	_set_fs_sysfs_attr $dev $attr ${policy}
>> +
>> +	case "$FSTYP" in
>> +	btrfs)
>> +		_get_fs_sysfs_attr $dev $attr | grep -q "[${policy}]"
>> +		if [[ $? != 0 ]]; then
>> +			echo "Setting sysfs $attr $policy failed"
>> +		fi
>> +		;;
>> +	*)
>> +		_fail \
>> +"sysfs syntax verification for '${attr}' '${policy}' for '${FSTYP}' is not implemented"
>> +		;;
>> +	esac
>> +}
>> +
>> +_set_sysfs_policy_must_fail()
>> +{
>> +	local dev=$1
>> +	local attr=$2
>> +	shift
>> +	shift
>> +	local policy=$@
>> +
>> +	_set_fs_sysfs_attr $dev $attr ${policy} | _filter_sysfs_error \
>> +							   | tee -a $seqres.full
>> +}
>> +
>> +# Verify sysfs attribute rejects invalid input.
>> +# Usage syntax:
>> +#   _verify_sysfs_syntax <$dev> <$attr> <$policy> [$value]
>> +# Examples:
>> +#   _verify_sysfs_syntax $TEST_DEV read_policy pid
>> +#   _verify_sysfs_syntax $TEST_DEV read_policy round-robin 4k
>> +# Note:
>> +#  Process must call . ./common/filter
>> +_verify_sysfs_syntax()
>> +{
>> +	local dev=$1
>> +	local attr=$2
>> +	local policy=$3
>> +	local value=$4
>> +
>> +	# Do this in the test case so that we know its prerequisites.
>> +	# '_require_fs_sysfs_attr_policy $TEST_DEV $attr $policy'
> 
> But it's commented out ... ?

Yeah, I commented it out since it's just an example.
I think I need to rephrase the comment.

Thanks, Anand

> 
> --D
> 
>> +
>> +	# Test policy specified wrongly. Must fail.
>> +	_set_sysfs_policy_must_fail $dev $attr "'$policy $policy'"
>> +	_set_sysfs_policy_must_fail $dev $attr "'$policy t'"
>> +	_set_sysfs_policy_must_fail $dev $attr "' '"
>> +	_set_sysfs_policy_must_fail $dev $attr "'${policy} n'"
>> +	_set_sysfs_policy_must_fail $dev $attr "'n ${policy}'"
>> +	_set_sysfs_policy_must_fail $dev $attr "' ${policy}'"
>> +	_set_sysfs_policy_must_fail $dev $attr "' ${policy} '"
>> +	_set_sysfs_policy_must_fail $dev $attr "'${policy} '"
>> +	_set_sysfs_policy_must_fail $dev $attr _${policy}
>> +	_set_sysfs_policy_must_fail $dev $attr ${policy}_
>> +	_set_sysfs_policy_must_fail $dev $attr _${policy}_
>> +	_set_sysfs_policy_must_fail $dev $attr ${policy}:
>> +	# Test policy longer than 32 chars fails stable.
>> +	_set_sysfs_policy_must_fail $dev $attr 'jfdkkkkjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjffjfjfffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff'
>> +
>> +	# Test policy specified correctly. Must pass.
>> +	_set_sysfs_policy $dev $attr $policy
>> +
>> +	# If the policy has no value return
>> +	if [[ -z $value ]]; then
>> +		return
>> +	fi
>> +
>> +	# Test value specified wrongly. Must fail.
>> +	_set_sysfs_policy_must_fail $dev $attr "'$policy: $value'"
>> +	_set_sysfs_policy_must_fail $dev $attr "'$policy:$value '"
>> +	_set_sysfs_policy_must_fail $dev $attr "'$policy:$value typo'"
>> +	_set_sysfs_policy_must_fail $dev $attr "'$policy:${value}typo'"
>> +	_set_sysfs_policy_must_fail $dev $attr "'$policy :$value'"
>> +
>> +	# Test policy and value all specified correctly. Must pass.
>> +	_set_sysfs_policy $dev $attr $policy:$value
>> +}
>> -- 
>> 2.47.0
>>
>>