Mike Gerdts wrote: > > 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.
Right... see below. Technically most platforms already have ksh93 as /usr/bin/ksh (e.g. Linux, FreeBSD, MacOS X, OpenSolaris/Indiana etc., Solaris <= 10 uses ksh88) but there may be still be some platforms with hacked pdksh clones around (but I doubt that those who have ported Dtrace fall into this category since "pdksh" is unmaintained since many years, leaving "ksh93" as the only maintained and actively-under-development Korn Shell version around) ... ;-( > - 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? Erm... please define "accepted standard" ... (Note that I started the shell style guide document out of desperation how ksh features are "abused" (which usually leads to bad performace (something which is unneccesary since even complex applications executed by ksh93 version 't' can reach the same level of performance of equivalent JAVA apps)) and not about stuff like whether functions are intended with one tab or four spaces (maybe we should rename the document from "style guide" to "coding guidelines" some day...)). > 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. :) Ok... > 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. Mhhh... what about changing the "sh-bang"-line to "#!/opt/SUNWdtrt/bin/dtest -s" then where "-s" means "interpret the given test script" ? > > 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? I am not sure. At least Linux has seperate /usr/bin/ and /bin/ directories while Solaris's /bin/ is a softlink to /usr/bin/ ... but I don't know how FreeBSD/NetBSD/DragonFly's filesytem layout looks like. [snip] > >> +# 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)? The "==" operator was added in ksh88 version 'g', unfortunately the ksh(1) manual page was never updated to reflect this (it appears to be at ksh88 version 'e' level while the binary is a hacked ksh88 version 'i'). See http://mail.opensolaris.org/pipermail/ksh93-integration-discuss/2008-July/006333.html for a short evaluation. > ksh(1) and test(1) could use an update. Known problem - http://bugs.opensolaris.org/view_bug.do?bug_id=6753139 ('ksh(1) does not document the "==" operator in [[ ]]') was filed to address this documentation problem but currently the bug is "stuck" in "4-Defer:No Plan to Fix" (somehow I feel someone thought this is a ksh88 code bug and not a documentation issue and did the wrong bug triage in this case... ;-( ). [snip] > >> +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. BTW: I forgot to note that $ ksh83 -n <scriptname> # will primarity complain about POSIX shell standard violations (e.g. "depreciated" constructs), syntax errors and starting with ksh93 update1 some statements with "undefined behaviour", e.g. it will not enforce ksh93-specific features and not complain about stuff like "(( math >= 6 ))" vs. "(( $math >= 6 ))" (both forms are valid statements, however the 2nd form is slower (because ${math} will force a number-->string--->number conversion cycle) and causes base2--->base10--->base2 conversion rounding errors for floating-point values unless you use the C99 "hexfloat" format or use the (future) decimal floating-point datatype (not supported yet, we have to wait until Sun Studio implements IEEE754r))). > This would probably be a good check to get integrated > into a lint or similar build pass. Well, we already have the wrapper and infrastructure ready... the problem is that I still have no sponsor for the bug... ;-( > What variant of ksh should I expect to see on other platforms that > support dtrace? I don't have a full overview about all platforms which implement Dtrace. See above which platforms have ksh93 as /usr/bin/ksh, Solaris 10 still uses ksh88 as /usr/bin/ksh but we _may_ get ksh93 as /usr/bin/ksh93 in Solaris 10 at some point (short: the ksh93-integration codebase can easily be moved to Solaris 10 - the problem is to find a "paying customer" who throws some $$$$ into Sun's direction... ;-/ ). > Is the test suite something that needs to be portable > back to S10? Uhm... I don't know. If you need to be portable across multiple platforms with different Bourne-shell compatible shells the best option may be to stick with the feature set defined in the POSIX shell standard (the shell style guide has "labels" which give hints which fetures are shell specific - anything marked with "ksh" should work with the POSIX shell standard while stuff marked with "ksh93" is currently ksh93-specific). ---- Bye, Roland -- __ . . __ (o.\ \/ /.o) [EMAIL PROTECTED] \__\/\/__/ MPEG specialist, C&&JAVA&&Sun&&Unix programmer /O /==\ O\ TEL +49 641 3992797 (;O/ \/ \O;) _______________________________________________ dtrace-discuss mailing list dtrace-discuss@opensolaris.org
