Alan Coopersmith wrote: > Roland Mainz wrote: > > The webrev can be found a > > http://cr.opensolaris.org/~gisburn/ksh93_integration_update2_20090420_webrev/ > > I haven't done a full review, since there's so much there (so many of the > files > aren't really changed, just updated copyright or comment about what path you > generated them from),
Erm... that was my fault... I forgot to include the list of files which do not fall under the normal OS/Net review rules because they are either "upstream" or generated by other scripts. Basically "upstream" and generated files are in these directories: usr/src/lib/libshell/common/ usr/src/lib/libshell/(i386|amd64|sparc|sparcv9)/src/ usr/src/lib/libshell/(i386|amd64|sparc|sparcv9)/include/ usr/src/lib/libcmd/common/ usr/src/lib/libcmd/(i386|amd64|sparc|sparcv9)/src/ usr/src/lib/libcmd/(i386|amd64|sparc|sparcv9)/include/ usr/src/lib/libdll/common/ usr/src/lib/libdll/(i386|amd64|sparc|sparcv9)/src/ usr/src/lib/libsum/common/ usr/src/lib/libsum/(i386|amd64|sparc|sparcv9)/src/ usr/src/lib/libsum/(i386|amd64|sparc|sparcv9)/include/ usr/src/lib/libpp/common/ usr/src/lib/libpp/(i386|amd64|sparc|sparcv9)/src/ usr/src/lib/libast/common/ usr/src/lib/libast/(i386|amd64|sparc|sparcv9)/src/ usr/src/lib/libast/(i386|amd64|sparc|sparcv9)/include/ usr/src/cmd/ast/msgcc/*.(c|*sh) > so I just looked a bit, but here's some things I notice > (most of which are suggestions that could be rejected or deferred to a later > update): > > http://cr.opensolaris.org/~gisburn/ksh93_integration_update2_20090420_webrev/usr/src/cmd/ksh/builtins/alias.c.udiff.html > > If bfastpathrec is always kept in alphabetical order, then you could have > find_bfastpathrec stop searching as soon as strcmp returns > 0, but if you > add such an optimization you should also add a comment noting the requirement > for alphabetical ordering so future people adding to the list don't break it. Fixed. Originally I tried using hash values generated by a seperate script to do a fast lookup but after implementing I found that this some disadvantages: 1. The resulting compiled binary was more than twice as big. 2. The list is not big enougth that it makes a noticeable difference on an Ultra5 - the speed difference between |libc::getexecname()| and |libast::pathprog()| has a greater impact on performance than all the hash'ed list lookup (e.g. |libast::pathprog()| is much faster than |libc::getexecname()|). > http://cr.opensolaris.org/~gisburn/ksh93_integration_update2_20090420_webrev/usr/src/lib/libast/amd64/include/ast/ast_dir.h.udiff.html > and many others... > > Updating nothing in the file but the copyright date seems incorrect - you > can't > just extend copyright protection by a year without changing anything, or > copyright expiration dates become even more meaningless than they already are. Umpf... that's a generated include file... there is little we can do except starting to patch each single include file (we had that discussion for the initial ksh93-integration putback and the update2 putback and the final resolution was "we don't touch the generated headers" (it's too much "hassle" to patch them since the next update just stomps over the changes again and again)) > http://cr.opensolaris.org/~gisburn/ksh93_integration_update2_20090420_webrev/usr/src/lib/libcmd/common/mktemp.c.html > > This seems to be missing the -p & -u options of the Solaris /usr/bin/mktemp > (though -u is deprecated), so would need those added if you plan to > replace/bind > /usr/bin/mktemp in the future. Fixed... but we don't target "mktemp" for this putback nor bind it to /usr/bin/mktemp as builtin. > http://cr.opensolaris.org/~gisburn/ksh93_integration_update2_20090420_webrev/usr/src/lib/libshell/common/data/signals.c.html > > The default actions for SIGIO & SIGPOLL are different: > 113 "IO", VAL(SIGIO,SH_SIGIGNORE), > S("IO signal"), > 154 "POLL", VAL(SIGPOLL,SH_SIGDONE), > S("Polling alarm"), > > But on Solaris: > #define SIGIO SIGPOLL /* socket I/O possible (SIGPOLL alias) */ > > Is this going to cause issues? No, this shouldn't cause issues... but I added this to my list of things we need to fix for the next update. > (Not that most shell scripts should be getting > either one, unless there's some builtin to make the needed ioctl calls.) Right... but we're not there... yet (type plugins will quickly get us into that hell... but that's targeted for ksh93-integration update4). > http://cr.opensolaris.org/~gisburn/ksh93_integration_update2_20090420_webrev/usr/src/lib/libshell/common/scripts/rssread.sh.html Erm... this is _demo_ code... > line 124-127: should a special case be added for https since that's missing > from > Solaris /etc/services? Or is that pointless since you don't have SSL > support? > In which case, shouldn't you print a "SSL not supported" error if ${protocol} > == > "https" (or != "http" since that looks like all you support). Fixed - I added https support to rssread and crawlsrccomments.sh > (A further enhancement even later would recognize file: as not a network > protocol at all.) Fixed (however plain files still need the file://-prefix for now... but I revamp that code again for ksh93-integration update3). > http://cr.opensolaris.org/~gisburn/ksh93_integration_update2_20090420_webrev/usr/src/lib/libshell/common/scripts/shpiano.sh.udiff.html > > Did you have fixes for the issues with boomer that you were discussing with > gdamore on IRC? 8-) Half and half... I did some changes but not all of them (again, it's demo code... but I am going to fix the issues anyway if I have some free cycles). > http://cr.opensolaris.org/~gisburn/ksh93_integration_update2_20090420_webrev/usr/src/lib/libshell/common/sh/io.c.udiff.html > > @@ -715,10 +714,24 @@ > Why are you calling open with O_CREAT only if the file already exists? > That seems backwards. Erm... why ? > http://cr.opensolaris.org/~gisburn/ksh93_integration_update2_20090420_webrev/usr/src/pkgdefs/SUNWcsu/prototype_com.udiff.html > > Since you're moving several commands (comm, logname, mkfifo, paste, uniq) > from SUNWesu to SUNWcsu, you'll need to talk to the gatekeeper about whether > they'll need to deliver a pkghistory file so that upgrade does the right thing > there. Ok... > http://cr.opensolaris.org/~gisburn/ksh93_integration_update2_20090420_webrev/raw_files/new/usr/src/lib/libshell/misc/shell_styleguide.docbook > > Will there be alt strings in the image tags generated for the images of text > like "ksh88", "perf", etc.? Usually the docbook XSL style sheet should do that... > Typos: > alterantive => alternative > ambigous => ambiguous > arthmetrict => arithmetic > becase => because > carefull => careful > chanche => chance > charatcer => character > charatcers => characters > functionlity => functionality > guranteed => guaranteed > interfer => interfere > interractive => interactive > unneccesary => unnecessary > usefull => useful > writeable => writable > 'but $SECONDS in the PS4 prompt' => 'put $SECONDS in the PS4 prompt' Not fixed yet... I am doing that for the next update... > Many of my comments from last summer seem to still apply (including some of > the > above typos and some more typos not above): > http://mail.opensolaris.org/pipermail/shell-discuss/2008-June/001109.html I know... but documentation cleanup has currently a bit lower priority than all the code testing and therefore I hadn't time yet to fix this... sorry... ;-( > Should usr/src/prototypes/prototype.ksh be updated to start with > "#! /usr/bin/ksh93" and include the recommended set options at the start > of the body? And an "exit 0" at the end? Erm... that's AFAIK a question for gatekeepers... ---- Bye, Roland -- __ . . __ (o.\ \/ /.o) roland.ma...@nrubsig.org \__\/\/__/ MPEG specialist, C&&JAVA&&Sun&&Unix programmer /O /==\ O\ TEL +49 641 3992797 (;O/ \/ \O;) _______________________________________________ opensolaris-code mailing list opensolaris-code@opensolaris.org http://mail.opensolaris.org/mailman/listinfo/opensolaris-code
