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

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

[Qu Wenruo]

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.

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"
+fi
-- 
2.45.2

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

[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.

> 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

[ "$main_root_ptr" -eq "$backup_new_root_ptr" ] || _fail "Backup $slot_num not overwritten"

Thanks,

Josef

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

[Qu Wenruo]

在 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
>

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

[David Sterba]

On Wed, Jun 05, 2024 at 07:38:39AM +0930, Qu Wenruo wrote:
> >> -[ "$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.

The thing that I ask for is to quote all variables, otherwise the _ in
variable name is recognized so ${two_words} is the same as $two_words.
Where it would matter is if _ follows the variable, otherwise no ${ }.