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

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

[Anand Jain]

Patches 1/3 and 2/3 lay the groundwork for testing sysfs input validation.
Patch 3/3 introduces the actual test case.

Anand Jain (3):
  fstests: common/rc: set_fs_sysfs_attr: redirect errors to stdout
  fstests: filter: helpers for sysfs error filtering
  fstests: btrfs: testcase for sysfs policy syntax verification

 common/filter       | 24 ++++++++++++
 common/rc           |  3 +-
 tests/btrfs/329     | 92 +++++++++++++++++++++++++++++++++++++++++++++
 tests/btrfs/329.out |  2 +
 4 files changed, 120 insertions(+), 1 deletion(-)
 create mode 100755 tests/btrfs/329
 create mode 100644 tests/btrfs/329.out

-- 
2.47.0

[PATCH 1/3] 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>
---
 common/rc | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/common/rc b/common/rc
index a3ee09d54cf0..2c4621bad5d0 100644
--- a/common/rc
+++ b/common/rc
@@ -5082,7 +5082,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 2/3] fstests: filter: helpers for sysfs error filtering

[Anand Jain]

Added filter helpers to handle sysfs write errors, to ensure the sysfs
write command fails with an "invalid argument" error.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 common/filter | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/common/filter b/common/filter
index 7e02ded377cc..30bb9161d620 100644
--- a/common/filter
+++ b/common/filter
@@ -671,5 +671,29 @@ _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/'
+}
+
+_expect_error_invalid_argument()
+{
+	local line
+
+	read -r line || {
+		echo "ERROR: Expected 'Invalid argument' but got no input" >&2
+		return 1
+	}
+
+	if [[ $line != *"Invalid argument"* ]]; then
+		echo "ERROR: Expected 'Invalid argument' but got: $line" >&2
+		return 1
+	fi
+}
+
 # make sure this script returns success
 /bin/true
-- 
2.47.0

[PATCH 3/3] 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     | 92 +++++++++++++++++++++++++++++++++++++++++++++
 tests/btrfs/329.out |  2 +
 2 files changed, 94 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..9f63ab951eac
--- /dev/null
+++ b/tests/btrfs/329
@@ -0,0 +1,92 @@
+#! /bin/bash
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (c) 2025 Oracle.  All Rights Reserved.
+#
+# FS QA Test 329
+#
+# Verify sysfs knob input syntax.
+#
+. ./common/preamble
+_begin_fstest auto quick
+
+. ./common/filter
+
+# Modify as appropriate.
+_require_scratch
+_require_fs_sysfs read_policy
+
+_scratch_mkfs > /dev/null 2>&1 || _fail "mkfs failed"
+_scratch_mount
+
+set_sysfs_policy()
+{
+	local attr=$1
+	shift
+	local policy=$@
+
+	_set_fs_sysfs_attr $SCRATCH_DEV $attr ${policy}
+	_get_fs_sysfs_attr $SCRATCH_DEV $attr | grep -q "[${policy}]"
+	if [[ $? != 0 ]]; then
+		echo "Setting sysfs $attr $policy failed"
+	fi
+}
+
+set_sysfs_policy_must_fail()
+{
+	local attr=$1
+	shift
+	local policy=$@
+
+	_set_fs_sysfs_attr $SCRATCH_DEV $attr ${policy} | _filter_sysfs_error \
+			| _expect_error_invalid_argument | tee -a $seqres.full
+}
+
+verify_sysfs_syntax()
+{
+	local attr=$1
+	local policy=$2
+	local value=$3
+
+	# Test policy specified wrongly. Must fail.
+	set_sysfs_policy_must_fail $attr "'$policy $policy'"
+	set_sysfs_policy_must_fail $attr "'$policy t'"
+	set_sysfs_policy_must_fail $attr "' '"
+	set_sysfs_policy_must_fail $attr "'${policy} n'"
+	set_sysfs_policy_must_fail $attr "'n ${policy}'"
+	set_sysfs_policy_must_fail $attr "' ${policy}'"
+	set_sysfs_policy_must_fail $attr "' ${policy} '"
+	set_sysfs_policy_must_fail $attr "'${policy} '"
+	set_sysfs_policy_must_fail $attr _${policy}
+	set_sysfs_policy_must_fail $attr ${policy}_
+	set_sysfs_policy_must_fail $attr _${policy}_
+	set_sysfs_policy_must_fail $attr ${policy}:
+	# Test policy longer than 32 chars fails stable.
+	set_sysfs_policy_must_fail $attr 'jfdkkkkjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjjffjfjfffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff'
+
+	# Test policy specified correctly. Must pass.
+	set_sysfs_policy $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 $attr "'$policy: $value'"
+	set_sysfs_policy_must_fail $attr "'$policy:$value '"
+	set_sysfs_policy_must_fail $attr "'$policy:$value '"
+	set_sysfs_policy_must_fail $attr "'$policy: $value'"
+	set_sysfs_policy_must_fail $attr "'$policy :$value'"
+
+	# Test policy and value all specified correctly. Must pass.
+	set_sysfs_policy $attr $policy:$value
+}
+
+verify_sysfs_syntax read_policy			pid
+verify_sysfs_syntax read_policy			round-robin 4k
+verify_sysfs_syntax allocation/data/chunk_size	10g
+
+echo Silence is golden
+
+status=0
+exit
diff --git a/tests/btrfs/329.out b/tests/btrfs/329.out
new file mode 100644
index 000000000000..9794dc15960d
--- /dev/null
+++ b/tests/btrfs/329.out
@@ -0,0 +1,2 @@
+QA output created by 329
+Silence is golden
-- 
2.47.0

