Requesting (preliminary) review from gatekeepers... / was: Re:[ksh93-integration-discuss] webrev status

[Roland Mainz]

Dan Price wrote:
> On Sat 16 Sep 2006 at 03:09AM, Roland Mainz wrote:
> > Mike Kupfer wrote:
> > > Okay, I've posted preliminary ksh93 webrev at
> > > http://cr.grommit.com/~kupfer/ksh93/.  As I mentioned in my earlier
> > > mail, there are a few entries in the webrev that are broken (e.g.,
> > > usr/src/lib/libcmd/sparcv9/Makefile), but everything else that I've
> > > looked at looks okay.
> 
> First pass of comments from looking at this:
> 
> wordexp:
>         - What's the point of conditionally supporting ksh93 wordexp?
>           It looks from your packaging like ksh93 will always be
>           present... so, why not use the ksh93 wordexp always?  I don't
>           see the value of having to maintain two versions of this.
>           That would eliminate a lot of the complexity you are adding.

We have three issues here:
1. The initial putback of ksh93 ONLY aims at introducing ksh93 and the
related libraries (libshell, libcmd, libdll and libast) and NOTHING
else. This should avoid problems (such as having a "flag day" (or worse
things)), keep the ARC case simple and allow an 1:1 backport of the
patch to older Solaris version (e.g. Solaris 10).

2. The ksh93 version of |libc::wordexp()| was added that OpenSolaris
distributions like Nexgenta, SchillyX etc. do not need to ship+use the
closed-source (=evil, 666, bad etc.) "/usr/bin/ksh". The idea is to have
a build switch which controlls whether ksh93 is installed as
/usr/bin/ksh or not and use ksh93 only for |libc::wordexp()| if the old
Solaris ksh is not available.

3. Playing around with |libc::wordexp()| is extraordinary dangerous. If
|libc::wordexp()| doesn't work then SMF will not work properly. In the
worst case SMF crashes, resulting in a machine which cannot change
runlevels anyore - no shutdown, no booting, no escape path. The ksh93
version of |libc::wordexp()| was tested and works, howver I'd like to
minimise the risk of the initial putback as much as possible and turn
this switch "on" as a seperate, later step (which will require that
April runs the matching standard test suites which cover
|libc::wordexp()| (e.g. whether this is 100% POSIX conformant) and there
wasn't any time left yet to do that (short: it's compliciated)).

>         - (an aside is that it's awfully expensive to do wordexp()
>            this way and it's too bad that we're reimplementing an old
>            behavior which was a bad idea to begin with.)

Known problem. The idea is to use libshell (which is ksh93 made
available as shared library - this would completely eliminate the need
for a |fork()|) - however the current libshell API is not threadsafe nor
reentrant and both things need to be addressed first (this is
unfortunately lots of work but we're madly hacking around in this area
right now...).
 
>         - Please don't do #ifdef DEBUG_WORDEXP.  Don't have conditional
>           code around like this.  It just rots.

Ok... I'll remove that next week (but see the SMF issue above. It helps
a lot when SMF has hiccups again...). ...

>         - This seems like a textbook case for using strlcat()
> 
>         - Since it uses fork, this is going to be quite costly.  Could
>           this be done with posix_spawn() instead? (which is vfork()
>           based).  I realize that the current wordexp() has the
>           same issue.

I am not sure how ksh93 will behave in this case (actually we call
isaexec+ksh93 (/usr/bin/ksh93 is a hardlink to isaexec which calls
either a 64bit or a 32bit version of ksh93 depending on whether the
machine+OS is 64bit capable) - and in this case the |vfork()| may be
only a small fraction...) ...
Note that |libc::wordexp()| is only called a few times during system
boot - one per entry for inetd services and that's all (e.g. any
performance optimisations are more or less for /dev/null in this case...
;-/ ) ...

> getusershell.c:
>         - You're adding rksh here (I assume it was just an
>           oversight that it was missing).  Is there a separate
>           bugid filed for that?

AFAIK no... should I file one (in the same bug we may want to create
/usr/rbin/ as location for binaries/scripts which are allowed in
restricted shells - the idea is from the ksh93 manual page which says:
-- snip --
     The system administrator often sets up a directory  of  com-
     mands  (e.g.,  /usr/rbin)  that  can  be  safely  invoked by
     rsh/rksh.
-- snip --
) ? If I remeber it correctly the ARC case already covers the addition
of "rksh" ...

> other:
>         - What contents will be in llib-lcmd.ln?  In the webrev
>           it shows up as:
> 
>           ---- ---- --- --- --- --- usr/src/lib/libcmd/common/llib-lcmd

Ouch... that's empty (and it shouldn't be empty) ... something went
wrong with that file... ;-((
... I'll look at that issue next week...

----

Bye,
Roland

P.S.: Just for the log: I am away this weekend and back on tuesday...

-- 
  __ .  . __
 (o.\ \/ /.o) roland.mainz at nrubsig.org
  \__\/\/__/  MPEG specialist, C&&JAVA&&Sun&&Unix programmer
  /O /==\ O\  TEL +49 641 7950090
 (;O/ \/ \O;)