Thanks for taking the time to review. There are two areas of concern with the changes you recommend:
- Portability. This test suite is intended to run on all platforms that Dtrace has been ported to. - Style. I started by copying another script in the same directory. Is the shell style document you reference the accepted standard such that it trumps consistency with other scripts in the directory? On Sun, Nov 16, 2008 at 5:08 PM, Roland Mainz <[EMAIL PROTECTED]> wrote: > Mike Gerdts wrote: >> >> I've posted an updated webrev at: >> >> http://cr.opensolaris.org/~mgerdts/6750659-2008-11-16/ >> >> Changes since the previous (11-15) webrev include the following >> changes to the test case. >> >> - Move the test case into the usdt directory >> - Fix packaging for the test case in SUNWdtrt >> - Remove leading tab for "all" target in generated Makefile > > Quick race over > http://cr.opensolaris.org/~mgerdts/6750659-2008-11-16/usr/src/cmd/dtrace/test/tst/common/usdt/tst.corruptenv.ksh.patch > (patch code is quoted with "> "): > -- snip -- >> --- /dev/null Sun Nov 16 15:39:56 2008 >> +++ new/usr/src/cmd/dtrace/test/tst/common/usdt/tst.corruptenv.ksh Sun >> Nov 16 15:39:55 2008 >> @@ -0,0 +1,107 @@ >> +#!/bin/ksh -p > > 1. Assuming your target is is only Solaris >= 10 the "-p" option is not > needed anymore. ok, but irrelevant. :) The script is not even delivered executable - /opt/SUNWdtrt/bin/dtest calls the script as an argument to ksh and as such the sh-bang line is really needed at all. > 2. Please use /usr/bin/, not /bin/ (saves a softlink lookup) > [snip] In the event that someone does chmod +x on the file and tries to run it on BSD or MacOS, will ksh be found in /bin, /usr/bin, or both? >> + >> +PATH=/usr/bin:/usr/sbin:$PATH >> + >> +if [[ $# != 1 ]]; then > > Please use (( $# != 1 )) in this case (see > http://www.opensolaris.org/os/project/shell/shellstyle/#avoid_unneccesary_string_number_conversions > - $# and $? are integer variables and using a string operator (e.g. "!=" > in [[ expr ]]) causes an extra integer--->string--->integer conversion) ok >> + print -u2 'expected one argument: <dtrace-path>' >> + exit 2 >> +fi >> + >> +# >> +# jdtrace does not implement the -h option that is required to generate >> +# C header files. >> +# >> +if [[ "$1" = */jdtrace ]] ; then > > Please replace "=" with "==" - the "=" operator is depreciated since a > while. Does this apply to the ksh implementation in other supported OS's (S10, BSD, MacOS)? ksh(1) and test(1) could use an update. >> + exit 0 >> +fi >> + >> +dtrace=$1 >> +startdir=$(pwd) >> +dir=$(mktemp -td drtiXXXXXX) >> +if [[ $? != 0 ]] ; then > > http://www.opensolaris.org/os/project/shell/shellstyle/#compare_exit_code_using_math ok >> + print -u2 'Could not create safe temporary directory' >> + exit 2 >> +fi >> + >> +cd $dir > > Please add double-quotes around "$dir" ... ok >> +cat > Makefile <<EOF >> +all: main >> + >> +main: main.o prov.o >> + \$(CC) -o main main.o prov.o >> + >> +main.o: main.c prov.h >> + \$(CC) -c main.c >> + >> +prov.h: prov.d >> + $dtrace -h -s prov.d >> + >> +prov.o: prov.d main.o >> + $dtrace -G -32 -s prov.d main.o >> +EOF >> + >> +cat > prov.d <<EOF >> +provider tester { >> + probe entry(); >> +}; >> +EOF >> + >> +cat > main.c <<EOF >> +#include <stdlib.h> >> +#include <sys/sdt.h> >> +#include "prov.h" >> + >> +int >> +main(int argc, char **argv, char **envp) >> +{ >> + envp[0] = (char*)0xff; >> + TESTER_ENTRY(); >> + exit(0); > > Erm... AFAIK this should be |return 0;| ok > >> +} >> +EOF >> + >> +make > /dev/null >> +status=$? >> +if [ $status -ne 0 ]; then > > Please change this to: > -- snip -- > if (( status != 0 )); then > -- snip -- ok > >> + print -u2 "failed to build" >> +else >> + ./main >> + status=$? >> +fi >> + >> +cd $startdir > > Please add double-quotes. ok > >> +rm -rf $dir >> + >> +exit $status > -- snip -- > > In general it may be nice to check whether the script passes $ ksh93 -n > <scriptname> # (there is at least one warning) ... fair enough. This would probably be a good check to get integrated into a lint or similar build pass. What variant of ksh should I expect to see on other platforms that support dtrace? Is the test suite something that needs to be portable back to S10? Again, thanks for taking the time to review. -- Mike Gerdts http://mgerdts.blogspot.com/ _______________________________________________ dtrace-discuss mailing list dtrace-discuss@opensolaris.org
