On Fri, Apr 03, 2020 at 07:20:16PM +0200, David Sterba wrote:
> On Mon, Mar 30, 2020 at 03:02:32PM +0800, Qu Wenruo wrote:
> > +# Temporary command array for building real command
> > +declare -a cmd_array
>
> This gets declared only once when the script is sourced but there are
> numerous calls to 'unset cmd_array'. How does this interact?
>
> The functions are called in the same shell level so this will unset and
> destroy the cmd_array, so it can't work after first use of eg.
> run_check. So there's some shell magic because this seems to work.
Adding some debugging prints before and after the cmd_array, the variable
exists at the beginning of expand_command and is destroyed after unset.
> > +expand_command()
> > +{
> > + local command_index
> > + local ins
> > + local spec
> > +
> > + ins=$(_get_spec_ins "$@")
> > + spec=$(($ins-1))
> > + spec=$(_cmd_spec "${@:$spec}")
> > +
> > + if [ "$1" = 'root_helper' ] && _is_target_command "$2" ; then
> > + command_index=2
> > + elif _is_target_command "$1" ; then
> > + command_index=1
> > + else
> > + # Since we iterate from offset 1, this never get hit,
> > + # thus we won't insert $INSTRUMENT
> > + command_index=0
> > + fi
> > +
> > + for ((i = 1; i <= $#; i++ )); do
> > + if [ $i -eq $command_index -a ! -z "$INSTRUMENT" ]; then
> > + cmd_array+=("$INSTRUMENT")
>
> This inserts the contents of INSTRUMENT as one value, that won't work
> eg. for INSTRUMENT='valgrind --tool=memcheck' or any extra parameters
> passed to valgrind or whatever other tool we'd like to use.
With unquoted $INSTRUMENT it does what I'd expect.
> > + fi
> > + if [ $i -eq $ins -a ! -z "$spec" ]; then
> > + cmd_array+=("$spec")
> > + fi
> > + cmd_array+=("${!i}")
> > +
> > + done
> > +}
> > +
> > # Argument passing magic:
> > # the command passed to run_* helpers is inspected, if there's 'btrfs command'
> > # found and there are defined additional arguments, they're inserted just after
> > @@ -145,49 +202,28 @@ _cmd_spec()
> >
> > run_check()
> > {
> > - local spec
> > - local ins
> > + expand_command "$@"
>
> The cmd_array should be reset before it's used to build a new command,
> not after.
unset cmd_array won't work here, but cmd_array=() resets the values and
does not destroy the variable.
> > + echo "====== RUN CHECK ${cmd_array[@]}" >> "$RESULTS" 2>&1
> > + if [[ $TEST_LOG =~ tty ]]; then echo "CMD: ${cmd_array[@]}" > /dev/tty; fi
> >
> > - ins=$(_get_spec_ins "$@")
> > - spec=$(($ins-1))
> > - spec=$(_cmd_spec "${@:$spec}")
> > - set -- "${@:1:$(($ins-1))}" $spec "${@: $ins}"
> > - echo "====== RUN CHECK $@" >> "$RESULTS" 2>&1
> > - if [[ $TEST_LOG =~ tty ]]; then echo "CMD: $@" > /dev/tty; fi
> > -
> > - # If the command is `root_helper` or mount/umount, don't call INSTRUMENT
> > - # as most profiling tool like valgrind can't handle setuid/setgid/setcap
> > - # which mount normally has.
> > - # And since mount/umount is only called with run_check(), we don't need
> > - # to do the same check on other run_*() functions.
> > - if [ "$1" = 'root_helper' -o "$1" = 'mount' -o "$1" = 'umount' ]; then
> > - "$@" >> "$RESULTS" 2>&1 || _fail "failed: $@"
> > - else
> > - $INSTRUMENT "$@" >> "$RESULTS" 2>&1 || _fail "failed: $@"
> > - fi
> > + "${cmd_array[@]}" >> "$RESULTS" 2>&1 || _fail "failed: ${cmd_array[@]}"
> > + unset cmd_array
