Re: [PATCH] btrfs-progs: test-misc: search the backup slot to use at runtime

[David Sterba <dsterba@suse.cz>]

On Tue, Oct 12, 2021 at 06:19:44PM +0800, Qu Wenruo wrote:
> >> +slot_num=$(echo $found | cut -f1 -d:)
> >> +# To follow the dump-super output, where backup slot starts at 0.
> >> +slot_num=$(($slot_num - 1))
> >> +
> >> +# Save the backup slot info into the log
> >> +_log "Backup slot $slot_num will be utilized"
> >> +dump_super | grep -A9 "backup $slot_num:" >> "$RESULTS"
> > 
> > Please don't use the $RESULTS in tests, it's an implementation detail
> > and there should always be helpers hiding, in this case it's run_check.
> 
> In this particular case, won't run_check dump all the output into the log?
> 
> As here we only want the grep result to be output, not the full dump_super.

Sorry, I was not specific, what I meant is:

"dump_super | run_check grep ..."

so only the result of grep ends up in the log.

> >>   run_check "$INTERNAL_BIN/btrfs-corrupt-block" -m $main_root_ptr -f generation "$TEST_DEV"
> >                                                      ^^^^^^^^^^^^^^
> > 
> > Please always quote variables (unless there's a reason not to like for
> > $SUDO_HELPER).
> 
> Forgot it again, but it should be safe as $main_root_ptr should just be 
> a u64 number.
> 
> But yeah, I should quote it...

I know in this case it's clear there's a number, the point is
consistency of the whole testsuite, so one does not have to think if
this unquoted variable is ok or not. The initialization may happen in
other context or lines away or the variable name could be misleading.

IIRC I've seen some commands fail and instead of a single word in the
output there was the error message. Quoting makes it fail at least
somehow safely instead of letting the variable words be interpreted as
individual parameters.