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

[Roland Mainz]

On 9/16/06, Dan Price <dp at eng.sun.com> wrote:
> 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.

Erm... no... I do not hate "flag days" in general - I only do not like
to create a "flag day" when there is no convincing reason. I do not
want to cause unneccesary work for half of Sun engineering - that's
all.

> And
> altering libc to depend upon ksh93 would not AFAIK need to be arc'd
> (although maybe it would, I'm not sure).

Unfortunately it needs to be ARC'ed because |libc::wordexp()| uses
/usr/bin/ksh for normal applications and /usr/xpg4/bin/sh for
XPG4-compilant applications. ksh93 is a XPG4 shell which means we
would force the XPG4 behaviour in all cases. The difference is very
subtle - the current /usr/xpg4/bin/sh is build from the same sources
as /usr/bin/ksh (with some of #ifdef's) and for most cases they behave
identical. It's hard to find a difference and running into a problem
with |wordexp()| is even harder. The number of consumers of
|wordexp()| and the current usage make it even more unlikely to hit a
difference. But it still needs to be ARC'ed to make the burocracy
happy... ;-(

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

Erm... the code is not enabled by default right now and AFAIK does not
need to be ARC'ed (April or Mike may correct me). Otherwise we would
have to ARC the full migration of /usr/bin/ksh to ksh93 in one step
(which is considered impossible to do in one step by almost all Sun
people I've talked to) and likely the migration of /sbin/sh, too
(there is a switch for that, too).

And note this code urgendly needed for the /usr/bin/ksh migration to
ksh93. If ARC asks whether we did propper testing then our answer MUST
be "yes". And this answer should be supported by one or more
OpenSolaris distributions which already have this switch turned "on"
(AFAIK most of them (except Solaris) are just waiting to turn the
switch "on" once it hits the OS/Net tree) for a LONG time (e.g. one,
two, three years...). This is one of the most critical - if not THE
most critical item for the /usr/bin/ksh migration to ksh93. WIthout
propper testing we can cancel this project because we would have zero
chanches to succeed.

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

The code in |libc::wordexp()| works and has been tested over a few
months (note: The later addition of the 64bit bits may have damaged
the Makefile stuff but I am going to test&fix that when the switch to
B48 is done), that is not my concern as programmer. But as admin I
have a feeling in my stomach that we shouldn't do it with the initial
putback. The next problem is that such a step needs to be ARC'ed (see
above) which will likely only work as part of the full migration of
/usr/bin/ksh to ksh93 (the testing in this area is almost identical
and IMO doing twice the work is...uhm... stupid) which turns this into
a chicken&egg problem.

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

I am sure the code in |libc::wordexp()| works. The "risk" is that
somehow the installation of libc goes wrong (which is one of the
reasons why this in the OS/Net - trying to get everyone to follow
detailed installation description turned-out to be impossible and
doing even one minor item wrong results in a totally broken OS
installation). For example for x86 you have to patch all four versions
(plain i386, i386/hw1, i386/hw2 and AMD54), otherwise some or all of
the applications use the wrong version and blow up. And if this
happens with SMF you're running into very very ugly problems with
getting the machine back to a normal state (excluding the option to
reinstall the machine from scratch).

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

Erm... |fork()| uses "copy on write" in Solaris ? What's the problem here ?

> That said, as long as you're not
> inducing a significant slowdown compared to the existing implementation,
> it's a low priority issue.

AFAIK the new version should be faster (excluding the isaexec issue)
because the shell itself is faster and the startup footprint is
smaller than /usr/bin/ksh (assuming ksh93 and it's libs are already in
memory), thanks to things like lazyloading of the larger libraries
like libnsl/libsocket/etc., "-xstrconst" and other optimisations
(after the first putback we will take a closer look at startup time
and do more work for ksh93s).

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

Small correction: I think it's in the ARC case (at least the manpage
for |getusershell()| AFAIK has it) but I am not sure anymore and I
cannot check it right now (I am a few hundred km away from my office,
writing this email on my laptop and don't have April's ARC draft
here... ;-(( ).

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

IMO it would be not usefull to exclude "rksh" and "rksh93" in the
default list of |getusershell()| since you can always stick "set -o
restricted" in /etc/profile (this is a ksh93 feature that you can turn
the "restriced" mode on dynamically (but you cannot turn it off once
you are restricted)).

----

Bye,
Roland

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