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

[Dan Price]

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.

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

        - Please don't do #ifdef DEBUG_WORDEXP.  Don't have conditional
          code around like this.  It just rots.

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

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?

other:
        - What contents will be in llib-lcmd.ln?  In the webrev
          it shows up as:

          ---- ---- --- --- --- --- usr/src/lib/libcmd/common/llib-lcmd

Thanks,

        -dp

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