Re: [PATCH] btrfs-progs: fix a check condition in misc/038

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

在 2024/6/5 01:24, Josef Bacik 写道:
> On Tue, Jun 04, 2024 at 03:30:00PM +0930, Qu Wenruo wrote:
>> The test case always fail in my VM, with the following error:
>>
>>   $ sudo TEST=038\* make test-misc
>>      [TEST]   misc-tests.sh
>>      [TEST/misc]   038-backup-root-corruption
>>   Backup 2 not overwritten
>>   test failed for case 038-backup-root-corruption
>>
>> After more debugging, the it turns out that there is nothing wrong
>> except the final check:
>>
>>   [ "$main_root_ptr" -ne "$backup_new_root_ptr" ] || _fail "Backup 2 not overwritten"
>>
>> The _fail() is only triggered if the previous check returns false, which
>> is completely the opposite.
>>
>> In fact the "[ check ] || _fail" pattern is the worst thing in the bash
>> world, super easy to cause the opposite check condition.
>>
>
> Except we do this all of the time, we should be used to it by now.

Well that's true, but at the same time when one see such line, it will
takes more time to fully understand the check.

Thus that's why I hate such line, it takes me over 15 min to find out
that everything is fine, except the last line.

So I really hope no new comers would spend their time to fall into the
same hole.

>
>> Fix it by use a proper "if []; then fi" block, and since we're here also
>> update the error message to use the newest slot number instead.
>>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>> ---
>>   tests/misc-tests/038-backup-root-corruption/test.sh | 4 +++-
>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/tests/misc-tests/038-backup-root-corruption/test.sh b/tests/misc-tests/038-backup-root-corruption/test.sh
>> index 9be0cee36239..0f97577849cc 100755
>> --- a/tests/misc-tests/038-backup-root-corruption/test.sh
>> +++ b/tests/misc-tests/038-backup-root-corruption/test.sh
>> @@ -61,4 +61,6 @@ main_root_ptr=$(dump_super | awk '/^root\t/{print $2}')
>>   slot_num=$(( ($slot_num + 1) % 4 ))
>>   backup_new_root_ptr=$(dump_super | grep -A1 "backup $slot_num" | grep backup_tree_root | awk '{print $2}')
>>
>> -[ "$main_root_ptr" -ne "$backup_new_root_ptr" ] || _fail "Backup 2 not overwritten"
>> +if [ "$main_root_ptr" -ne "$backup_new_root_ptr" ]; then
>> +	_fail "Backup ${slot_num} not overwritten"
>
> Don't we prefer just "$slot_num"?  I feel like I've gotten yelld at for this
> before.  Just change the existing thing to be correct

I thought we prefer ${} to be more safe, but since it's followed by a
space, it should be no difference.

I'll update the patch for more debugging, since on CI it fails (not sure
if it's kernel or something else), but I'm afraid the "if [] then; fi"
would be kept.

Thanks,
Qu
>
> [ "$main_root_ptr" -eq "$backup_new_root_ptr" ] || _fail "Backup $slot_num not overwritten"
>
> Thanks,
>
> Josef
>