Hi Roland, Thank you for your code review. As others have mentioned, the items from your comments 1-4 will not be fixed, since those files will be removed in the next day or two.
I will incorporate your comments, item 5, on the tools/extract_postrun script. However, since this doesn't really affect functionality at this time, I am planning to postpone it until we got the other needed functionality of the project done. So, I probably won't get to it till sometime next week. When I do have the fix done, I will send out another code review and you can take a look at my changes. Thanks, --Karen Roland Mainz wrote: > Karen Tung wrote: > [snip] > >> The webrev is here: >> >> http://cr.opensolaris.org/~ktung/postrun/ >> > [snip] > > Only a quick look at > http://cr.opensolaris.org/~ktung/postrun/postrun_fix.patch ... > > 1. new/src/bootcd_skel/etc/profile is _old_ and predates the > ksh93-integration putback. IMO it may be nice to sync with B72 or later > (it should include "ksh93" in all cases where "ksh" is listed, too). > > > > 2. The patch seems to miss /etc/ksh.kshrc which is important for ksh93 > (see PSARC 2006/587 for the background. Short: Without this file ksh93 > will not enable any editor mode (for strict POSIX shell conformance - we > work around this usability issue by enabling the "gmacs" editor mode in > that config file)) > > > > 3. new/src/bootcd_skel/lib/opengl/ogl_select/nvidia_vendor_select > >> +PATH=/usr/bin:/usr/sbin:/usr/openwin/bin:/usr/X11/bin >> + >> +# >> +# No support for SPARC, yet >> +# >> +if [ `uname -p` = "sparc" ]; then >> + return 0 >> +fi >> > > Please use $( ... ), not ` ... ` (see > http://www.opensolaris.org/os/project/shell/shellstyle/#use_posix_subshell_syntax). > > > >> +MYIDENTITY="NVDAnvda nvidia" >> + >> +# If this is just a probe, identify ourself and leave. >> +if [ $# -eq 1 ]; then >> > > Please use math expressions, e.g. > -- snip -- > if (( $# == 1 )); then > -- snip -- > (see > http://www.opensolaris.org/os/project/shell/shellstyle/#use_posix_arithmetric_expressions) > > >> + if [ $1 = "identify" ]; then >> > > Please use [[ ]] (see > http://www.opensolaris.org/os/project/shell/shellstyle/#use_ksh_test_syntax) > > >> + echo $MYIDENTITY >> > > Quotes missing around "$MYIDENTITY" - and please use "print", not "echo" > in ksh88/ksh93 scripts (see > http://www.opensolaris.org/os/project/shell/shellstyle/#use_print_not_echo). > > >> + return 0 >> + fi >> +fi >> + >> +# Make a directory link. $1 is the pathname. >> +make_dir_link() { >> > > Please use "function make_dir_link", not "make_dir_link()" - see > http://www.opensolaris.org/os/project/shell/shellstyle/#use_ksh_style_function_syntax > (very important for ksh scripts) > > >> + if [ $# != 1 ]; then >> > > Please use (( $# != 1 )) when comparing numbers (see > http://www.opensolaris.org/os/project/shell/shellstyle/#use_posix_arithmetric_expressions) > ... > > >> + return >> + fi >> + if [ ! -d $1 ]; then >> > > Please use [[ ]] ... > > >> + mkdir -p $1 >> > > Please put quotes around "$1" ... > > >> + fi >> + chmod 755 $1 >> +} >> + >> +dir_init() { >> > > Please use "function dir_init", not "dir_init()" - see above... > > >> + make_dir_link $LINKDIR >> + make_dir_link $LINKDIR/lib >> + make_dir_link $LINKDIR/lib/amd64 >> + make_dir_link $LINKDIR/include >> + make_dir_link $LINKDIR/server >> + make_dir_link $LINKDIR/server/amd64 >> > > Quotes around "$LINKDIR/lib" would be nice... > > >> +} >> + >> +# Make a file link. $1 is the source path, $2 is the target path >> +make_file_link() { >> > > Please use "make_file_link", not "make_file_link()" - see above... > > >> + if [ $# != 2 ]; then >> > > Please use (( )) when comparing numbers (see above). > > >> + return >> + fi >> + if [ -h $2 ]; then >> > > Please use [[ ]] > > >> + rm -f $2 >> > > Please put quotes around "$2" to avoid that "rm" can run amok by > accident... > > >> + fi >> + ln -sf $1 $2 >> > > Please put quotes around both "$1" and "$2" (see "amok run" issue > above). > > >> +} >> + >> +dir_init >> + >> +# User libraries >> +make_file_link /usr/X11/lib/NVIDIA/libGL.so.1 $LINKDIR/lib/libGL.so.1 >> +make_file_link /usr/X11/lib/NVIDIA/amd64/libGL.so.1 >> $LINKDIR/lib/amd64/libGL.so.1 >> > > Quotes, please > > >> + >> +# Server libraries >> +make_file_link /usr/X11/lib/modules/extensions/NVIDIA/libglx.so >> $LINKDIR/server/libglx.so >> +make_file_link /usr/X11/lib/modules/extensions/NVIDIA/amd64/libglx.so >> $LINKDIR/server/amd64/libglx.so >> > > Quotes, please > > >> + >> +# Includes >> +make_file_link /usr/X11/include/NVIDIA/GL/gl.h $LINKDIR/include/gl.h >> +make_file_link /usr/X11/include/NVIDIA/GL/glext.h $LINKDIR/include/glext.h >> +make_file_link /usr/X11/include/NVIDIA/GL/glx.h $LINKDIR/include/glx.h >> +make_file_link /usr/X11/include/NVIDIA/GL/glxext.h $LINKDIR/include/glxext.h >> > > Quotes, please > > >> +# Devices for livemedia >> +if [ -f /.livecd ] >> > > Please use [[ ]] instead of [ ] > > > > 4. new/src/bootcd_skel/root/installer/install-finish > >> --- /dev/null Wed Oct 24 23:56:39 2007 >> +++ new/src/bootcd_skel/root/installer/install-finish Wed Oct 24 >> 23:56:39 2007 >> @@ -0,0 +1,415 @@ >> +#!/bin/ksh >> > [snip] > >> +BASEDIR=$1 >> > > Please put quotes around "$1" ... > > >> +INSTALL_TYPE=$2 >> > > Quotes... > > >> +BOOTENVRC=$BASEDIR/boot/solaris/bootenv.rc >> > > Quotes... > > >> +GRUBMENU=$BASEDIR/boot/grub/menu.lst >> +ALTGRUBMENU=$BASEDIR/stubboot/boot/grub/menu.lst >> + >> +set_boot_active() >> > > Please use "function set_boot_active", not "set_boot_active()" - see > http://www.opensolaris.org/os/project/shell/shellstyle/#use_ksh_style_function_syntax > (very important for ksh scripts) > > >> +{ >> + RAW_SLICE="$1" >> + >> + TMP1=/tmp/.set_active.1.$$ >> + TMP2=/tmp/.set_active.2.$$ >> + >> + # RAW_SLICE is a /dev path >> + # >> + echo "$RAW_SLICE" | grep "p0:boot$" > /dev/null 2>&1 >> + if [ "$?" -eq 0 ]; then >> > > Please use (( )) when comparing numbers, e.g. please replace this with > -- snip -- > if (( $? == 0 )); then > -- snip -- > > >> + P0=`echo "$RAW_SLICE" | sed 's/p0:boot$/p0/g'` >> > > Please use $( ... ) and not `...` > > >> + else >> + P0=`echo "$RAW_SLICE" | sed 's/s.$/p0/g'` >> > > Please use $( ... ) and not `...` > > >> + fi >> + >> + fdisk -W "$TMP1" "$P0" >> + grep -v \* "$TMP1" | grep -v '^[ ]*$' > "$TMP2" >> + rm -f "$TMP1" >> + >> + # make sure there is a Solaris partition before doing anything >> + # >> + awk '{ >> + if ( $1 == "130" ) exit 10 >> + else if ( $1 == "191" ) exit 10 >> + } ' "$TMP2" >> + if [ $? != 10 ] ; then >> > > Please replace this woth > -- snip -- > if (( $? != 10 )) ; then > -- snip -- > > >> + rm -f "$TMP2" >> + return 0 >> + fi >> + >> + # if there is a Solaris2 partition, set it active, otherwise >> + # set the Solaris (130 aka Linux swap active) >> + # >> + awk '{ print $1 }' "$TMP2" | grep 191 > /dev/null >> + if [ $? = 0 ] ; then >> > > Please replace this woth > -- snip -- > if (( $? == 0 )) ; then > -- snip -- > > >> + awk '{ >> + if ( $1 == "191" ) >> + printf "%s 128 %s %s %s %s %s %s %s %s\n", $1, \ >> + $3, $4, $5, $6, $7, $8, $9, $10 >> + else printf "%s 0 %s %s %s %s %s %s %s %s\n", \ >> + $1, $3, $4, $5, $6, $7, $8, $9, $10 >> + }' "$TMP2" > "$TMP1" >> + else >> + awk '{ >> + if ( $1 == "130" ) >> + printf "%s 128 %s %s %s %s %s %s %s %s\n", $1, \ >> + $3, $4, $5, $6, $7, $8, $9, $10 >> + else printf "%s 0 %s %s %s %s %s %s %s %s\n", \ >> + $1, $3, $4, $5, $6, $7, $8, $9, $10 >> + }' "$TMP2" > "$TMP1" >> + fi >> + >> + fdisk -F "$TMP1" "$P0" >> + >> + rm -f "$TMP1" >> + rm -f "$TMP2" >> +} >> + >> +add_failsafe_menu() >> > > Please use "function add_failsafe_menu", not "add_failsafe_menu()" - see > http://www.opensolaris.org/os/project/shell/shellstyle/#use_ksh_style_function_syntax > (very important for ksh scripts) > > >> +{ >> + RDSK="$1" >> + bootadm update-menu -R $BASEDIR -o $RDSK >> + >> + # Check and update menu.lst in /stubboot >> + # >> + if [ -n "$ENT" ]; then >> > > Plese use [[ ]] instead of [ ] ... > > >> + bootadm update-menu -R $BASEDIR/stubboot -o $RDSK,$BASEDIR >> + fi >> +} >> + >> +# fix the failsafe menu to redirect console to tty. >> +fix_failsafe_menu() >> > > Please use "function fix_failsafe_menu", not "fix_failsafe_menu()" - see > http://www.opensolaris.org/os/project/shell/shellstyle/#use_ksh_style_function_syntax > (very important for ksh scripts) > > >> +{ >> + MENUFILE="$1" >> + >> + # convert multiboot to dboot >> + grep "/boot/multiboot kernel/unix -s" $MENUFILE > /dev/null 2>&1 >> + if [ $? = 0 ]; then >> > > Please replace this with > -- snip -- > if (( $? == 0 )); then > -- snip -- > > >> + sed "s#/boot/multiboot kernel/unix >> -s#/boot/platform/i86pc/kernel/unix -s#" $MENUFILE > $MENUFILE.new >> + cat $MENUFILE.new > $MENUFILE >> + rm $MENUFILE.new >> + fi >> + >> + # set failsafe console >> + grep "/boot/platform/i86pc/kernel/unix -s -B console=" $MENUFILE \ >> + > /dev/null 2>&1 >> + if [ $? = 0 ]; then >> > > Please replace this with > -- snip -- > if (( $? == 0 )); then > -- snip -- > > >> + case "$osconsole" in >> + tty[ab]) >> + sed "s#/boot/platform/i86pc/kernel/unix >> -s#/boot/platform/i86pc/kernel/unix -s -B console=${osconsole}#" $MENUFILE > >> $MENUFILE.new >> + cat $MENUFILE.new > $MENUFILE >> + rm $MENUFILE.new >> + ;; >> + esac >> + fi >> +} >> + >> +# bootpath may not be present in bootenv.rc after installing S10 FCS. >> +# Fix it here so system boots correctly following an upgrade >> +fix_bootpath() >> > > Please use "function fix_bootpath", not "fix_bootpath()" - see > http://www.opensolaris.org/os/project/shell/shellstyle/#use_ksh_style_function_syntax > (very important for ksh scripts) > > >> +{ >> + grep "^setprop[ ]\{1,\}bootpath" $BOOTENVRC > /dev/null >> + if [ $? = 0 ]; then >> > > Please replace this with > -- snip -- > if (( $? == 0 )); then > -- snip -- > > >> + return >> > > Please define an exit code for "return" (0, right ?) ... > > >> + fi >> + >> + rootdev=`grep -v "^#" $BASEDIR/etc/vfstab | \ >> + grep "[ ]/[ ]" | nawk '{print $1}'` >> > > Please use $( ... ) and not `...` > > >> + bootpath=`ls -l $rootdev | nawk '{ print $11 }' |\ >> + sed -e 's#[./]*/devices/#/#'` >> > > Please use $( ... ) and not `...` > > >> + echo "setprop bootpath $bootpath" >> $BOOTENVRC >> > > Please use quotes around "$BOOTENVRC" ... > > >> +} >> + >> +# no bootpath needed for zfs boot. >> +# XXX blatant hack: _setup_bootblock should be fixed >> +# in the spmisvc library to not put bootpath in bootenv.rc >> +# in the first place for zfs boot >> +remove_bootpath() >> +{ >> + grep "^setprop[ ]\{1,\}bootpath" $BOOTENVRC > /dev/null >> + if [ $? = 0 ]; then >> > > Please replace this with > -- snip -- > if (( $? == 0 )); then > -- snip -- > > > >> + sed '/^setprop[ ][ ]*bootpath[ ]/d' \ >> + $BOOTENVRC > $BOOTENVRC.tmp >> > > Please use quotes and '{'/'}' around the variable name, e.g. > -- snip -- > sed '/^setprop[ ][ ]*bootpath[ ]/d' \ > "$BOOTENVRC" > "${BOOTENVRC}.tmp" > -- snip -- > > >> + mv $BOOTENVRC.tmp $BOOTENVRC >> > > Please replace this with > -- snip -- > mv "${BOOTENVRC}.tmp" "$BOOTENVRC" > -- snip -- > (same issue as above, e.g. quotes and '{'/'}'). > > >> + fi >> +} >> + >> +# since the root device might be a metadevice, all the components need to >> +# be located so each can be operated upon individually >> +# >> +get_rootdev_list() >> > > Please use "function get_rootdev_list", not "get_rootdev_list()" - see > above... > > >> +{ >> + rootfstype=`grep -v "^#" $BASEDIR/etc/vfstab | \ >> > > Please use $( ... ) instead of `...` and quotes around > "$BASEDIR/etc/vfstab" ... > > >> + grep "[ ]/[ ]" | nawk '{print $4}'` >> + >> + if [ "$rootfstype" = "zfs" ] ; then >> > > Please use [[ ]] instead of [ ] ... > > >> + rootpool=`grep -v "^#" $BASEDIR/etc/vfstab | \ >> + grep "[ ]/[ ]" | nawk '{print $1}' | \ >> + sed 's,/.*,,'` >> > > Please use $( ... ) instead of `...` and quotes around > "$BASEDIR/etc/vfstab" ... > > > >> + rootdataset=`grep -v "^#" $BASEDIR/etc/vfstab | \ >> + grep "[ ]/[ ]" | nawk '{print $1}'` >> + >> > > Please use $( ... ) instead of `...` and quotes around > "$BASEDIR/etc/vfstab" ... > > [Ok, enougth review bickering for this script, the remaning issues are > always the same as listed above: Quotes, use (( )) for math expressions > and comparisaions, use [[ ]] instead of [ ], use $( ... ) instead of > `...` etc. etc. etc. (all stuff listed in > http://www.opensolaris.org/os/project/shell/shellstyle/) ] > > > > 5. new/tools/extract_postrun > >> +if [ $# != 3 ] ; then >> > > Please use (( )) when comparing numbers/integer variables, e.g. this > should be > -- snip -- > if (( $# != 3 )) ; then > -- snip -- > > >> + echo "$0: svr4_pkg_list svr4_pkg_pool dst_dir" >> > > Please replace "echo" with "print" > > >> +fi >> + >> +svr4_pool=$2 >> +dst=$3 >> +num=1 >> + >> +if [ ! -d $dst ] ; then >> > > Plese use [[ ]] instead of [ ] ... > > >> + mkdir -p $dst >> > > Please put quotes around "$dst" ... > > >> +fi >> + >> +cat $1 | \ >> > > Please put quotes around "$1" > > >> +while read line ; do >> + myfile=$svr4_pool/$line/install/postinstall >> + if [ -f $myfile ] ; then >> > > Please use [[ ]] instead of [ ] ... > > >> + grep -il postrun $myfile > /dev/null 2>& 1 >> + if [ "$?" = "0" ] ; then >> > > Please use (( )) when comparing numbers/integer/float variables, e.g. > this should be > -- snip -- > if (( $? == 0 )) ; then > -- snip -- > > >> + ff=$dst/$line.postinstall.$num >> > > Please use quotes... > > >> + cp $myfile $ff.1 >> > > Please use quotes and use '{'/'}' around "ff" to avoid the ambigous case > of compound variables, e.g. this should be > -- snip -- > cp "$myfile" "${ff}.1" > -- snip -- > > >> + # Change "( echo" into "echo" >> + sed 's/^( echo/echo/g' $ff.1 > $ff.2 >> > > Please change this to > -- snip -- > sed 's/^( echo/echo/g' "${ff}.1" > "${ff}.2" > -- snip -- > > >> + # Remove any trailing ") | \" >> + sed 's/) | \\$//g' $ff.2 | grep -v BASEDIR > $ff.3 >> + /bin/sh $ff.3 > $ff >> + /bin/rm $ff.1 $ff.2 $ff.3 >> + num=`expr $num + 1` >> > > Ouch... > ... ksh88/ksh93/bash have builtin integer math, e.g. this could be > either > -- snip -- > (( num=num+1` )) > -- snip -- > for ksh88/ksh93/bash > or > -- snip -- > (( num++ )) > -- snip -- > for ksh93 > > >> + fi >> + fi >> + >> +done >> + >> +cat << \EXEC_SCRIPT > $dst/exec_postrun >> > > Please put quotes around "$dst/exec_postrun" ... > > >> +#!/bin/sh >> + >> +# This executes all the postinstall scripts in the current directory. >> +cd /postrun_scripts >> + >> +# Find out how many scripts are there >> +num_scripts=`ls *postinstall*|wc -l|sed 's/^ *//g` >> > > Please use $( ... ) and not `...` > > >> +num=1 >> + >> +while [ $num -le $num_scripts ] ; do >> > > Please use (( )) for comparing numbers, e.g. (( num <= num_scripts )). > BTW: If you use ksh93 you can save the "num++" below and replace the > "num=1 ; while [ $num -le $num_scripts ] ; do" with > -- snip -- > for (( num=1 ; num <= num_scripts ; num++ )) ; do > -- snip -- > > >> + >> + exec_name=`echo *.$num` >> > > Please use $( ... ) instead of `...` ... > > >> + echo "Executing $exec_name" >> + /bin/sh $exec_name > /dev/null 2>& 1 >> > > Please put quotes around "$exec_name". > > >> + if [ "$?" != "0" ] ; then >> > > Please replace this with > -- snip -- > if (( $? != 0 )) ; then > -- snip -- > > >> + echo "$exec_name failed" >> + fi >> + num=`/bin/expr $num + 1` >> > > See previous issue, ksh88/ksh93/bash have builtin math... > > >> +done >> > > > > ---- > > Bye, > Roland > >
