[PATCH v5 0/6] fstests: btrfs: add test case to validate sysfs input arguments

[PATCH v5 0/6] fstests: btrfs: add test case to validate sysfs input arguments

[Anand Jain]

v5:
- Added `rb`.
- New patch (2/6) for unset `seqres` (can merge into 1/6, but skipped as 1/6 got `rb`).
- Fixed `git am` whitespace.
- Renamed patch (was: `fstests: common/rc: add sysfs argument verification helpers`).
- Prefixed helpers function in common/sysfs with `_`.
- Updated function names in last two patches.

v4:
https://lore.kernel.org/fstests/cover.1740721626.git.anand.jain@oracle.com/

v3:
https://lore.kernel.org/fstests/b297a34f-4c09-48bb-86a3-fea50c364ba8@oracle.com/

v2:
https://lore.kernel.org/fstests/cover.1738752716.git.anand.jain@oracle.com/

v1:
https://lwn.net/ml/all/cover.1738161075.git.anand.jain@oracle.com/

Anand Jain (6):
  fstests: common/rc: set_fs_sysfs_attr: redirect errors to stdout
  fstests: common/rc: fix unset seqres in _set_fs_sysfs_attr
  fstests: filter: helper for sysfs error filtering
  fstests: common/sysfs: add new file sysfs and helpers
  fstests: btrfs: testcase for sysfs policy syntax verification
  fstests: btrfs: testcase for sysfs chunk_size attribute validation

 common/filter       |   9 +++
 common/rc           |  10 +++-
 common/sysfs        | 141 ++++++++++++++++++++++++++++++++++++++++++++
 tests/btrfs/329     |  19 ++++++
 tests/btrfs/329.out |  19 ++++++
 tests/btrfs/334     |  19 ++++++
 tests/btrfs/334.out |  14 +++++
 7 files changed, 230 insertions(+), 1 deletion(-)
 create mode 100644 common/sysfs
 create mode 100755 tests/btrfs/329
 create mode 100644 tests/btrfs/329.out
 create mode 100755 tests/btrfs/334
 create mode 100644 tests/btrfs/334.out

-- 
2.47.0

[PATCH v5 1/6] fstests: common/rc: set_fs_sysfs_attr: redirect errors to stdout

[Anand Jain]

Redirect sysfs write errors to stdout as a preparatory patch to enable
testing of expected sysfs write failures. Also, log the executed
sysfs write command and its failure if any to seqres.full for better
debugging and traceability.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
Reviewed-by: Zorro Lang <zlang@redhat.com>
---
 common/rc | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/common/rc b/common/rc
index 16d627e1bdd4..e89eee5de840 100644
--- a/common/rc
+++ b/common/rc
@@ -5208,7 +5208,8 @@ _set_fs_sysfs_attr()
 
 	local dname=$(_fs_sysfs_dname $dev)
 
-	echo "$content" > /sys/fs/${FSTYP}/${dname}/${attr}
+	echo "echo '$content' 2>&1 > /sys/fs/${FSTYP}/${dname}/${attr}" >> $seqres.full
+	echo "$content" 2>&1 > /sys/fs/${FSTYP}/${dname}/${attr} | tee -a $seqres.full
 }
 
 # Print the content of /sys/fs/$FSTYP/$DEV/$ATTR
-- 
2.47.0

[PATCH v5 2/6] fstests: common/rc: fix unset seqres in _set_fs_sysfs_attr

[Anand Jain]

Ensure logs don’t write to a `.full` file when `_set_fs_sysfs_attr()`
is called during setup (before a testcase) in XFS due to unset seqres.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 common/rc | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/common/rc b/common/rc
index e89eee5de840..ca1d13ca1f0b 100644
--- a/common/rc
+++ b/common/rc
@@ -5201,6 +5201,13 @@ _set_fs_sysfs_attr()
 	local attr=$1
 	shift
 	local content="$*"
+	local logfile="/dev/null"
+
+	# This function may be called outside a testcase during setup,
+	# so seqres might not be set.
+	if [[ -v seqres ]]; then
+		logfile="$seqres.full"
+	fi
 
 	if [ ! -b "$dev" -o -z "$attr" -o -z "$content" ];then
 		_fail "Usage: _set_fs_sysfs_attr <mounted_device> <attr> <content>"
@@ -5208,8 +5215,8 @@ _set_fs_sysfs_attr()
 
 	local dname=$(_fs_sysfs_dname $dev)
 