Re: [PATCH 3/3] fstests: btrfs: testcase for sysfs policy syntax verification

[Dave Chinner]

On Wed, Jan 29, 2025 at 11:19:54PM +0800, Anand Jain wrote:
> Checks if the sysfs attribute sanitizes arguments and verifies
> input syntax.
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
>  tests/btrfs/329     | 92 +++++++++++++++++++++++++++++++++++++++++++++
>  tests/btrfs/329.out |  2 +
>  2 files changed, 94 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..9f63ab951eac
> --- /dev/null
> +++ b/tests/btrfs/329
> @@ -0,0 +1,92 @@
> +#! /bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +# Copyright (c) 2025 Oracle.  All Rights Reserved.
> +#
> +# FS QA Test 329
> +#
> +# Verify sysfs knob input syntax.
> +#
> +. ./common/preamble
> +_begin_fstest auto quick
> +
> +. ./common/filter
> +
> +# Modify as appropriate.
> +_require_scratch
> +_require_fs_sysfs read_policy
> +
> +_scratch_mkfs > /dev/null 2>&1 || _fail "mkfs failed"
> +_scratch_mount
> +
> +set_sysfs_policy()
> +{
> +	local attr=$1
> +	shift
> +	local policy=$@
> +
> +	_set_fs_sysfs_attr $SCRATCH_DEV $attr ${policy}
> +	_get_fs_sysfs_attr $SCRATCH_DEV $attr | grep -q "[${policy}]"
> +	if [[ $? != 0 ]]; then
> +		echo "Setting sysfs $attr $policy failed"
> +	fi
> +}
> +
> +set_sysfs_policy_must_fail()
> +{
> +	local attr=$1
> +	shift
> +	local policy=$@
> +
> +	_set_fs_sysfs_attr $SCRATCH_DEV $attr ${policy} | _filter_sysfs_error \
> +			| _expect_error_invalid_argument | tee -a $seqres.full

This "catch an exact error or output a different error then use
golden image match failure on secondary error to mark the test as
failed" semantic is .... overly complex.

The output on failure of _filter_sysfs_error will be "Invalid
input". If there's some other failure or it succeeds, the output
will indicate the failure that occurred (i.e. missing line means no
error, different error will output directly by the filter). The
golden image matching will still fail the test.

IOWs, _expect_error_invalid_argument and the output to seqres.full
can go away if the test.out file has a matching error for each
call to set_sysfs_policy_must_fail(). i.e it looks like:

QA output created by 329
Invalid input
Invalid input
Invalid input
Invalid input
Invalid input
Invalid input
.....
Invalid input

-Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 3/3] fstests: btrfs: testcase for sysfs policy syntax verification

