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

[Dan Price]

On Sat 16 Sep 2006 at 04:28AM, Roland Mainz wrote:
> 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).

I know you hate flag days but there would be no flag day here.  And
altering libc to depend upon ksh93 would not AFAIK need to be arc'd
(although maybe it would, I'm not sure).  If it *does* require ARC'ing
then I would argue that integrating code which you expect some people to
enable by ifdef would constitute something you would have to ARC
*anyway*, and so your existing putback would need to document it.

> 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)).

Either you should be confident in what you put back, or don't put it
back.  I prefer not to have code under ifdef which we're not testing and
which might or might not work.  You can trivially come back and add
this in a later step when your team has done the needed testing.

Anyway, Pure-OpenSolaris distros will use this code, right?  Aren't
you creating risk for them if you're not sure this works?

> >         - 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...
> ;-/ ) ...

There may be any number of customer applications out there using this
public API.  Anyway, my suggestion about vfork is so that some poor guy
who has 2GB of anon mapped doesn't have to suffer the hit of forking
that (or running out of reserve).  That said, as long as you're not
inducing a significant slowdown compared to the existing implementation,
it's a low priority issue.

> ) ? If I remeber it correctly the ARC case already covers the addition
> of "rksh" ...

If you're arcing that, then fine.  I assumed that this was an
unrelated issue that you just happened to find.  Sounds like not.

        -dp

-- 
Daniel Price - Solaris Kernel Engineering - dp at eng.sun.com - blogs.sun.com/dp