-	echo "echo '$content' 2>&1 > /sys/fs/${FSTYP}/${dname}/${attr}" >> $seqres.full
-	echo "$content" 2>&1 > /sys/fs/${FSTYP}/${dname}/${attr} | tee -a $seqres.full
+	echo "echo '$content' 2>&1 > /sys/fs/${FSTYP}/${dname}/${attr}" >> $logfile
+	echo "$content" 2>&1 > /sys/fs/${FSTYP}/${dname}/${attr} | tee -a $logfile
 }
 
 # Print the content of /sys/fs/$FSTYP/$DEV/$ATTR
-- 
2.47.0

Re: [PATCH v5 2/6] fstests: common/rc: fix unset seqres in _set_fs_sysfs_attr

[Darrick J. Wong]

On Mon, Apr 07, 2025 at 11:48:16AM +0800, Anand Jain wrote:
> Ensure logs don’t write to a `.full` file when `_set_fs_sysfs_attr()`
> is called during setup (before a testcase) in XFS due to unset seqres.
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
>  common/rc | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/common/rc b/common/rc
> index e89eee5de840..ca1d13ca1f0b 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -5201,6 +5201,13 @@ _set_fs_sysfs_attr()
>  	local attr=$1
>  	shift
>  	local content="$*"
> +	local logfile="/dev/null"

If we're not actually running a test, should the error output go to
$check.full instead?

--D

> +
> +	# This function may be called outside a testcase during setup,
> +	# so seqres might not be set.
> +	if [[ -v seqres ]]; then
> +		logfile="$seqres.full"
> +	fi
>  
>  	if [ ! -b "$dev" -o -z "$attr" -o -z "$content" ];then
>  		_fail "Usage: _set_fs_sysfs_attr <mounted_device> <attr> <content>"
> @@ -5208,8 +5215,8 @@ _set_fs_sysfs_attr()
>  
>  	local dname=$(_fs_sysfs_dname $dev)
>  
> -	echo "echo '$content' 2>&1 > /sys/fs/${FSTYP}/${dname}/${attr}" >> $seqres.full
> -	echo "$content" 2>&1 > /sys/fs/${FSTYP}/${dname}/${attr} | tee -a $seqres.full
> +	echo "echo '$content' 2>&1 > /sys/fs/${FSTYP}/${dname}/${attr}" >> $logfile
> +	echo "$content" 2>&1 > /sys/fs/${FSTYP}/${dname}/${attr} | tee -a $logfile
>  }
>  
>  # Print the content of /sys/fs/$FSTYP/$DEV/$ATTR
> -- 
> 2.47.0
> 
> 

Re: [PATCH v5 2/6] fstests: common/rc: fix unset seqres in _set_fs_sysfs_attr

[Anand Jain]

On 8/4/25 00:58, Darrick J. Wong wrote:
> On Mon, Apr 07, 2025 at 11:48:16AM +0800, Anand Jain wrote:
>> Ensure logs don’t write to a `.full` file when `_set_fs_sysfs_attr()`
>> is called during setup (before a testcase) in XFS due to unset seqres.
>>
>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>> ---
>>   common/rc | 11 +++++++++--
>>   1 file changed, 9 insertions(+), 2 deletions(-)
>>
>> diff --git a/common/rc b/common/rc
>> index e89eee5de840..ca1d13ca1f0b 100644
>> --- a/common/rc
>> +++ b/common/rc
>> @@ -5201,6 +5201,13 @@ _set_fs_sysfs_attr()
>>   	local attr=$1
>>   	shift
>>   	local content="$*"
>> +	local logfile="/dev/null"
> 
> If we're not actually running a test, should the error output go to
> $check.full instead?
> 

Ah. I didn't know. It seems like no one is using check.full.

-----
$ find . -type f -exec grep check\.full {} \; -print
	rm -f $check.full
./check
-----

Thanks, Anand


