[DTrace-devel] [PATCH 04/17] runtest: make ZAPTHESE an array

[Eugene Loh]

Reviewed-by: Eugene Loh <eugene.loh at oracle.com>
with minor comments...

On 8/10/22 18:06, Nick Alcock via DTrace-devel wrote:
> This lets us reliably have multiple elements in ZAPTHESE, removing those
> that have died naturally without disturbing the others that might still
> be running.
> ---
>   runtest.sh | 23 ++++++++++++++---------
>   1 file changed, 14 insertions(+), 9 deletions(-)
>
> diff --git a/runtest.sh b/runtest.sh
> index 00fadaf75e6e..6fab0df21552 100755
> --- a/runtest.sh
> +++ b/runtest.sh
> @@ -75,7 +75,7 @@ find_next_numeric_dir()
>       done
>   }
>   
> -ZAPTHESE=
> +declare -a ZAPTHESE=
>   
>   # run_with_timeout TIMEOUT CMD [ARGS ...]
>   #
> @@ -372,15 +372,15 @@ log()
>       printf "%s" "$*" | sed 's,%,%%,g' | xargs -0n 1 printf >> $LOGFILE
>   }
>   
> -# At shutdown, delete this directory, kill requested process groups, and restore
> +# At shutdown, delete this directory, kill requested processes, and restore
>   # core_pattern.  When this is specifically an interruption, report as much in
>   # the log.
>   closedown()
>   {
> -    for pid in $ZAPTHESE; do
> -        kill -9 -- $(printf " -%i" $pid)
> +    for pid in ${ZAPTHESE[@]}; do
> +        kill -9 -- $pid
> +        unset "ZAPTHESE[$((${#ZAPTHESE[@]}-1))]"

I guess okay, but kind of weird.  You kill pids "in order" but unset 
them in reverse order.

>       done
> -    ZAPTHESE=
>       if [[ -z $orig_core_pattern ]]; then
>           echo $orig_core_pattern > /proc/sys/kernel/core_pattern
>           echo $orig_core_uses_pid > /proc/sys/kernel/core_uses_pid
> @@ -626,7 +626,7 @@ if [[ -z $NOBADDOF ]]; then
>       test/utils/badioctl > /dev/null 2> $tmpdir/badioctl.err &
>       declare ioctlpid=$!
>   
> -    ZAPTHESE="$ioctlpid"
> +    ZAPTHESE+=("$ioctlpid")

No need for the quotation marks.  And in a later patch (usdt: daemon), 
you add dtprobed_pid without the quotation marks.  Might as well employ 
a consistent style.

>       for _test in $(if [[ $ONLY_TESTS ]]; then
>                         echo $TESTS | sed 's,\.r$,\.d,g; s,\.r ,.d ,g'
>                      else
> @@ -651,7 +651,8 @@ if [[ -z $NOBADDOF ]]; then
>           sum "badioctl stderr:"
>           tee -a < $tmpdir/badioctl.err $LOGFILE >> $SUMFILE
>       fi
> -    ZAPTHESE=
> +    # equivalent of unset "ZAPTHESE[-1]" on bash < 4.3

Might as well omit the comment.  It doesn't explain much.  Further, you 
use this idiom twice elsewhere without commenting on it.

> +    unset "ZAPTHESE[$((${#ZAPTHESE[@]}-1))]"
>       exit 0
>   fi
>   
> @@ -1193,6 +1194,7 @@ for dt in $dtrace; do
>   
>                   trigger_timing=synchro
>                   trigger_delay=
> +                needszap=
>                   if exist_options trigger-timing $_test &&
>                      [[ "x$(extract_options trigger-timing $_test)" != "xsynchro" ]]; then
>                       trigger_timing="$(extract_options trigger-timing $_test)"
> @@ -1215,7 +1217,8 @@ for dt in $dtrace; do
>                       ( [[ -n $trigger_delay ]] && sleep $trigger_delay; exec $trigger; ) &
>                       _pid=$!
>                       disown %-
> -                    ZAPTHESE="$_pid"
> +                    needszap=t
> +                    ZAPTHESE+=("$_pid")

Same minor comment on the quotation marks.

>                       if [[ $dt_flags =~ \$_pid ]]; then
>                           dt_flags="$(echo ''"$dt_flags" | sed 's,\$_pid[^a-zA-Z],'$_pid',g; s,\$_pid$,'$_pid',g')"
> @@ -1240,7 +1243,9 @@ for dt in $dtrace; do
>                   if [[ -n $_pid ]] && [[ "$(ps -p $_pid -o ppid=)" -eq $BASHPID ]]; then
>                       kill $_pid >/dev/null 2>&1
>                   fi
> -                ZAPTHESE=
> +                if [[ -n $needszap ]]; then
> +                    unset "ZAPTHESE[$((${#ZAPTHESE[@]}-1))]"
> +                fi
>                   unset _pid
>               fi
>