| Message ID | a49e6b43e3c140995567fea035017309b4bcd53c.1717480797.git.wqu@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs-progs: fix a check condition in misc/038 | expand |
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
在 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 >
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 ${ }.
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
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(-)