On 2020/4/4 上午1:20, 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.
Exactly what I tested before crafting the patch.
# declare -a tmp
# tmp+=('test')
# unset tmp
# tmp+=('test2')
# echo ${tmp[@]}
And it worked, thus you see the patch.
>
>> +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.
That means the INSTRUMENT must be space split, no way to pass anything
not split by space.
Despite that, I'm pretty OK to change it.
>
>> + 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.
Is there any difference? Or unset it before just makes it cleaner?
Thanks,
Qu
>
>> + 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
Attachment:
signature.asc
Description: OpenPGP digital signature
