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