Re: [PATCH] fstests: btrfs/253: fix false alert due to _set_fs_sysfs_attr changes

[Qu Wenruo <wqu@suse.com>]

在 2025/4/21 16:30, Naohiro Aota 写道:
> On Sun Apr 20, 2025 at 5:38 PM JST, Qu Wenruo wrote:
>> [FALSE FAILURE]
>> Test btrfs/253 now fails like the following:
>>
>> btrfs/253 2s ... - output mismatch (see ~/xfstests/results//btrfs/253.out.bad)
>>      --- tests/btrfs/253.out	2022-05-11 11:25:30.753333331 +0930
>>      +++ ~/xfstests/results//btrfs/253.out.bad	2025-04-20 17:28:39.139180394 +0930
>>      @@ -5,6 +5,7 @@
>>       Calculate request size so last memory allocation cannot be completely fullfilled.
>>       Third allocation.
>>       Force allocation of system block type must fail.
>>      +./common/rc: line 5213: echo: write error: No space left on device
>>       Verify first allocation.
>>       Verify second allocation.
>>       Verify third allocation.
>>      ...
>>      (Run 'diff -u ~/xfstests/tests/btrfs/253.out ~/xfstests/results//btrfs/253.out.bad'  to see the entire diff)
>>
>> [CAUSE]
>> Since commit 0a9011ae6a36 ("fstests: common/rc: set_fs_sysfs_attr:
>> redirect errors to stdout") the function _set_fs_sysfs_attr() always
>> output everything into stdout, thus the stderr redirection makes no
>> sense anymore.
>>
>> And the expected failure will cause output difference and fail the test.
>>
>> [FIX]
>> Instead of the useless re-direct of stderr, save the stdout and check if
>> it contains the word "error" to determine if it failed.
>>
>> Fixes: 0a9011ae6a36 ("fstests: common/rc: set_fs_sysfs_attr: redirect errors to stdout")
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>> ---
>>   tests/btrfs/253 | 7 ++++++-
>>   1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/tests/btrfs/253 b/tests/btrfs/253
>> index adbc6bfb..ad69dfe1 100755
>> --- a/tests/btrfs/253
>> +++ b/tests/btrfs/253
>> @@ -228,7 +228,12 @@ alloc_size "Data" FOURTH_DATA_SIZE_MB
>>   # Force chunk allocation of system block type must fail.
>>   #
>>   echo "Force allocation of system block type must fail."
>> -_set_fs_sysfs_attr ${SCRATCH_BDEV} allocation/system/force_chunk_alloc 1 2>/dev/null
>> +output=$(_set_fs_sysfs_attr ${SCRATCH_BDEV} allocation/system/force_chunk_alloc 1)
>> +
>> +if ! echo "$output" | grep -q "error" ; then
>> +	echo "Force allocation succeeded unexpectedly."
>> +	echo "$output" >> $seqres.full
>> +fi
> 
> Can't we use _set_sysfs_policy_must_fail() instead? Apparently, that
> function does not looks only for "policy" setting. So, renaming the
> function would also be appreciated.

Thanks a lot for pointing out the _must_fail() version.

Didn't notice that since the functions are split into common/rc and 
common/sysfs.

I have to say the split and naming is really bad, especially when none 
of the policy related helpers are not utilized by anyone.

I'll keep the fix simple for now, and leave the rename for Anand, as he 
would have a more clear view why those helpers are introduced and may 
have a better naming scheme.

Thanks,
Qu

> 
>>   
>>   #
>>   # Verification of initial allocation.