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

[Dave Chinner <david@fromorbit.com>]

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