Re: [PATCH v2] fstests: btrfs/244: add test case to verify the behavior of deleting non-existing device

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

On 2021/8/6 下午7:42, Nikolay Borisov wrote:
>
>
> On 6.08.21 г. 14:33, Qu Wenruo wrote:
>> There is a kernel regression for btrfs, that when passing non-existing
>> devid to "btrfs device remove" command, kernel will crash due to NULL
>> pointer dereference.
>>
>> The test case is for such regression, it will:
>>
>> - Create and mount an empty single-device btrfs
>> - Try to remove devid 3, which doesn't exist for above fs
>> - Make sure the command exits properly with expected error message
>>
>> The kernel fix is titled "btrfs: fix NULL pointer dereference when
>> deleting device by invalid id".
>>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>> ---
>> Changelog:
>> v2:
>> - Change the subject to also verify the error behavior
>> - Include the error message into golden output
>> - Also verify the return value of btrfs command
>> ---
>>   tests/btrfs/244     | 47 +++++++++++++++++++++++++++++++++++++++++++++
>>   tests/btrfs/244.out |  2 ++
>>   2 files changed, 49 insertions(+)
>>   create mode 100755 tests/btrfs/244
>>   create mode 100644 tests/btrfs/244.out
>>
>> diff --git a/tests/btrfs/244 b/tests/btrfs/244
>> new file mode 100755
>> index 00000000..fbefeedf
>> --- /dev/null
>> +++ b/tests/btrfs/244
>> @@ -0,0 +1,47 @@
>> +#! /bin/bash
>> +# SPDX-License-Identifier: GPL-2.0
>> +# Copyright (c) 2021 SUSE Linux Products GmbH.  All Rights Reserved.
>> +#
>> +# FS QA Test 244
>> +#
>> +# Make sure "btrfs device remove" won't crash when non-existing devid
>> +# is provided
>> +#
>> +. ./common/preamble
>> +_begin_fstest auto quick volume dangerous
>> +
>> +# Override the default cleanup function.
>> +# _cleanup()
>> +# {
>> +# 	cd /
>> +# 	rm -r -f $tmp.*
>> +# }
>> +
>> +# Import common functions.
>> +# . ./common/filter
>> +
>> +# real QA test starts here
>> +
>> +# Modify as appropriate.
>> +_supported_fs btrfs
>> +_require_scratch
>> +
>> +_scratch_mkfs >> $seqres.full 2>&1
>> +_scratch_mount
>> +
>> +# Above created fs only contains one device with devid 1, device remove 3
>> +# should just fail with proper error message showing devid 3 can't be found.
>> +# Although on unpatched kernel, this will trigger a NULL pointer dereference.
>> +$BTRFS_UTIL_PROG device remove 3 $SCRATCH_MNT
>> +ret=$?
>> +
>> +if [ $ret -ne 1 ]; then
>> +	echo "Unexpected return value from btrfs command, has $ret expected 1"
>> +fi
>
> <rant>
> This just shows how broken progs are w.r.t return values. The generally
> accepted return value is 0 on success, yet it returns 1 on success since
> the functions implementing this functionality in progs treat the return
> value as a boolean.
> </rant>


Nope, we're testing a failure case, thus returning 1 is what we expect.

Thanks,
Qu
>
>
>
>> +
>> +# Fstests will automatically check the filesystem to make sure metadata is not
>> +# corrupted.
>> +
>> +# success, all done
>> +status=0
>> +exit
>> diff --git a/tests/btrfs/244.out b/tests/btrfs/244.out
>> new file mode 100644
>> index 00000000..629adf2a
>> --- /dev/null
>> +++ b/tests/btrfs/244.out
>> @@ -0,0 +1,2 @@
>> +QA output created by 244
>> +ERROR: error removing devid 3: No such file or directory
>>