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

[Nick Alcock]

On 29 Aug 2022, Eugene Loh via DTrace-devel told this:

> 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.

Ugh, I didn't notice that! The problem is that fixing that makes things
in this already ugly code even uglier: you'd have to stuff the array
expansion through a tac or something.

This is also pig ugly, so I've turned this into a

declare -ga ZAPTHESE=

outside the loop, since all we're actually trying to do is blank the
whole thing out once we're done killing from it: there's no need to go
popping elements from it one by one here.

>>       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.

But what if the pid contains spaces or quotation marks??! ;) (Fixed.)

>>       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.

It's there for one single reason: once we can rely on not using bash
4.3, we can go back to something readable!

>>                       _pid=$!
>>                       disown %-
>> -                    ZAPTHESE="$_pid"
>> +                    needszap=t
>> +                    ZAPTHESE+=("$_pid")
>
> Same minor comment on the quotation marks.

Even more so, given that it's just been populated *without* using
quotation marks.

(All fixed in my next push bar the comment removal. It serves a future
deuglification purpose.)

(Also added an S-o-b, which was missing. I'll do that to everything
without further note.)

-- 
NULL && (void)