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

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

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.