[Anand Jain]

>> +set_sysfs_policy_must_fail()
>> +{
>> +	local attr=$1
>> +	shift
>> +	local policy=$@
>> +
>> +	_set_fs_sysfs_attr $SCRATCH_DEV $attr ${policy} | _filter_sysfs_error \
>> +			| _expect_error_invalid_argument | tee -a $seqres.full
> 
> This "catch an exact error or output a different error then use
> golden image match failure on secondary error to mark the test as
> failed" semantic is .... overly complex.
> 
> The output on failure of _filter_sysfs_error will be "Invalid
> input". If there's some other failure or it succeeds, the output
> will indicate the failure that occurred (i.e. missing line means no
> error, different error will output directly by the filter). The
> golden image matching will still fail the test.
> 
> IOWs, _expect_error_invalid_argument and the output to seqres.full
> can go away if the test.out file has a matching error for each
> call to set_sysfs_policy_must_fail(). i.e it looks like:
> 
> QA output created by 329
> Invalid input
> Invalid input
> Invalid input
> Invalid input
> Invalid input
> Invalid input
> .....
> Invalid input

Thanks for the review.

This test case verifies the sysfs interface syntax in general.
Relying on golden output can cause false negatives on older
kernels lacking support for newer sysfs policies.
Creating individual test cases for each sysfs interface is
unnecessary overhead.

With this approach, when needed, we use:

if _has_fs_sysfs_attr $dev <sysfs-interface>; then
     verify_sysfs_syntax <sysfs-interface> <value>
fi

- Anand

Re: [PATCH 3/3] fstests: btrfs: testcase for sysfs policy syntax verification

[Dave Chinner]

On Fri, Jan 31, 2025 at 02:43:03PM +0800, Anand Jain wrote:
> 
> > > +set_sysfs_policy_must_fail()
> > > +{
> > > +	local attr=$1
> > > +	shift
> > > +	local policy=$@
> > > +
> > > +	_set_fs_sysfs_attr $SCRATCH_DEV $attr ${policy} | _filter_sysfs_error \
> > > +			| _expect_error_invalid_argument | tee -a $seqres.full
> > 
> > This "catch an exact error or output a different error then use
> > golden image match failure on secondary error to mark the test as
> > failed" semantic is .... overly complex.
> > 
> > The output on failure of _filter_sysfs_error will be "Invalid
> > input". If there's some other failure or it succeeds, the output
> > will indicate the failure that occurred (i.e. missing line means no
> > error, different error will output directly by the filter). The
> > golden image matching will still fail the test.
> > 
> > IOWs, _expect_error_invalid_argument and the output to seqres.full
> > can go away if the test.out file has a matching error for each
> > call to set_sysfs_policy_must_fail(). i.e it looks like:
> > 
> > QA output created by 329
> > Invalid input
> > Invalid input
> > Invalid input
> > Invalid input
> > Invalid input
> > Invalid input
> > .....
> > Invalid input
> 
> Thanks for the review.
> 
> This test case verifies the sysfs interface syntax in general.
> Relying on golden output can cause false negatives on older
> kernels lacking support for newer sysfs policies.
> Creating individual test cases for each sysfs interface is
> unnecessary overhead.
> 
> With this approach, when needed, we use:
> 
> if _has_fs_sysfs_attr $dev <sysfs-interface>; then
>     verify_sysfs_syntax <sysfs-interface> <value>
> fi

One test instance per sysfs attribute, please.

i.e. move verify_sysfs_syntax() gets moved to common/ somewhere,
then the test for any given sysfs attr is a simple 10 liner with a
fixed golden output.

We can then do the same sort of input testing for sysfs attrs that
belong to other filesystems, too, not just a handful of btrfs
specific ones this test touches. I'd much prefer such tests are
largely generic like so:

....
_require_fs_sysfs_attr $TEST_DEV <sysfs-attr>
_verify_sysfs_syntax $TEST_DEV <sysfs-attr>
exit

If the sysfs-attr doesn't exist, then the test is _not_run and
this emits a log file note that can be captured. If it does exist
and doesn't behave correctly, the test then fails.

Note that things like "test not run because sysfs attr does not
exist" notes in the log files can be important for QE
people trying to track whether backports for older/stable kernels
work correctly. The proposed test is completely silent on whether
any specific sysfs attr was tested or not, and that's not really
helpful in identifying whether something works correctly or not...

-Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 3/3] fstests: btrfs: testcase for sysfs policy syntax verification

[Anand Jain]

On 4/2/25 08:18, Dave Chinner wrote:
> On Fri, Jan 31, 2025 at 02:43:03PM +0800, Anand Jain wrote:
>>
>>>> +set_sysfs_policy_must_fail()
>>>> +{
>>>> +	local attr=$1
>>>> +	shift
>>>> +	local policy=$@
>>>> +
>>>> +	_set_fs_sysfs_attr $SCRATCH_DEV $attr ${policy} | _filter_sysfs_error \
>>>> +			| _expect_error_invalid_argument | tee -a $seqres.full
>>>
>>> This "catch an exact error or output a different error then use
>>> golden image match failure on secondary error to mark the test as
>>> failed" semantic is .... overly complex.
>>>
>>> The output on failure of _filter_sysfs_error will be "Invalid
>>> input". If there's some other failure or it succeeds, the output
>>> will indicate the failure that occurred (i.e. missing line means no
>>> error, different error will output directly by the filter). The
>>> golden image matching will still fail the test.
>>>
>>> IOWs, _expect_error_invalid_argument and the output to seqres.full
>>> can go away if the test.out file has a matching error for each
>>> call to set_sysfs_policy_must_fail(). i.e it looks like:
>>>
>>> QA output created by 329
>>> Invalid input
>>> Invalid input
>>> Invalid input
>>> Invalid input
>>> Invalid input
>>> Invalid input
>>> .....
>>> Invalid input
>>
>> Thanks for the review.
>>
>> This test case verifies the sysfs interface syntax in general.
>> Relying on golden output can cause false negatives on older
>> kernels lacking support for newer sysfs policies.
>> Creating individual test cases for each sysfs interface is
>> unnecessary overhead.
>>
>> With this approach, when needed, we use:
>>
>> if _has_fs_sysfs_attr $dev <sysfs-interface>; then
>>      verify_sysfs_syntax <sysfs-interface> <value>
>> fi
> 
> One test instance per sysfs attribute, please.
> 
> i.e. move verify_sysfs_syntax() gets moved to common/ somewhere,
> then the test for any given sysfs attr is a simple 10 liner with a
> fixed golden output.
> 
> We can then do the same sort of input testing for sysfs attrs that
> belong to other filesystems, too, not just a handful of btrfs
> specific ones this test touches. I'd much prefer such tests are
> largely generic like so:
> 
> ....
> _require_fs_sysfs_attr $TEST_DEV <sysfs-attr>
> _verify_sysfs_syntax $TEST_DEV <sysfs-attr>
> exit
> 
> If the sysfs-attr doesn't exist, then the test is _not_run and
> this emits a log file note that can be captured. If it does exist
> and doesn't behave correctly, the test then fails.
> 

> Note that things like "test not run because sysfs attr does not
> exist" notes in the log files can be important for QE
> people trying to track whether backports for older/stable kernels
> work correctly.

It can also be useful for backup code testing.
I’ve addressed that in V2 and sent the fix.

Thanks, Anand


> The proposed test is completely silent on whether
> any specific sysfs attr was tested or not, and that's not really
> helpful in identifying whether something works correctly or not...
> 
> -Dave.