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), 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. ----- 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. ----- 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. ----- 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? (Not that most shell scripts should be getting either one, unless there's some builtin to make the needed ioctl calls.) ----- http://cr.opensolaris.org/~gisburn/ksh93_integration_update2_20090420_webrev/usr/src/lib/libshell/common/scripts/rssread.sh.html 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). (A further enhancement even later would recognize file: as not a network protocol at all.) ----- 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-) ----- 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. ----- 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. ----- 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.? 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' 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 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? -- -Alan Coopersmith- alan.coopersmith at sun.com Sun Microsystems, Inc. - X Window System Engineering
