Unfortunately, I can't really help out with the code review, but I will
offer up a few comments/questions/suggestions, based solely on the
message below.
1) You should remove ident lines now -- you won't be allowed to put them
back and fix them later. (Not unless you get some kind of special
waiver from ON CRT, and I really don't think one should be granted.)
2) As far as your builtins go, I have a couple of concerns, which *may*
come up at ARC. So by properly answering these ahead of time (or at
least knowing the answers), you'll save trouble later at ARC:
a) Are you 100% certain that the builtins are 100% drop-in
compatible with the old versions? (I'm not talking about adding new
options -- I'm saying no change where the old commands did *not*
generate an error of some sort.) Note that I suspect for printf at
least, you'll have some formats which now will be handled differently.
For example, printf "%z" prints "z", instead of returning an error.
While it would be insane for anyone to rely on that behavior (and so it
shouldn't be a problem at ARC), any new format specifiers or
interpretation would need to be documented in the ARC materials.
b) The normal perf. concerns. The effect of your change may be
positive, but some kind of analysis would be useful. (And I'm not
talking about your atypical scientific computing applications. I'm
talking about impact to things that folks are already doing. A good
perf. test might be building ON -- with *stock* Makefiles. Just because
you can tune it for ksh93 doesn't mean everyone will do so, so what I
want to know is how "untuned" applications will behave. Boot time
analysis may be helpful as well.
c) kill -l change is slight, but seems also to be "arbitrary". Is
there a reason that the old SPACE behavior isn't retained?
3) I've not looked at sum, but are you making use of (or later plan to
make use of) the encryption framework in Solaris? The Solaris
encryption framework has highly optimized (CPU-specific!)
implementations for various ciphers and hash algorithms, as well as
hardware offload support. Compare the perf. of digest(1) with your
implementation of sum, for example. Run time comparisons against
builtin sum are important as well, as the old code is probably quite
heavily used in automated scripts and such. (Run against a big file...
I don't care so much about startup overheads in this case, as I do about
time to sum a 4GB iso file.)
4) Will we finally get open man pages for these commands as well? "man
type" is not terribly useful on OpenSolaris 2008.05.
-- Garrett
Roland Mainz wrote:
> Hi!
>
> ----
>
> <NOTE>Please note the _timeout_ is set to 2008-08-25 since we don't have
> much time left anymore, after that point the code will move to a
> Mercurial tree for final review and integration.</NOTE>
>
> Here comes round "two" of the preliminary code review for the first
> update of the ksh93 integration project: I created a webrevs based on
> today's (2008-08-22) ksh93-integration
> prototype012 Subversion tree which is based on the OS/Net Mercurial HEAD
> version.
>
> The webrev can be found at
> http://cr.opensolaris.org/~gisburn/ksh93_integration/ksh93_update1/webrev_20080821_001/
>
>
> ** Notes/comments:
> * "ident" lines will be removed either later or with a seperate putback
> immediately following this one to avoid constant bitrot in our tree by
> the currently "ident"-lint hunt in OS/Net
>
> * Most of the files cover a simple update but we have lots of new code,
> including:
> - usr/src/cmd/ksh/ now contains a subdirectory builtins/ which handles
> the mapping the commands "alias", "command", "fc", "fg", "getopts",
> "hash", "jobs", "kill", "printf", "read", "rev", "sleep", "sum", "test",
> "type", "ulimit", "umask", "unalias", "wait" (most of them were
> previously mapped to /usr/bin/ksh)
> - usr/src/cmd/shcomp/ contains the ksh93 shell script compiler
> - usr/src/cmd/nsadmin/ksh.kshrc contains a change which provides a
> default "PS1" based on a similar prompt delivered by SuSE. The change
> may be commented out in the final version to simplify the ARC case
> - usr/src/lib/libsum/ contains a new library from AT&T which handles
> cipher hashing method for the ksh93 "sum" builtin
> - usr/src/uts/ contains a new exec kernel module to recognize compiled
> shell scripts (generated by "shcomp" ; this module works in a similar
> way as the "javaexec" exec module)
>
> * The following utilties are affected by the change:
> - /usr/bin/kill's output for the "-l" option now uses newline as
> delimiter for values instead of a SPACE (both standard and the manual
> page explicitly allow both forms)
> - /usr/bin/printf is now mapped to ksh93's builtin command (e.g. another
> piece of closed-source software is gone and we get support for C99/XPG6,
> too... :-) )
> - /usr/bin/sleep is now mapped to ksh93's "sleep" builtin (which finally
> provides sub-second timeouts and C99/XPG6 conformance)
> - /usr/bin/sum is now mapped to ksh93's "sum" builtin (which includes
> lots of new functionality, including selectable ciphers)
> - /usr/bin/test now supports additional options and sub-second
> timestamps for files (ksh88 only supported 1sec granularity)
> - /usr/bin/ulimit's output for the "-a" option has changed, however the
> output is considered "not an interface", e.g. only for informative
> purposes
>
> Thanks in advance for your help!
>
> ----
>
> Bye,
> Roland
>
> P.S.: Rely-To: is set to ksh93-integration-discuss at opensolaris.org
>
>
