[DTrace-devel] [PATCH 3/4] Test the dtrace provider cleanup mechanism

[Eugene Loh]

On 06/02/2020 07:28 PM, Kris Van Hees wrote:

> On Fri, May 29, 2020 at 08:10:39PM -0400, eugene.loh at oracle.com wrote:
>> From: Eugene Loh <eugene.loh at oracle.com>
>>
>> Signed-off-by: Eugene Loh <eugene.loh at oracle.com>
>> ---
>>   test/unittest/providers/tst.dtrace_cleanup.sh | 43 +++++++++++++++++++
>>   1 file changed, 43 insertions(+)
>>   create mode 100755 test/unittest/providers/tst.dtrace_cleanup.sh
>>
>> diff --git a/test/unittest/providers/tst.dtrace_cleanup.sh b/test/unittest/providers/tst.dtrace_cleanup.sh
>> new file mode 100755
>> index 00000000..ee4892ff
>> --- /dev/null
>> +++ b/test/unittest/providers/tst.dtrace_cleanup.sh
>> @@ -0,0 +1,43 @@
>> +#!/bin/bash
>> +#
>> +# Oracle Linux DTrace.
>> +# Copyright (c) 2020, Oracle and/or its affiliates. All rights reserved.
>> +# Licensed under the Universal Permissive License v 1.0 as shown at
>> +# http://oss.oracle.com/licenses/upl.
>> +
>> +##
>> +#
>> +# ASSERTION:
>> +# The dtrace probes BEGIN and END will create pid-tagged uprobes that
>> +# will be cleaned up when the dtrace process exits.
>> +#
>> +##
>> +
>> +dtrace=$1
>> +UPROBE_EVENTS=/sys/kernel/debug/tracing/uprobe_events
>> +
>> +$dtrace $dt_flags -n 'BEGIN {trace(1234)} END {trace(4321)}' &
> Any reason why you added trace() actions?  I don't think they are needed for
> this test.

No particular reason.  By habit, I think it's nice to produce output, 
but I agree that output plays no real role here.  I can remove the actions.

>> +pid=$!
>> +
>> +sleep 1
> This could be a problem in some cases - if the system is bogged down it could
> potentially happen that it takes more than a second for probes to get enabled.
> A more guaranteed test could redirect the output from the dtrace process to a
> file and look for the message that states that the probes have been enabled.

You mean spin on the file until it appears?  I guess I don't know what 
mechanism to favor here.

>> +BEG0=`grep -c p:dt_${pid}_dtrace/BEGIN $UPROBE_EVENTS`
>> +END0=`grep -c p:dt_${pid}_dtrace/END   $UPROBE_EVENTS`
>> +
>> +kill $pid
>> +
>> +sleep 1
> Perhaps it would be better to wait on $pid, so you know for sure that the
> process is gone?  That way you can avoid the arbitrary time limit.

Yeah.  Guess so.  I'll change this.

>> +BEG1=`grep -c p:dt_${pid}_dtrace/BEGIN $UPROBE_EVENTS`
>> +END1=`grep -c p:dt_${pid}_dtrace/END   $UPROBE_EVENTS`
>> +
>> +if [[ $BEG0 != 1 || \
>> +     $END0 != 1 || \
>> +     $BEG1 != 0 || \
>> +     $END1 != 0 ]]; then
>> +	echo "problem"
> I would remove this - the "exit 1" already signals that the test fails, as
> will the disccrepancy between the actual results and the expected results..

You mean remove the "echo problem"?  Seems inconsequential, but I can 
remove it.

> Alternatively you could use the expected results file feature of the testsuite
> engine, but I personally do prefer your version of testing the outcome in the
> test itself.

Great.  We'll stick to the in-test check.

>> +	echo "actual: $BEG0 $END0 $BEG1 $END1"
>> +	echo "expect: 1 1 0 0"
>> +	exit 1
>> +fi
>> +
>> +exit 0
>> +