> --D
> 
>> +
>> +	# This function may be called outside a testcase during setup,
>> +	# so seqres might not be set.
>> +	if [[ -v seqres ]]; then
>> +		logfile="$seqres.full"
>> +	fi
>>   
>>   	if [ ! -b "$dev" -o -z "$attr" -o -z "$content" ];then
>>   		_fail "Usage: _set_fs_sysfs_attr <mounted_device> <attr> <content>"
>> @@ -5208,8 +5215,8 @@ _set_fs_sysfs_attr()
>>   
>>   	local dname=$(_fs_sysfs_dname $dev)
>>   
>> -	echo "echo '$content' 2>&1 > /sys/fs/${FSTYP}/${dname}/${attr}" >> $seqres.full
>> -	echo "$content" 2>&1 > /sys/fs/${FSTYP}/${dname}/${attr} | tee -a $seqres.full
>> +	echo "echo '$content' 2>&1 > /sys/fs/${FSTYP}/${dname}/${attr}" >> $logfile
>> +	echo "$content" 2>&1 > /sys/fs/${FSTYP}/${dname}/${attr} | tee -a $logfile
>>   }
>>   
>>   # Print the content of /sys/fs/$FSTYP/$DEV/$ATTR
>> -- 
>> 2.47.0
>>
>>

Re: [PATCH v5 2/6] fstests: common/rc: fix unset seqres in _set_fs_sysfs_attr

[Darrick J. Wong]

On Tue, Apr 08, 2025 at 11:10:43PM +0800, Anand Jain wrote:
> 
> 
> On 8/4/25 00:58, Darrick J. Wong wrote:
> > On Mon, Apr 07, 2025 at 11:48:16AM +0800, Anand Jain wrote:
> > > Ensure logs don’t write to a `.full` file when `_set_fs_sysfs_attr()`
> > > is called during setup (before a testcase) in XFS due to unset seqres.
> > > 
> > > Signed-off-by: Anand Jain <anand.jain@oracle.com>
> > > ---
> > >   common/rc | 11 +++++++++--
> > >   1 file changed, 9 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/common/rc b/common/rc
> > > index e89eee5de840..ca1d13ca1f0b 100644
> > > --- a/common/rc
> > > +++ b/common/rc
> > > @@ -5201,6 +5201,13 @@ _set_fs_sysfs_attr()
> > >   	local attr=$1
> > >   	shift
> > >   	local content="$*"
> > > +	local logfile="/dev/null"
> > 
> > If we're not actually running a test, should the error output go to
> > $check.full instead?
> > 
> 
> Ah. I didn't know. It seems like no one is using check.full.

Oh, they do, but it's subtle:

$ git grep seqres=
check:852:      seqres="$check"
check:1120:     seqres="$check"

--D

> -----
> $ find . -type f -exec grep check\.full {} \; -print
> 	rm -f $check.full
> ./check
> -----
> 
> Thanks, Anand
> 
> 
> > --D
> > 
> > > +
> > > +	# This function may be called outside a testcase during setup,
> > > +	# so seqres might not be set.
> > > +	if [[ -v seqres ]]; then
> > > +		logfile="$seqres.full"
> > > +	fi
> > >   	if [ ! -b "$dev" -o -z "$attr" -o -z "$content" ];then
> > >   		_fail "Usage: _set_fs_sysfs_attr <mounted_device> <attr> <content>"
> > > @@ -5208,8 +5215,8 @@ _set_fs_sysfs_attr()
> > >   	local dname=$(_fs_sysfs_dname $dev)
> > > -	echo "echo '$content' 2>&1 > /sys/fs/${FSTYP}/${dname}/${attr}" >> $seqres.full
> > > -	echo "$content" 2>&1 > /sys/fs/${FSTYP}/${dname}/${attr} | tee -a $seqres.full
> > > +	echo "echo '$content' 2>&1 > /sys/fs/${FSTYP}/${dname}/${attr}" >> $logfile
> > > +	echo "$content" 2>&1 > /sys/fs/${FSTYP}/${dname}/${attr} | tee -a $logfile
> > >   }
> > >   # Print the content of /sys/fs/$FSTYP/$DEV/$ATTR
> > > -- 
> > > 2.47.0
> > > 
> > > 
> 
> 

[PATCH v5 3/6] fstests: filter: helper for sysfs error filtering

[Anand Jain]

Added filter helper to filter sysfs write errors, retain only the
error part.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
Reviewed-by: Zorro Lang <zlang@redhat.com>
---
 common/filter | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/common/filter b/common/filter
index 1ebfd27e898e..bbe13f4c8a8d 100644
--- a/common/filter
+++ b/common/filter
@@ -674,5 +674,14 @@ _filter_flakey_EIO()
 	sed -e "s#.*: Input\/output error#$message#"
 }
 
+# Filters
+#      +./common/rc: line 5085: echo: write error: Invalid argument
+# to
+# 	Invalid argument
+_filter_sysfs_error()
+{
+	sed 's/.*: \(.*\)$/\1/'
+}
+
 # make sure this script returns success
 /bin/true
-- 
2.47.0

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

[Anand Jain]

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
+
+	local dname=$(_fs_sysfs_dname $dev)
+	test -e /sys/fs/${FSTYP}/${dname}/${attr}
+
+	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'
+
+	# 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

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

[Darrick J. Wong]

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?

> +
> +	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?

> +
> +	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 ... ?

--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
> 
> 

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

[Anand Jain]

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
>>
>>

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

[Darrick J. Wong]

On Wed, Apr 09, 2025 at 12:05:11AM +0800, Anand Jain wrote:
> 
> 
> 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?

Doh, I missed that it was _fail and not echo.  Please ignore my
comment.

> > > +
> > > +	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

<nod>

>         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.

Ok.

--D

> 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
> > > 
> > > 
> 
> 

[PATCH v5 5/6] fstests: btrfs: testcase for sysfs policy syntax verification

[Anand Jain]

Checks if the sysfs attribute sanitizes arguments and verifies
input syntax.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 tests/btrfs/329     | 19 +++++++++++++++++++
 tests/btrfs/329.out | 19 +++++++++++++++++++
 2 files changed, 38 insertions(+)
 create mode 100755 tests/btrfs/329
 create mode 100644 tests/btrfs/329.out

diff --git a/tests/btrfs/329 b/tests/btrfs/329
new file mode 100755
index 000000000000..c87e96a4f38a
--- /dev/null
+++ b/tests/btrfs/329
@@ -0,0 +1,19 @@
+#! /bin/bash
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (c) 2025 Oracle.  All Rights Reserved.
+#
+# FS QA Test 329
+#
+# Verify sysfs knob input syntax for read_policy round-robin
+#
+. ./common/preamble
+_begin_fstest auto quick
+
+. ./common/sysfs
+. ./common/filter
+
+_require_fs_sysfs_attr_policy $TEST_DEV read_policy round-robin
+_verify_sysfs_syntax $TEST_DEV read_policy round-robin 4096
+
+status=0
+exit
diff --git a/tests/btrfs/329.out b/tests/btrfs/329.out
new file mode 100644
index 000000000000..eff7573adb6a
--- /dev/null
+++ b/tests/btrfs/329.out
@@ -0,0 +1,19 @@
+QA output created by 329
+Invalid argument
+Invalid argument
+Invalid argument
+Invalid argument
+Invalid argument
+Invalid argument
+Invalid argument
+Invalid argument
+Invalid argument
+Invalid argument
+Invalid argument
+Invalid argument
+Invalid argument
+Invalid argument
+Invalid argument
+Invalid argument
+Invalid argument
+Invalid argument
-- 
2.47.0

[PATCH v5 6/6] fstests: btrfs: testcase for sysfs chunk_size attribute validation

[Anand Jain]

Checks if the sysfs attribute sanitizes arguments and verifies
input syntax allocation/data/chunk_size.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 tests/btrfs/334     | 19 +++++++++++++++++++
 tests/btrfs/334.out | 14 ++++++++++++++
 2 files changed, 33 insertions(+)
 create mode 100755 tests/btrfs/334
 create mode 100644 tests/btrfs/334.out

diff --git a/tests/btrfs/334 b/tests/btrfs/334
new file mode 100755
index 000000000000..fd1ac67f9000
--- /dev/null
+++ b/tests/btrfs/334
@@ -0,0 +1,19 @@
+#! /bin/bash
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (c) 2025 Oracle.  All Rights Reserved.
+#
+# FS QA Test 334
+#
+# Verify sysfs knob input syntax for allocation/data/chunk_size
+#
+. ./common/preamble
+_begin_fstest auto quick
+
+. ./common/sysfs
+. ./common/filter
+
+_require_fs_sysfs_attr $TEST_DEV allocation/data/chunk_size
+_verify_sysfs_syntax $TEST_DEV allocation/data/chunk_size 256m
+
+status=0
+exit
diff --git a/tests/btrfs/334.out b/tests/btrfs/334.out
new file mode 100644
index 000000000000..f64f9ac09499
--- /dev/null
+++ b/tests/btrfs/334.out
@@ -0,0 +1,14 @@
+QA output created by 334
+Invalid argument
+Invalid argument
+Invalid argument
+Invalid argument
+Invalid argument
+Invalid argument
+Invalid argument
+Invalid argument
+Invalid argument
+Invalid argument
+Invalid argument
+Invalid argument
+Invalid argument
-- 
2.47.0